bun:sqlite: fix stale Structure reads when a getter mutates the params object during bind

[robobun]

Problem

bun:sqlite binds a params object by capturing target->structure() once in rebindObject and reusing that Structure's property offsets for every parameter. Property reads between parameters can run user JS (a named getter on the params object, or an index getter for an anonymous ? parameter). If that getter deletes and adds properties, the object transitions to a new Structure, and the next parameter's lookup through the old Structure reads whatever now occupies the stale offset:

• delete + add: the value of an unrelated, non-parameter property gets silently bound into the query
• delete only: the stale offset holds an empty slot, and reading it crashes (release: Segmentation fault at address 0x5; debug WebKit: ASSERTION FAILED: value in JSObject::getOwnNonIndexPropertySlot, JSObject.h:1147)

Repro (wrong value bound):

import { Database } from "bun:sqlite";
const db = new Database(":memory:", { strict: true });
const q = db.query("select ?1 as a, $b as b, $c as c");
const t = {};
t[0] = "i";
Object.defineProperty(t, "b", {
  enumerable: true,
  get() {
    delete t.c;
    t.secret = "SECRET-NOT-A-PARAM"; // remove this line -> segfault instead
    return "two";
  },
});
t.c = "three";
console.log(q.all(t)); // c: "SECRET-NOT-A-PARAM", expected a missing-parameter error

The same mechanism hits the default (non-strict) mode through an index getter for an anonymous ? parameter, where the stale offset feeds fastGetOwnProperty.

Fix

In rebindObject (src/jsc/bindings/sqlite/JSSQLStatement.cpp), stop caching the Structure across parameters:

• the ?N/$name mixed path re-reads target->structure() for each getOwnNonIndexPropertySlot call
• the named-parameters fast path and the generic slot path are merged into one loop that re-reads the Structure and re-decides canUseFastGetOwnProperty per parameter, so an object that gains getters mid-bind falls back to a full getOwnPropertySlot lookup instead of reading a raw offset

node:sqlite (NodeSqlite.cpp) is not affected; it looks parameters up by name on each read.

Verification

Five new tests in test/js/bun/sqlite/sqlite.test.js cover both modes: foreign-value bind now throws Missing parameter "$c" (strict) or binds NULL (default), the delete-only crash now errors cleanly in a spawned child, and when the getter re-adds the parameter the currently held value is bound. All five fail on bun 1.4.0-canary (two with wrong values, one TypeError, one segfault) and pass with this change; the full test/js/bun/sqlite/ suite passes (131 tests).

---

no test proof · iteration 0 · 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]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 39 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 72 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 923447b1-df60-461d-8e86-1e6903172934

📥 Commits

Reviewing files that changed from the base of the PR and between c3995e4 and 30a466e.

📒 Files selected for processing (2)

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

Walkthrough

Changes

SQLite binding mutation handling

Layer / File(s)	Summary
Current-structure parameter lookup src/jsc/bindings/sqlite/JSSQLStatement.cpp	rebindObject re-reads the target structure for each parameter. Named-parameter lookup selects the current fast or generic path and retains missing-parameter handling and statement lifetime validation.
Mutation regression coverage test/js/bun/sqlite/sqlite.test.js	Tests cover getter-driven mutations, strict and default modes, bind-time values, later getters, and catchable missing-parameter errors.

Possibly related PRs

• oven-sh/bun#32841: Both modify SQLite parameter rebinding in JSSQLStatement.cpp.
• oven-sh/bun#37109: Both modify SQLite named and object parameter binding behavior.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the SQLite fix for stale Structure reads during parameter binding.
Description check	✅ Passed	The description explains the problem, fix, affected code, and verification results, although it uses different headings from the template.
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.

---

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

[robobun]

^{Updated 10:06 PM PT - Aug 16th, 2026}

✅ @Jarred-Sumner, your commit 30a466ef6e5a1a8276da20f519e3383f1cda7b25 passed in Build #99860! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37212

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

