From 25f57eca3353b2090efcbea5671a515193727c6b Mon Sep 17 00:00:00 2001 From: "Kamat, Trivikram" <16024985+trivikr@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:47:59 -0700 Subject: [PATCH] sqlite: reject busy statement finalization in authorizer An iterator can leave a statement active between sqlite3_step() calls. Finalizing that statement from an authorizer callback can release its locks and change the outcome of the statement being authorized. Reject close() and Symbol.dispose when the statement is busy and the connection is in an authorizer callback. Continue allowing idle statements to be finalized. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: codex:gpt-5.6-sol --- doc/api/sqlite.md | 2 +- src/node_sqlite.cc | 11 ++++++++ src/node_sqlite.h | 5 ++-- test/parallel/test-sqlite-authz.js | 43 +++++++++++++++++++++++++++--- 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 95d1143a2427..6686ea8ac79f 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -1127,7 +1127,7 @@ Finalizes the prepared statement. An exception is thrown if the statement is already finalized. An [`ERR_INVALID_STATE`][] error is thrown if this statement is currently executing, which happens when the method is called from a callback that the statement itself triggered, such as a user-defined function, an -aggregate function, or a [`'sqlite.db.query'`][] subscriber. Other statements +aggregate function, or a [`'sqlite.db.query'`][] subscriber. Idle statements on the same connection can be finalized from such a callback. This method is a wrapper around [`sqlite3_finalize()`][]. diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index d0da887062bd..fd34c06f0077 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -160,6 +160,15 @@ inline MaybeLocal IntegerToValue(Isolate* isolate, (db)->IsInAuthorizerCallback(), \ "database cannot be accessed from an authorizer callback") +// Finalizing a busy statement from an authorizer can release its locks and +// change the outer statement's outcome. +#define THROW_AND_RETURN_IF_BUSY_IN_AUTHORIZER(env, stmt) \ + THROW_AND_RETURN_ON_BAD_STATE( \ + (env), \ + (stmt)->db_->IsInAuthorizerCallback() && \ + sqlite3_stmt_busy((stmt)->statement_.get()), \ + "database cannot be accessed from an authorizer callback") + // A statement's virtual machine cannot be reentered while sqlite3_step() is // running it. Finalizing it frees the VM outright, and re-running it resets the // VM mid-execution; both are use-after-free rather than merely a contract @@ -2900,6 +2909,7 @@ void StatementSync::Close(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); THROW_AND_RETURN_IF_STEPPING(env, stmt); + THROW_AND_RETURN_IF_BUSY_IN_AUTHORIZER(env, stmt); stmt->Close(); } @@ -2913,6 +2923,7 @@ void StatementSync::Dispose(const FunctionCallbackInfo& args) { return; } THROW_AND_RETURN_IF_STEPPING(env, stmt); + THROW_AND_RETURN_IF_BUSY_IN_AUTHORIZER(env, stmt); stmt->Close(); } diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 475a759e75e8..bf2f765abddb 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -307,9 +307,8 @@ class DatabaseSync : public BaseObject { void DecrementAuthorizerDepth() { --authorizer_depth_; } bool IsInAuthorizerCallback() const { return authorizer_depth_ > 0; } - // Finalizing a statement frees its virtual machine, so a callback that - // SQLite invokes from inside sqlite3_step() must not finalize the statement - // being stepped. Other statements on the connection are safe to finalize. + // A callback must not finalize the statement being stepped. Other statements + // are safe unless they are busy during an authorizer callback. void PushSteppingStatement(sqlite3_stmt* stmt) { stepping_statements_.push_back(stmt); } diff --git a/test/parallel/test-sqlite-authz.js b/test/parallel/test-sqlite-authz.js index f6020ce9047e..eda923e258b9 100644 --- a/test/parallel/test-sqlite-authz.js +++ b/test/parallel/test-sqlite-authz.js @@ -397,10 +397,8 @@ suite('authorizer callback reentrancy', () => { assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); }); - // Only the statement being stepped is unsafe to finalize. Other statements - // on the connection have their own virtual machines, so finalizing them from - // a callback is allowed. - it('allows finalizing a statement that is not being executed', () => { + // An idle statement has no virtual-machine state or locks to release. + it('allows finalizing an idle statement', () => { const db = new DatabaseSync(':memory:'); db.exec('CREATE TABLE t (x INTEGER)'); db.exec('INSERT INTO t VALUES (1)'); @@ -417,6 +415,43 @@ suite('authorizer callback reentrancy', () => { }); }); + // A paused iterator is busy and may hold locks between sqlite3_step() calls. + it('rejects finalizing another active statement', () => { + for (const method of ['close', 'dispose']) { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1), (2), (3)'); + const stmt = db.prepare('SELECT x FROM t'); + const iter = stmt.iterate(); + iter.next(); + let outcome = 'authorizer callback did not run'; + + db.setAuthorizer((actionCode) => { + if (actionCode === constants.SQLITE_DROP_TABLE) { + try { + if (method === 'close') { + stmt.close(); + } else { + stmt[Symbol.dispose](); + } + outcome = 'did not throw'; + } catch (err) { + outcome = `${err.code}: ${err.message}`; + } + } + return constants.SQLITE_OK; + }); + + assert.throws(() => db.exec('DROP TABLE t'), { + code: 'ERR_SQLITE_ERROR', + message: 'database table is locked', + }); + assert.strictEqual(outcome, expectedError); + db.setAuthorizer(null); + iter.return(); + } + }); + // Disposal is idempotent, so a statement that is already finalized must stay // a no-op even inside a callback. Throwing here would turn a `using` scope's // real exception into a SuppressedError.