bun:sqlite: fix use-after-free closing a database with FTS5 tables

[robobun]

Fixes #37044

Repro

import { Database } from 'bun:sqlite';

const db = new Database(':memory:');
db.exec('CREATE VIRTUAL TABLE notes_fts USING fts5(body)');
db.query("SELECT rowid FROM notes_fts WHERE notes_fts MATCH 'hello'").all();
db.close(); // panic(main thread): Segmentation fault at address 0x28

Crashes in sqlite3Fts5IndexClose / fts5DisconnectMethod under sqlite3_close. Reproduces on Linux and macOS; ASAN reports a heap-use-after-free with sqlite3_finalize (from jsSQLStatementCloseStatementFunction) as the free site and sqlite3Fts5IndexClose as the read.

Cause

Regression from #36573. close() sweeps every prepared statement on the connection via sqlite3_next_stmt() and finalizes the ones bun isn't keeping. Virtual-table modules like FTS5 cache their own internal prepared statements and finalize them themselves during vtab disconnect inside sqlite3_close*, so the sweep finalized them first and the disconnect then called sqlite3_finalize on dangling pointers. This also matches the reporter's observations: DROP TABLE before close() avoids it (teardown goes through xDestroy while the statements are still valid), as does never calling close().

Since #36573, every bun-created statement wrapper is tracked in VersionSqlite3::statements, so the sweep's only remaining job was the transient statement in db.run() when a bound-parameter getter re-entrantly closes the database mid-bind.

Fix

Only finalize the statements bun tracks. If untracked statements still make sqlite3_close() return SQLITE_BUSY (the re-entrant db.run() case), retire the handle with sqlite3_close_v2(), which defers the close until the owner finalizes them, instead of finalizing other code's statements behind its back. The db.run() path now lets its transient statement be finalized by its scoped owner, which completes the deferred close.

Verification

New test fails on the unfixed build (segfault) and passes with the fix. bun bd test test/js/bun/sqlite/sqlite.test.js 114 pass (including the #36573/#36793 close semantics tests), test/js/node/sqlite/ 115 pass, test/regression/issue/14709.test.ts 5 pass. The reporter's 500-iteration loop prints survived under the ASAN debug build.

---

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

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

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: 9a90414d-75bd-4887-971b-dcd57ed720a5

📥 Commits

Reviewing files that changed from the base of the PR and between 97187c8 and 7f86be2.

📒 Files selected for processing (2)

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

Walkthrough

SQLite statement closing now defers database shutdown until tracked statements drain, preserves retained prepared statements for close(false), and uses sqlite3_close_v2() for remaining statements. FTS5 tests cover repeated close operations and file release after final statement finalization.

Changes

SQLite close lifecycle

Layer / File(s)	Summary
Close and finalization behavior src/jsc/bindings/sqlite/JSSQLStatement.cpp	closeIfDrained() is defined after JSSQLStatement. Re-entrant query cleanup finalizes transient statements. Database closing finalizes Bun-owned statements and defers completion for retained statements.
FTS5 lifecycle regression coverage test/js/bun/sqlite/sqlite.test.js	Tests cover repeated FTS5 database closes with both close modes and deletion after the final prepared statement is finalized.

Possibly related issues

• bun:sqlite segfaults in FTS5 vtab teardown on Database.close() (macOS, 1.4.0-canary) #37044 — Addresses related SQLite/FTS5 teardown crashes and adds lifecycle regression tests.
• bun:sqlite: close() can't finalize db.query() statements past MAX_QUERY_CACHE_SIZE, so close(true) throws "database is locked" #36572 — Covers SQLite statement finalization during database closure, including retained statements and close(true) behavior.

Possibly related PRs

• oven-sh/bun#36573 — Modifies SQLite statement lifecycle and database-close finalization.
• oven-sh/bun#36793 — Extends SQLite close lifecycle handling with deferred closure and close(false) behavior.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the bun:sqlite closing bug and the FTS5 use-after-free fix.
Description check	✅ Passed	The description explains the failure, cause, fix, regression coverage, and verification results, although it does not use the template headings.
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]

Good catch on closeIfDrained(): it gated on sqlite3_next_stmt() returning null, which never happens while FTS5 holds its cached statements, so a close() deferred by a kept prepare() statement never completed and the file handle stayed open until GC. Fixed in 97187c8 by draining on the bun-tracked statement list instead (sqlite3_close_v2 handles vtab-owned statements via disconnect), with a test covering the FTS5 + prepare() + close() + finalize() path.

