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

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<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.
                     if (!scope.exception())
                         throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
                     return {};
@@ -1861,34 +1859,36 @@ 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<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());
     }
 
+    // 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.
     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 +3002,17 @@ JSValue createJSSQLStatementConstructor(Zig::GlobalObject* globalObject)
 }
 
 } // namespace WebCore
+
+// Drained means every statement bun tracks is finalized. Statements sqlite3
+// still knows about (virtual-table modules' cached statements) don't count:
+// sqlite3_close_v2 finalizes them via vtab disconnect.
+void VersionSqlite3::closeIfDrained()
+{
+    if (!closed || !db)
+        return;
+    for (auto* statement : statements) {
+        if (statement->stmt)
+            return;
+    }
+    closeHandle();
+}

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 = `
+    const { Database } = require("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(() => {