diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 7de0f2d88ce..ab09d310964 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 c6b8a8d0a61..8558b91cc15 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 7fec61dbf9a..ef7105f6324 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 f6020ce9047..eda923e258b 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.