Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions src/jsc/bindings/sqlite/JSSQLStatement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1554,7 +1554,8 @@
SQLiteBindingsMap bindings { static_cast<uint16_t>(count > -1 ? count : 0), strict };
JSC::JSValue reb = rebindStatement(lexicalGlobalObject, bindingsAliveScope.value(), scope, db, versionDB, sql.stmt, bindings, safeIntegers, nullptr);
if (versionDB->handle() != db) [[unlikely]] {
sql.stmt = nullptr; // close() during binding already finalized it via the sqlite3_next_stmt() sweep
// close() during binding deferred sqlite3_close via close_v2;
// finalizing sql.stmt on scope exit completes it.
Comment thread
robobun marked this conversation as resolved.
if (!scope.exception())
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
Expand Down Expand Up @@ -1861,34 +1862,36 @@
return JSValue::encode(jsUndefined());
}

// close(false) keeps db.prepare() statements usable and defers sqlite3_close until they drain; everything else is finalized now.
WTF::HashSet<sqlite3_stmt*> kept;
// close(false) keeps db.prepare() statements usable and defers sqlite3_close until they drain; everything else bun owns is finalized now.
bool keptAny = false;
for (auto* statement : versionDB->statements) {
if (!statement->stmt)
continue;
if (!force && !statement->ownedByDatabase) {
kept.add(statement->stmt);
keptAny = true;
continue;
}
sqlite3_finalize(statement->stmt);
statement->stmt = nullptr;
statement->finalizedByClose = true;
}
for (sqlite3_stmt* stmt = sqlite3_next_stmt(db, nullptr); stmt;) {
sqlite3_stmt* next = sqlite3_next_stmt(db, stmt);
if (!kept.contains(stmt))
sqlite3_finalize(stmt);
stmt = next;
}

versionDB->closed = true;
if (!kept.isEmpty()) {
if (keptAny) {
return JSValue::encode(jsUndefined());
}

Check failure on line 1882 in src/jsc/bindings/sqlite/JSSQLStatement.cpp

View check run for this annotation

Claude / Claude Code Review

closeIfDrained() never fires with FTS5: file handle held past last finalize()

`closeIfDrained()` still gates on `!sqlite3_next_stmt(db, nullptr)`, which is non-null whenever FTS5 (or any vtab that caches statements) is present — the same extension-owned statements this PR correctly stops finalizing. So after `close(false)` keeps a `db.prepare()` statement and the user later finalizes it, `closeIfDrained()` sees FTS5's cached statements and skips `closeHandle()`, leaving the sqlite3 handle and file descriptor open until GC (breaking "close(false) releases the file once the
Comment thread
robobun marked this conversation as resolved.

// Statements bun doesn't track may still be live on the connection:
// virtual-table modules (e.g. FTS5) cache their own prepared statements
// and finalize them during vtab disconnect inside sqlite3_close*, and a
// re-entrant close() from a bound-parameter getter leaves db.run()'s
// transient statement alive on this stack. Never finalize those behind
// their owner's back; on SQLITE_BUSY, close_v2 defers the close until
// they drain.
Comment thread
robobun marked this conversation as resolved.
Outdated
int statusCode = force ? sqlite3_close(db) : sqlite3_close_v2(db);
if (statusCode != SQLITE_OK && force) {
if (statusCode == SQLITE_BUSY) {
sqlite3_close_v2(db);
statusCode = SQLITE_OK;
}
versionDB->db = nullptr;

Expand Down
29 changes: 29 additions & 0 deletions test/js/bun/sqlite/sqlite.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1789,6 +1789,35 @@ it("close() releases the database file when only query() statements are outstand
expect(existsSync(file)).toBe(false);
});

it("close() does not crash with FTS5 virtual tables (#37044)", async () => {
// close() must not finalize FTS5's internal prepared statements behind the
// vtab's back; doing so use-after-frees in sqlite3_close's vtab disconnect.
const src = `
const { Database } = require("bun:sqlite");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
for (let i = 0; i < 10; i++) {
const db = new Database(":memory:");
db.exec("CREATE VIRTUAL TABLE notes_fts USING fts5(body)");
db.exec("INSERT INTO notes_fts(body) VALUES ('hello world'), ('goodbye moon')");
db.query("SELECT rowid FROM notes_fts WHERE notes_fts MATCH 'hello'").all();
db.query("SELECT count(*) c FROM notes_fts").get();
db.close(i % 2 === 0);
}
console.log("survived");
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", src],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout.trim()).toBe("survived");
expect(exitCode).toBe(0);
});

it("should dispose even if a prepared statement is still live", () => {
let prepared;
expect(() => {
Expand Down