From 676103dd89ea4ce884dd438fe94f3504cf873a1a 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 7de0f2d88cea..ab09d3109648 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -1128,7 +1128,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 c6b8a8d0a611..8558b91cc15a 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 7fec61dbf9a4..ef7105f63242 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -299,9 +299,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.