[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`:
- Line 1796: Replace the dynamic require in the child fixture with a static
Database import from bun:sqlite, leaving the fixture’s usage unchanged and
avoiding unrelated test changes.

🪄 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: 3140b3b3-8c2c-4da9-9a8f-fbf01ec8f5cf

📥 Commits

Reviewing files that changed from the base of the PR and between be6a334 and 97187c8.

📒 Files selected for processing (2)

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

[claude]

LGTM — the follow-up on closeIfDrained() addresses my earlier concern; it now drains on the bun-tracked statement list and sqlite3_close_v2 handles vtab-owned statements via disconnect.

What was reviewed:

• Traced close()/close(true) with FTS5: sqlite3Close runs disconnectAllVtab before connectionIsBusy, so FTS5's cached statements are gone before the busy check; the SQLITE_BUSY fallback is only reachable for the re-entrant db.run() transient statement.
• Verified AutoDestructingSQLiteStatement finalizes sql.stmt on scope exit against a zombified connection, completing the deferred close (previously the sweep freed it mid-bind, so this is strictly safer).
• Checked ~JSSQLStatement removes this from statements before closeIfDrained() iterates it, and jsSQLStatementFunctionFinalize nulls stmt first — no self-blocking on the drain check.

Extended reasoning...

Overview

Fixes a heap-use-after-free when db.close() is called on a bun:sqlite database that has an FTS5 virtual table. #36573 introduced a sweep over sqlite3_next_stmt() that finalized every statement on the connection, including FTS5's internally-cached prepared statements; FTS5 then finalized those same (freed) statements again during vtab disconnect inside sqlite3_close. The fix stops touching statements bun doesn't own: close() now iterates only versionDB->statements (bun's tracked wrappers), and if sqlite3_close() reports SQLITE_BUSY for a genuinely-untracked transient statement (the re-entrant db.run() bind case), it falls back to sqlite3_close_v2() which zombifies until the scoped owner finalizes.

I flagged a follow-up in my first pass: closeIfDrained() still gated on !sqlite3_next_stmt(db, nullptr), which never goes null while FTS5 holds cached statements, so a close(false)-deferred close would never complete for FTS5 databases. That was fixed in 97187c8 — closeIfDrained() now checks whether every bun-tracked JSSQLStatement::stmt is null and calls closeHandle() (sqlite3_close_v2), which handles vtab-owned statements via disconnectAllVtab. A new test covers exactly that path (FTS5 + prepare() + close() + finalize() → rmSync).

Security risks

None. This is a crash/UAF fix — it narrows what bun finalizes to what bun owns, which is the correct ownership discipline. No new user-controlled input reaches native code.

Level of scrutiny

High — native C++ memory-safety code in a widely-used API. I traced each path:

• close(false) with FTS5 only: bun's statements are finalized, keptAny=false, sqlite3_close_v2 runs disconnectAllVtab (FTS5 finalizes its own), returns OK.
• close(true) with FTS5 only: same, via sqlite3_close (disconnect precedes the busy check in sqlite3Close).
• Re-entrant db.close() from a bound-parameter getter during db.run(): the transient AutoDestructingSQLiteStatement is not in versionDB->statements; sqlite3_close_v2 zombifies, versionDB->db=nullptr, the caller detects handle()!=db, throws, and ~AutoDestructingSQLiteStatement finalizes the transient statement on the zombie — completing the close. Covered by the existing "run() reports a closed database when a bound parameter's getter closes it" test.
• closeIfDrained() callers: jsSQLStatementFunctionFinalize nulls stmt before calling it; ~JSSQLStatement removes this from the set before calling it. Neither self-blocks the drain.
• SQLITE_BUSY fallback narrowed from != SQLITE_OK && force to == SQLITE_BUSY: sqlite3_close only returns OK/BUSY/MISUSE, and sqlite3_close_v2 never returns BUSY, so the non-force path never hits the fallback and MISUSE correctly still surfaces via the later throw check.

Other factors

Two new tests (subprocess crash-repro looping over both close(true)/close(false), and the FTS5+prepare deferred-close file-release check) plus the full existing close-semantics suite (114 pass in sqlite.test.js, 115 in node/sqlite, 5 in the #14709 regression) per the PR description, verified under ASAN. All prior review feedback (mine, comment-cop on comment length, CodeRabbit on static import) has been addressed and resolved. The fix is architecturally the right layer: stop finalizing what bun doesn't own, let SQLite's documented zombie/disconnect semantics handle the rest.

[Jarred-Sumner]

Why does this not feel like the correct fix?isn’t the issue that we’re closing statements not ones by us? The issue is not that we’re closing our statements.

[robobun]

Right, that is exactly the bug and exactly what this changes. #36573 added a sqlite3_next_stmt() sweep to close() that finalized every statement on the connection, including statements bun never created: FTS5 keeps its own prepared statements and finalizes them during vtab disconnect inside sqlite3_close, so after the sweep it double-finalized freed handles. The fix deletes that sweep, so close() now only finalizes the statements bun tracks in versionDB->statements (which since #36573 is every wrapper bun creates).

The other two hunks are fallout from the sweep having done double duty:

• The sweep was what guaranteed sqlite3_close() could not return SQLITE_BUSY (it covered the one statement bun creates but does not track: db.run()'s transient statement, when a bound-parameter getter re-entrantly closes the db mid-bind). Without the sweep, that case falls back to sqlite3_close_v2(), which defers the close until the statement's scoped owner finalizes it, instead of finalizing it out from under the stack frame that is still using it.
• closeIfDrained() used sqlite3_next_stmt() == NULL as its "all drained" check, which never becomes true while FTS5 holds its statements, so a deferred close(false) would never complete. It now checks the tracked list instead.

No behavior change for bun-owned statements: they are all finalized on close(true), and close(false) still keeps prepare() statements usable.

[iOSonntag]

so whats the plan here?

[robobun]

The fix is complete and CI is green across all platforms, including regression tests for your exact repro (the close() crash loop and the deferred-close file release with FTS5). It is now waiting on maintainer review and merge; once merged it will be in the next canary build.