bun:sqlite: bind strict-mode numeric named parameters by their literal key

[robobun]

Problem

In bun:sqlite with strict: true, numeric named parameters ($1, :1, @1) are looked up at object key N-1 instead of key N, and $0 underflows to key "4294967295":

import { Database } from "bun:sqlite";
const db = new Database(":memory:", { strict: true });

db.query("select $1 as a").all({ 1: "one" });  // throws: Missing parameter "$1"
db.query("select $1 as a").all({ 0: "zero" }); // [{ a: "zero" }]  (binds the neighboring key)
db.query("select $0 as a").all({ "4294967295": "w" }); // [{ a: "w" }]  ((0 - 1) as unsigned)

The documented strict-mode contract is that object keys are the parameter name without its prefix ($name binds from { name }), so $1 should bind from key 1. When both key 0 and key 1 exist, the off-by-one silently binds the wrong value.

Cause

rebindObject in JSSQLStatement.cpp treats every digit-leading trimmed name as SQLite's ?NNN positional form: it parses the digits and reads getDirectIndex(value - 1), applying 1-based-position-to-0-based-index arithmetic to what is actually an object key. For $0 the subtraction wraps the unsigned index. There is no memory-safety impact: the wrapped index degrades to an ordinary missing property.

Fix

Distinguish the two forms by the parameter's prefix character:

• ?NNN is a 1-based position and keeps the array-like 0-based mapping (?1, ?2 still binds from { 0: ..., 1: ... }, matching arrays and the existing tests). This branch is unchanged from before; SQLite rejects ?0 at prepare time, so N - 1 cannot underflow here.
• $N / :N / @N are names; they now bind from the literal key via JSC::parseIndex, which applies canonical array-index semantics. Canonical numeric keys (1, 0) are index properties that the named-slot lookup cannot see, so they go through getDirectIndex(N); non-canonical digit names ($01, $4294967295, $1abc) fall through to the regular named lookup and bind from their literal key.

Arrays, non-numeric names, and default (non-strict) mode are unchanged.

Verification

New tests in test/js/bun/sqlite/sqlite.test.js cover $N/:N/@N binding from key N, out-of-order numeric names, the $0 wraparound, non-canonical digit names, mixed word/numeric names, and that ?N object binding and default-mode prefixed keys behave as before. 5 of the 7 fail on the previous build; all pass with the fix, and the full sqlite suite (133 tests) passes.

---

no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/sqlite/sqlite.test.js

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eba616a3-3af0-4ad3-aaed-e5f5fa5d3545

📥 Commits

Reviewing files that changed from the base of the PR and between 6e35c79 and 1eeef8b.

📒 Files selected for processing (2)

• src/jsc/bindings/sqlite/JSSQLStatement.cpp
• test/js/bun/sqlite/sqlite.test.js

Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review.

---

Walkthrough

Changes

SQLite parameter binding

Layer / File(s)	Summary
Numeric parameter lookup and regression coverage src/jsc/bindings/sqlite/JSSQLStatement.cpp, test/js/bun/sqlite/sqlite.test.js	?NNN parameters use zero-based lookup after subtracting one. $N, :N, and @N parameters use direct numeric lookup. Other names retain named-property lookup. Strict-mode tests cover valid, invalid, mixed, positional, and default-mode cases.

Possibly related PRs

• oven-sh/bun#37109: Modifies SQLite named-parameter binding behavior.
• oven-sh/bun#37211: Modifies SQLite parameter binding and numbered-parameter tests.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description clearly explains the problem, cause, fix, and verification, and it provides the information required by the template.
Title check	✅ Passed	The title clearly and concisely describes the main change to strict-mode numeric named parameter binding.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Good catch on the strlen(nullptr) in the missing-parameter message. It is pre-existing (also reachable on the base commit without mixing forms, e.g. select ?2 as x bound with {} in strict mode, since the numbering gap leaves position 1 unnamed), and it is already fixed by the open #37211, which adds exactly the suggested guard (name ? ... : "?" + (i + 1)) plus tests for the ?N gap case. Since that PR touches the same line, I left the guard out of this one to avoid conflicting changes; this PR stays scoped to the numeric-name lookup.

[robobun]

