sqlite: reject busy statement finalization in authorizer

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()`][].
 

src/node_sqlite.cc

@@ -160,6 +160,15 @@ inline MaybeLocal<Value> 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<Value>& 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<Value>& args) {
     return;
   }
   THROW_AND_RETURN_IF_STEPPING(env, stmt);
+  THROW_AND_RETURN_IF_BUSY_IN_AUTHORIZER(env, stmt);
   stmt->Close();
 }
 

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);
   }

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.