bun-37212 --bun

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/sqlite/sqlite.test.js`:
- Around line 293-296: Remove the exact empty-stderr assertion from this
subprocess regression test while continuing to consume stderr in the existing
Promise.all call. Keep the stdout error-message and exitCode expectations
unchanged, so the test validates the caught missing-parameter error without
depending on stderr silence.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 28da8c45-177f-49ee-adf1-b9549a3f853c

📥 Commits

Reviewing files that changed from the base of the PR and between 9d519e8 and 608e8f3.

📒 Files selected for processing (2)

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

[claude]

I reviewed this PR and found no bugs; the earlier inline feedback (stderr draining, comment length) has been addressed. Because this restructures the named-parameter bind loop in native JSC binding code, a human look is still worthwhile.

What was reviewed:

• The merged loop re-reads target->structure() and re-checks canUseFastGetOwnProperty per parameter; exception checks and statementStillAlive() ordering match the sibling paths.
• The empty-property branch now uses getDirectIndex uniformly, matching the hasOutOfOrderNames and isOnlyIndexed paths (previously the slow path used getOwnPropertySlotByIndex).
• Five new tests cover strict/default modes, foreign-value bind, delete-only crash (spawned), and mid-bind getter installation.

Extended reasoning...

Overview

This PR fixes a memory-safety bug in rebindObject (src/jsc/bindings/sqlite/JSSQLStatement.cpp): the function cached target->structure() once and reused it for every parameter lookup, but property reads between parameters can run user JS (getters) that transitions the object to a new Structure. Looking up through the stale Structure then reads the wrong offset — either binding an unrelated property's value into the query, or reading an empty butterfly slot and segfaulting. The fix re-reads the Structure per parameter and merges the fast/slow named-parameter branches into one loop that re-decides canUseFastGetOwnProperty per iteration. Five new tests in test/js/bun/sqlite/sqlite.test.js cover both modes and both failure shapes.

Security risks

None new. The bug being fixed is itself a memory-safety issue (reading a stale JSC butterfly offset → segfault or wrong value bound). The fix is the canonical JSC pattern for code that may re-enter JS between property reads. No new user-controlled data paths are introduced.

Level of scrutiny

Moderate-to-high. This is native C++ in the JSC bindings layer, which is memory-safety-sensitive per REVIEW.md. The change is well-scoped to one function, but it restructures two branches into a single merged loop and changes when the fast-path check runs (per-iteration vs. once up front). I traced the exception-check ordering against the old paths — statementStillAlive() runs after the value read, then !hasProperty && !scope.exception() gates the missing-parameter throw, then RETURN_IF_EXCEPTION catches anything from getOwnPropertySlot/slot.getValue/get — and it looks correct. The empty-property (anonymous ?) branch now uses getDirectIndex in all cases, matching what the old fast path and the hasOutOfOrderNames/isOnlyIndexed paths already do; the old slow path used getOwnPropertySlotByIndex there, which is a small semantic unification worth a maintainer's glance.

Other factors

All prior review threads are resolved: the spawned crash test now drains and asserts stderr (608e8f3), and both explanatory comments were collapsed to one line each (9960202, d836659). The bug-hunting pass on the current HEAD found nothing. Test coverage is thorough — strict and default modes, delete+add vs. delete-only, and the case where a getter installs a new getter for a later parameter mid-bind (which exercises the per-iteration canUseFastGetOwnProperty re-check). Given this is a non-trivial control-flow merge in JSC binding code on a hot path (every object-param bind), a maintainer familiar with JSC internals should confirm the restructuring and its perf impact are acceptable rather than merging on automated review alone.

[claude]

I reviewed this PR and didn't find any bugs; the earlier feedback (stderr drain, comment length) has been addressed and all threads are resolved. Because it restructures the named-parameter bind loop in JSC C++ bindings — merging the fast/slow paths and changing the empty-property indexed lookup from getOwnPropertySlotByIndex to getDirectIndex — a human look would still be worthwhile.

What was reviewed:

• Per-iteration target->structure() re-read in both the hasOutOfOrderNames path and the merged named-parameter loop; canUseFastGetOwnProperty is re-decided per parameter so a mid-bind getter falls through to the slot path.
• Exception handling in the merged loop: getOwnPropertySlot/slot.getValue throws are gated by !scope.exception() and caught by the RETURN_IF_EXCEPTION after statementStillAlive().
• The five new tests cover strict and default modes, delete+add vs delete-only, and indexed getters via getDirectIndex; the crash repro is spawned and asserts stderr/stdout/exit.

Extended reasoning...

Overview

This PR fixes a stale-Structure read in rebindObject (src/jsc/bindings/sqlite/JSSQLStatement.cpp). Previously the params object's Structure* was hoisted above the bind loop; a getter on one parameter that adds/deletes properties transitions the object to a new Structure, and the next lookup through the old Structure reads a stale offset — either an unrelated property's value (silently bound into the query) or an empty slot (segfault). The fix re-reads target->structure() per parameter and merges the previous fast-path (canUseFastGetOwnProperty) and slow-path (getOwnPropertySlot) branches into one loop that re-decides the fast-path predicate each iteration. Five tests are added covering strict and default modes.

Security risks

None material. The bug is user-controlled JS re-entry causing a stale-offset read into the same object's butterfly — not attacker-controlled memory. The fix strictly narrows the window by re-validating per iteration. No new external input is parsed.

Level of scrutiny

High. This is C++ JSC bindings code in the most-blocked review category (native memory safety, re-entrancy). The change restructures control flow: two branches become one, and the empty-property indexed lookup in what was the slow path switches from getOwnPropertySlotByIndex + slot.getValue to getDirectIndex (matching the other three branches). I traced the exception paths — getOwnPropertySlot and slot.getValue can throw; both are guarded by !scope.exception() before use and caught by RETURN_IF_EXCEPTION after the statementStillAlive() check — and confirmed getDirectIndex invokes indexed getters (the two default-mode tests depend on it). The change looks correct, but the branch merge in a hot bind path plus the indexed-lookup unification is the kind of restructuring a maintainer should sign off on.

Other factors

All prior review threads are resolved: my earlier stderr-drain comment was fixed in 608e8f3, the comment-cop multi-line-comment flags were collapsed to single lines in 9960202/d836659c0f, and CodeRabbit's stderr-assertion suggestion was declined with a repo-convention justification. Test coverage is thorough — both modes, delete+add and delete-only, a getter installed mid-bind, and a spawned crash regression that asserts stderr/stdout/exit. The bug hunting system found nothing this run.

[Jarred-Sumner]

NOt great perf hit. We can do better later.