Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
10 changes: 5 additions & 5 deletions docs/runtime/sqlite.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,15 @@ const db = new Database("./mydb.sqlite");

### `.close(throwOnError: boolean = false)`

To close a database connection but allow existing queries to finish, call `.close(false)`:
To close a database connection, call `.close()`:

```ts db.ts icon="/icons/typescript.svg" highlight={3}
const db = new Database();
// ... do stuff
db.close(false);
db.close();
```

To close the database and throw an error if there are any pending queries, call `.close(true)`:
Any prepared statements that were not finalized are finalized as part of closing, so the connection (and the database file) is released immediately. Using a statement after its database was closed throws an error. Pass `true` to throw if the connection fails to close:

```ts db.ts icon="/icons/typescript.svg" highlight={3}
const db = new Database();
Expand All @@ -129,8 +129,8 @@ db.close(true);
```

<Note>
`close(false)` is called automatically when the database is garbage collected. It is safe to call multiple times but
has no effect after the first.
`close()` is called automatically when the database is garbage collected. It is safe to call multiple times but has
no effect after the first.
</Note>

### `using` statement
Expand Down
14 changes: 7 additions & 7 deletions packages/bun-types/sqlite.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,25 +264,25 @@ declare module "bun:sqlite" {
/**
* Close the database connection.
*
* It is safe to call this method multiple times. If the database is already
* closed, this is a no-op. Running queries after the database has been
* closed throws an error.
* Prepared statements that were not finalized are finalized as part of
* closing, so the connection is released immediately. It is safe to call
* this method multiple times. If the database is already closed, this is
* a no-op. Running queries or using statements after the database has
* been closed throws an error.
*
* @example
* ```ts
* db.close();
* ```
* This is called automatically when the database instance is garbage collected.
*
* Internally, this calls `sqlite3_close_v2`.
* Internally, this calls `sqlite3_close`.
*/
close(
/**
* If `true`, throw an error if the database is in use
* If `true`, throw an error if the connection fails to close
* @default false
*
* When `true`, this calls `sqlite3_close` instead of `sqlite3_close_v2`.
*
* Learn more in the [sqlite3 documentation](https://www.sqlite.org/c3ref/close.html).
*
* In the future, Bun may default `throwOnError` to `true`, but for backwards compatibility it is `false` by default.
Expand Down
98 changes: 85 additions & 13 deletions src/jsc/bindings/sqlite/JSSQLStatement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,12 +194,20 @@
if (castedThis->stmt == nullptr || castedThis->version_db == nullptr) [[unlikely]] { \
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s)); \
return {}; \
} \
if (castedThis->version_db->finalizedStatementsOnClose) [[unlikely]] { \
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s)); \
return {}; \
}

#define CHECK_PREPARED_JIT \
if (castedThis->stmt == nullptr || castedThis->version_db == nullptr) [[unlikely]] { \
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s)); \
return {}; \
} \
if (castedThis->version_db->finalizedStatementsOnClose) [[unlikely]] { \
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s)); \
return {}; \
}
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(VersionSqlite3);
Expand All @@ -217,6 +225,12 @@
sqlite3* db;
std::atomic<uint64_t> version;
size_t reference_count;
// Set when close() finalized every statement on the connection via
// sqlite3_next_stmt(). Statement wrappers then hold dangling sqlite3_stmt
// pointers and must not pass them to sqlite again. The termination path
// and release() leave this false: they close with sqlite3_close_v2(),
// which keeps outstanding statements valid until each is finalized.
Comment thread
robobun marked this conversation as resolved.
Outdated
bool finalizedStatementsOnClose = false;

void release()
{
Expand Down Expand Up @@ -937,14 +951,20 @@
#undef CHECK_BIND
}

static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindingsMap& bindings, JSC::JSObject* target, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, bool safeIntegers, JSSQLStatement* statement)
static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindingsMap& bindings, JSC::JSObject* target, JSC::ThrowScope& scope, sqlite3* db, VersionSqlite3* versionDB, sqlite3_stmt* stmt, bool safeIntegers, JSSQLStatement* statement)
{
int count = 0;

// Reading a property off `target` can run arbitrary JS (getters, Proxy
// traps), which can call statement.finalize() and free `stmt`. Re-validate
// before touching `stmt` again after any callback into JS.
// traps), which can call statement.finalize() and free `stmt`, or close
// the database, which finalizes `stmt` via sqlite3_next_stmt().
// Re-validate before touching `stmt` again after any callback into JS.
Comment thread
robobun marked this conversation as resolved.
Outdated
const auto& statementStillAlive = [&]() -> bool {
if (versionDB->finalizedStatementsOnClose) [[unlikely]] {
if (!scope.exception())
throwException(globalObject, scope, createError(globalObject, "Database has closed"_s));
return false;
}
if (statement && statement->stmt != stmt) [[unlikely]] {
if (!scope.exception())
throwException(globalObject, scope, createError(globalObject, "Statement has finalized"_s));
Expand Down Expand Up @@ -1117,15 +1137,15 @@
return jsNumber(count);
}

static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, SQLiteBindingsMap& bindings, bool safeIntegers, JSSQLStatement* statement)
static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, JSC::ThrowScope& scope, sqlite3* db, VersionSqlite3* versionDB, sqlite3_stmt* stmt, SQLiteBindingsMap& bindings, bool safeIntegers, JSSQLStatement* statement)
{
sqlite3_clear_bindings(stmt);
JSC::JSArray* array = dynamicDowncast<JSC::JSArray>(values);
bindings.reset(sqlite3_bind_parameter_count(stmt));

if (!array) {
if (JSC::JSObject* object = values.getObject()) {
auto res = rebindObject(lexicalGlobalObject, bindings, object, scope, db, stmt, safeIntegers, statement);
auto res = rebindObject(lexicalGlobalObject, bindings, object, scope, db, versionDB, stmt, safeIntegers, statement);
RETURN_IF_EXCEPTION(scope, {});
return res;
}
Expand Down Expand Up @@ -1154,6 +1174,10 @@
} else {
value = array->getDirectIndex(lexicalGlobalObject, i);
RETURN_IF_EXCEPTION(scope, {});
if (versionDB->finalizedStatementsOnClose) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
}
if (statement && statement->stmt != stmt) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
return {};
Expand Down Expand Up @@ -1534,7 +1558,12 @@
int count = sqlite3_bind_parameter_count(sql.stmt);

SQLiteBindingsMap bindings { static_cast<uint16_t>(count > -1 ? count : 0), strict };
JSC::JSValue reb = rebindStatement(lexicalGlobalObject, bindingsAliveScope.value(), scope, db, sql.stmt, bindings, safeIntegers, nullptr);
JSC::JSValue reb = rebindStatement(lexicalGlobalObject, bindingsAliveScope.value(), scope, db, versionDB, sql.stmt, bindings, safeIntegers, nullptr);
// close() during binding already finalized sql.stmt via
// sqlite3_next_stmt(); keep the auto-destructor from
// finalizing it again.
Comment thread
robobun marked this conversation as resolved.
Outdated
if (versionDB->finalizedStatementsOnClose) [[unlikely]]
sql.stmt = nullptr;
RETURN_IF_EXCEPTION(scope, {});

if (versionDB->db != db) [[unlikely]] {
Expand Down Expand Up @@ -1841,11 +1870,23 @@
return JSValue::encode(jsUndefined());
}

// sqlite3_close_v2 is used for automatic GC cleanup
int statusCode = shouldThrowOnError ? sqlite3_close(db) : sqlite3_close_v2(db);
// Finalize every outstanding prepared statement on this connection so
// sqlite3_close() cannot fail with SQLITE_BUSY. Statement wrappers detect
// this through version_db->finalizedStatementsOnClose and never touch
// their now-freed sqlite3_stmt again.
Comment thread
robobun marked this conversation as resolved.
Outdated
while (sqlite3_stmt* stmt = sqlite3_next_stmt(db, nullptr)) {
sqlite3_finalize(stmt);
}
versionDB->finalizedStatementsOnClose = true;

Check warning on line 1880 in src/jsc/bindings/sqlite/JSSQLStatement.cpp

View check run for this annotation

Claude / Claude Code Review

#13082 test becomes vacuous under new close() semantics (rejections swallowed by allSettled)

The existing `#13082` test (test/js/bun/sqlite/sqlite.test.js) becomes vacuous under the new close() semantics: `stmt.all()/get()/run()` now throw "Database has closed" at `CHECK_PREPARED`, and the 100 rejections are silently swallowed by `Promise.allSettled` with no assertion on the settled values. Since the original UAF-under-GC path is now unreachable by construction, consider updating the test to assert the new throw (or removing it with a stated reason) rather than leaving it green while co
Comment thread
robobun marked this conversation as resolved.
Outdated

int statusCode = sqlite3_close(db);
if (statusCode != SQLITE_OK) {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, WTF::String::fromUTF8(sqlite3_errstr(statusCode))));
return {};
if (shouldThrowOnError) {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, WTF::String::fromUTF8(sqlite3_errstr(statusCode))));
return {};
}
// Non-strict close() never throws; defer the close instead.
sqlite3_close_v2(db);

Check warning on line 1889 in src/jsc/bindings/sqlite/JSSQLStatement.cpp

View check run for this annotation

Claude / Claude Code Review

close(true) throw path leaves finalizedStatementsOnClose=true with db handle still non-null

nit: on the `close(true)` throw path, `versionDB->db` stays non-null (and `sqlite3_close_v2` is skipped) even though `finalizedStatementsOnClose` was already set — so a caught error leaves the `VersionSqlite3` in `{db != nullptr, finalizedStatementsOnClose = true}`, where a subsequent `prepare()` creates a stmt whose wrapper is dead-on-arrival and whose destructor skips `sqlite3_finalize`. After the `sqlite3_next_stmt` loop this can only trigger over an extension-created `sqlite3_blob`/`sqlite3_
Comment thread
claude[bot] marked this conversation as resolved.
}

versionDB->db = nullptr;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Expand Down Expand Up @@ -2254,6 +2295,10 @@
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
return {};
}
if (castedThis->version_db->finalizedStatementsOnClose) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
}
status = sqlite3_step(stmt);
} while (status == SQLITE_ROW);
} else {
Expand All @@ -2266,6 +2311,10 @@
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
return {};
}
if (castedThis->version_db->finalizedStatementsOnClose) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
}
status = sqlite3_step(stmt);
} while (status == SQLITE_ROW);
}
Expand Down Expand Up @@ -2412,6 +2461,10 @@
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
return {};
}
if (castedThis->version_db->finalizedStatementsOnClose) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
}
status = sqlite3_step(stmt);
} while (status == SQLITE_ROW);
}
Expand Down Expand Up @@ -2501,9 +2554,13 @@
}

if (castedThis->stmt != stmt) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
return {};
}
if (castedThis->version_db->finalizedStatementsOnClose) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
}

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

View check run for this annotation

Claude / Claude Code Review

raw() exception arm calls sqlite3_reset on a stmt freed by close() during push()

The exception arm after `resultArray->push()` calls `sqlite3_reset(stmt)` *before* either liveness check, so an `Array.prototype[0]` setter that calls `db.close()` (which now `sqlite3_finalize`s every statement via `sqlite3_next_stmt`) and then throws hits `sqlite3_reset` on freed memory. The sibling `all()`/`values()` loops use a bare `RETURN_IF_EXCEPTION(scope, {})` after `push()` with no reset — matching them here (or moving both guards above the reset) fixes it.
Comment thread
robobun marked this conversation as resolved.
Outdated

status = sqlite3_step(stmt);
} while (status == SQLITE_ROW);
Expand Down Expand Up @@ -2607,6 +2664,12 @@

CHECK_THIS

// After close() the stmt pointer is dangling; report the same empty
// string a finalized statement produces.
Comment thread
robobun marked this conversation as resolved.
Outdated
if (castedThis->version_db && castedThis->version_db->finalizedStatementsOnClose) [[unlikely]] {
RELEASE_AND_RETURN(scope, JSValue::encode(jsEmptyString(vm)));
}

char* string = sqlite3_expanded_sql(castedThis->stmt);
if (!string) {
RELEASE_AND_RETURN(scope, JSValue::encode(jsEmptyString(vm)));
Expand Down Expand Up @@ -2823,7 +2886,9 @@
CHECK_THIS

if (castedThis->stmt) {
sqlite3_finalize(castedThis->stmt);
if (!(castedThis->version_db && castedThis->version_db->finalizedStatementsOnClose)) {
sqlite3_finalize(castedThis->stmt);
}
castedThis->stmt = nullptr;
}

Expand All @@ -2844,7 +2909,7 @@

JSSQLStatement::~JSSQLStatement()
{
if (this->stmt) {
if (this->stmt && !(this->version_db && this->version_db->finalizedStatementsOnClose)) {
sqlite3_finalize(this->stmt);
}

Expand Down Expand Up @@ -2873,7 +2938,7 @@
auto scope = DECLARE_THROW_SCOPE(vm);
auto* stmt = this->stmt;

auto val = rebindStatement(lexicalGlobalObject, values, scope, this->version_db->db, stmt, this->m_bindingNames, this->useBigInt64, this);
auto val = rebindStatement(lexicalGlobalObject, values, scope, this->version_db->db, this->version_db, stmt, this->m_bindingNames, this->useBigInt64, this);
RETURN_IF_EXCEPTION(scope, {});

// A getter invoked while binding can finalize this statement; the callers
Expand All @@ -2883,6 +2948,13 @@
return {};
}

// A getter can also close the database, which finalizes `stmt` without
// nulling this wrapper's pointer.
Comment thread
robobun marked this conversation as resolved.
Outdated
if (this->version_db->finalizedStatementsOnClose) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
}

if (val.isNumber()) {
RELEASE_AND_RETURN(scope, val);
} else {
Expand Down
Loading
Loading