CI status: 195 of 196 jobs passed. The one red lane (debian 13 x64-asan) fails only on test/js/web/workers/worker-terminate-funnels.test.ts, a leak that is unrelated to this change and reproduces on main; it is being tracked separately. All sqlite tests pass on every lane, so this is ready for review.

[robobun]

Rebased onto main after #37211 and #37212 landed. The only conflict was in sqlite.test.js, where both sides appended a test block at the same spot; kept both. The source change is unchanged. sqlite suite passes (140 tests), and the new tests still fail without the fix on current main.

[robobun]

^{Updated 9:33 PM PT - Aug 17th, 2026}

✅ @robobun, your commit 77ec31d1f550977eae2968b46b6dc398887cf36a passed in Build #100368! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37240

That installs a local version of the PR into your bun-37240 executable, so you can run:

bun-37240 --bun

[robobun]

Pushed 2093ec1 addressing the comment lint: removed the unreachable ?0 guard (SQLite rejects ?0 at prepare time, so the ?N branch is now byte-for-byte the original code) and cut the remaining comment to one line. No behavior change; sqlite suite still passes (140 tests). PR description updated to match.

[robobun]

Rebased onto current main (77ec31d, no source changes). The previous build's binary-size failure was a baseline mismatch: the comparison canary was built from main two commits ahead of this branch's base, and those two commits (#39484, #39486) shrink the binary by roughly the reported delta. This change adds one bool and an inlined parseIndex call. sqlite suite passes on the new base (140 tests).

[claude]

LGTM — the pre-existing strlen(nullptr) I flagged earlier is now guarded on main via #37211, and the comment-cop lint was addressed in 2093ec1.

What was reviewed:

• isPositional is read from name[0] before the += 1 trim and after the nullptr check, so the dereference is safe and the classification is correct for all four prefix chars.
• JSC::parseIndex(*str.impl()) — str is non-null here (branch is gated on name[0] being a digit), and non-canonical forms (01, 4294967295, 1abc) correctly fall through to the named-slot lookup.
• The ?N branch is byte-for-byte the original code; existing ?1, ?2 → {0:…, 1:…} tests and the new "?N still binds from 0-based" test cover it.
• Default (non-strict) mode is unaffected: the whole block is gated on trimLeadingPrefix.

Extended reasoning...

Overview

Eight lines changed in rebindObject's hasOutOfOrderNames branch in JSSQLStatement.cpp, plus a 44-line test block in sqlite.test.js. The change captures whether the untrimmed parameter name starts with ? (positional) vs $/:/@ (named), and only applies the N-1 array-index mapping to the positional form. Named numeric parameters now use JSC::parseIndex so canonical numeric keys resolve via getDirectIndex(N) and non-canonical digit strings fall through to the named-property lookup.

Security risks

None. This is a lookup-key selection change inside the strict-mode object-binding path. No untrusted-length arithmetic, no allocation, no new user-JS re-entry (getDirectIndex was already called here). The old $0 → getDirectIndex(-1 as unsigned) wraparound was not memory-unsafe (just a property miss), and the new path cannot underflow.

Level of scrutiny

Medium — native C++ binding code, but a very small localized change to an existing lambda with no new control flow beyond one if/else if. The ?N branch is identical to the original code. name[0] is dereferenced only after the existing name == nullptr guard, and str.impl() is non-null because the branch is gated on the trimmed name starting with a digit. JSC::parseIndex is the correct primitive: it applies the exact canonical-array-index rules JS uses to distinguish index properties from named properties, so getDirectIndex and getOwnNonIndexPropertySlot are chosen consistently with how JSC actually stores the key.

Other factors

All prior feedback is resolved: my earlier pre-existing-segfault note is now fixed on main via #37211 (visible at line 1041 in the current tree), and the comment-cop lint was addressed by trimming to a one-line comment and dropping the unreachable ?0 guard. Seven new tests cover the full variant matrix ($/:/@, out-of-order, $0, non-canonical digits, mixed word/numeric, ?N preservation, default-mode preservation); the author verified 5/7 fail without the fix and the full sqlite suite (140 tests) passes. CI on the previous revision was 195/196 with an unrelated worker-leak flake.