diff --git a/src/jsc/bindings/sqlite/JSSQLStatement.cpp b/src/jsc/bindings/sqlite/JSSQLStatement.cpp index 09cc3b05683e..d1511546b732 100644 --- a/src/jsc/bindings/sqlite/JSSQLStatement.cpp +++ b/src/jsc/bindings/sqlite/JSSQLStatement.cpp @@ -230,11 +230,8 @@ class VersionSqlite3 { sqlite3_close_v2(std::exchange(db, nullptr)); } - void closeIfDrained() - { - if (closed && db && !sqlite3_next_stmt(db, nullptr)) - closeHandle(); - } + // Defined after JSSQLStatement: needs its definition to inspect `stmt`. + void closeIfDrained(); void release() { @@ -1554,7 +1551,8 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteFunction, (JSC::JSGlobalObject * l SQLiteBindingsMap bindings { static_cast(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. if (!scope.exception()) throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s)); return {}; @@ -1861,34 +1859,33 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementCloseStatementFunction, (JSC::JSGlobalObj 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 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()); } + // Remaining statements are not bun's to finalize: vtab modules (FTS5) + // finalize their cached statements during disconnect inside sqlite3_close*, + // and a re-entrant close() from a bound-parameter getter leaves db.run()'s + // transient statement live on this stack (close_v2 defers until it drains). 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; @@ -3002,3 +2999,16 @@ JSValue createJSSQLStatementConstructor(Zig::GlobalObject* globalObject) } } // namespace WebCore + +// Drained = every bun-tracked statement finalized. Statements sqlite3 still +// holds (vtab modules' cached ones) don't count; close_v2 finalizes those. +void VersionSqlite3::closeIfDrained() +{ + if (!closed || !db) + return; + for (auto* statement : statements) { + if (statement->stmt) + return; + } + closeHandle(); +} diff --git a/test/js/bun/sqlite/sqlite.test.js b/test/js/bun/sqlite/sqlite.test.js index dd67228c37f1..7307ae9e3b47 100644 --- a/test/js/bun/sqlite/sqlite.test.js +++ b/test/js/bun/sqlite/sqlite.test.js @@ -1789,6 +1789,51 @@ 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 = ` + import { Database } from "bun:sqlite"; + 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("close() releases an FTS5 database file once the last prepare() statement is finalized (#37044)", () => { + using dir = tempDir("sqlite-close-unlink-fts5", {}); + const file = path.join(String(dir), "x.sqlite"); + const db = new Database(file); + db.exec("CREATE VIRTUAL TABLE t USING fts5(a)"); + // FTS5 caches internal prepared statements on the connection; they must not + // keep the deferred close from ever happening. + db.query("SELECT rowid FROM t WHERE t MATCH 'x'").all(); + const stmt = db.prepare("SELECT 1"); + db.close(); + stmt.finalize(); + // On Windows rmSync throws EBUSY if the handle stayed open past the last finalize. + rmSync(file); + expect(existsSync(file)).toBe(false); +}); + it("should dispose even if a prepared statement is still live", () => { let prepared; expect(() => {