sqlite: reject closing a session from a callback

SQLite runs "PRAGMA table_xinfo" from inside its pre-update hook, while
it is still walking the connection's session list. Deleting a session
from a callback that the PRAGMA triggers frees memory that walk is still
using. Both an authorizer callback and a 'sqlite.db.query' subscriber
reach that window, and either one segfaults.

Reject session.close() and Symbol.dispose when the connection is inside
any callback. An already-closed session stays a no-op so that disposal
remains idempotent.

Fixes: #65428
Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
PR-URL: #65454
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Author: TrevorBurnham

doc/api/sqlite.md

@@ -1014,8 +1014,11 @@ wrapper around [`sqlite3session_patchset()`][].
 ### `session.close()`
 
 Closes the session. An exception is thrown if the database or the session is not open,
-or if the session is currently generating a changeset or patchset. This method is a
-wrapper around [`sqlite3session_delete()`][].
+or if the session is currently generating a changeset or patchset. An
+[`ERR_INVALID_STATE`][] error is thrown if the method is called from a callback that
+SQLite invoked, such as an authorizer callback, a user-defined function, or a
+[`'sqlite.db.query'`][] subscriber, because SQLite may still be using the session.
+This method is a wrapper around [`sqlite3session_delete()`][].
 
 ### `session[Symbol.dispose]()`
 
@@ -1025,7 +1028,8 @@ added: v24.9.0
 
 Closes the session. If the session is already closed, then this is a no-op. An
 [`ERR_INVALID_STATE`][] error is thrown if the session is currently generating
-a changeset or patchset, under the same conditions as [`session.close()`][].
+a changeset or patchset, or if the method is called from a callback that SQLite
+invoked, under the same conditions as [`session.close()`][].
 
 ## Class: `StatementSync`
 

src/node_sqlite.cc

@@ -169,6 +169,16 @@ inline MaybeLocal<Value> IntegerToValue(Isolate* isolate,
           sqlite3_stmt_busy((stmt)->statement_.get()),                         \
       "database cannot be accessed from an authorizer callback")
 
+// SQLite's session module reaches back into JavaScript from inside the
+// pre-update hook, while it is still walking the connection's session list and
+// reading the table it found there. Deleting a session frees memory that walk
+// is still using, so no callback may close one.
+#define THROW_AND_RETURN_IF_SESSION_IN_CALLBACK(env, session)                  \
+  THROW_AND_RETURN_ON_BAD_STATE(                                               \
+      (env),                                                                   \
+      (session)->database_->IsInCallback(),                                    \
+      "session cannot be closed while in a 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
@@ -4358,6 +4368,9 @@ void Session::Close(const FunctionCallbackInfo<Value>& args) {
       env, session->session_ == nullptr, "session is not open");
   THROW_AND_RETURN_ON_BAD_STATE(
       env, session->is_generating_changeset_, "session is currently in use");
+  // Checked last: changeset generation runs the authorizer, so both conditions
+  // hold in that case and the more specific message above has to win.
+  THROW_AND_RETURN_IF_SESSION_IN_CALLBACK(env, session);
 
   session->Delete();
 }
@@ -4371,6 +4384,7 @@ void Session::Dispose(const FunctionCallbackInfo<Value>& args) {
   }
   THROW_AND_RETURN_ON_BAD_STATE(
       env, session->is_generating_changeset_, "session is currently in use");
+  THROW_AND_RETURN_IF_SESSION_IN_CALLBACK(env, session);
 
   session->Delete();
 }

test/parallel/test-sqlite-session.js

@@ -6,7 +6,8 @@ const {
   DatabaseSync,
   constants,
 } = require('node:sqlite');
-const { test, suite } = require('node:test');
+const { it, test, suite } = require('node:test');
+const dc = require('node:diagnostics_channel');
 const { nextDb } = require('../sqlite/next-db.js');
 const { Worker } = require('worker_threads');
 const { once } = require('events');
@@ -652,6 +653,157 @@ test('session[Symbol.dispose]() - after closing database is a no-op', () => {
   session[Symbol.dispose]();
 });
 
+// SQLite runs "PRAGMA table_xinfo" from inside its pre-update hook, while it is
+// still walking the connection's session list. Deleting a session from a
+// callback that PRAGMA triggers frees memory the walk is still using, so the
+// close has to be rejected instead.
+suite('session.close() - from a callback', () => {
+  const expectedError =
+    'ERR_INVALID_STATE: session cannot be closed while in a callback';
+
+  for (const method of ['close', 'dispose']) {
+    const closeSession = (session) => {
+      if (method === 'close') {
+        session.close();
+      } else {
+        session[Symbol.dispose]();
+      }
+    };
+
+    it(`rejects ${method} from an authorizer callback`, (t) => {
+      const database = new DatabaseSync(':memory:');
+      database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)');
+      const session = database.createSession();
+      let outcome = 'callback did not run';
+
+      database.setAuthorizer((actionCode, param1) => {
+        if (actionCode === constants.SQLITE_PRAGMA && param1 === 'table_xinfo') {
+          try {
+            closeSession(session);
+            outcome = 'did not throw';
+          } catch (err) {
+            outcome = `${err.code}: ${err.message}`;
+          }
+        }
+        return constants.SQLITE_OK;
+      });
+
+      database.exec('INSERT INTO data VALUES (1)');
+      t.assert.strictEqual(outcome, expectedError);
+
+      // The session survived and kept recording the insert.
+      database.setAuthorizer(null);
+      t.assert.notStrictEqual(session.changeset().length, 0);
+      session.close();
+    });
+
+    it(`rejects ${method} from a 'sqlite.db.query' subscriber`, (t) => {
+      const database = new DatabaseSync(':memory:');
+      database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)');
+      const session = database.createSession();
+      let outcome = 'callback did not run';
+
+      const handler = ({ sql }) => {
+        if (sql.includes('table_xinfo')) {
+          try {
+            closeSession(session);
+            outcome = 'did not throw';
+          } catch (err) {
+            outcome = `${err.code}: ${err.message}`;
+          }
+        }
+      };
+      dc.subscribe('sqlite.db.query', handler);
+      t.after(() => dc.unsubscribe('sqlite.db.query', handler));
+
+      database.exec('INSERT INTO data VALUES (1)');
+      t.assert.strictEqual(outcome, expectedError);
+
+      dc.unsubscribe('sqlite.db.query', handler);
+      t.assert.notStrictEqual(session.changeset().length, 0);
+      session.close();
+    });
+
+    // Deliberately broader than the crash: the pre-update hook is not on the
+    // stack here, so this close is safe today. Node cannot tell whether SQLite
+    // is inside that hook, so every callback is rejected. This pins the
+    // trade-off rather than leaving it to be discovered as a regression.
+    it(`rejects ${method} from a user-defined function`, (t) => {
+      const database = new DatabaseSync(':memory:');
+      database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)');
+      const session = database.createSession();
+      let outcome = 'callback did not run';
+
+      database.function('f', (x) => {
+        try {
+          closeSession(session);
+          outcome = 'did not throw';
+        } catch (err) {
+          outcome = `${err.code}: ${err.message}`;
+        }
+        return x;
+      });
+
+      database.exec('SELECT f(1)');
+      t.assert.strictEqual(outcome, expectedError);
+
+      // Still closable once the callback is off the stack.
+      session.close();
+      t.assert.throws(() => session.close(), { message: 'session is not open' });
+    });
+  }
+
+  // Rejecting disposal has a cost: a `using` declaration inside a callback
+  // demotes the block's own error to SuppressedError. Accepted for symmetry
+  // with StatementSync's disposal, which throws for a busy statement the same
+  // way. Pinned here so the trade-off is visible rather than surprising.
+  it('demotes a callback error when disposal is rejected', (t) => {
+    const database = new DatabaseSync(':memory:');
+    database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)');
+    let caught;
+
+    database.function('f', (x) => {
+      try {
+        using session = database.createSession();
+        t.assert.ok(session);
+        throw new Error('callback error');
+      } catch (err) {
+        caught = err;
+      }
+      return x;
+    });
+
+    database.exec('SELECT f(1)');
+    t.assert.ok(caught instanceof SuppressedError);
+    t.assert.strictEqual(caught.suppressed.message, 'callback error');
+    t.assert.strictEqual(
+      caught.error.message,
+      'session cannot be closed while in a callback',
+    );
+  });
+
+  it('leaves an already closed session disposable from a callback', (t) => {
+    const database = new DatabaseSync(':memory:');
+    database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)');
+    const session = database.createSession();
+    session.close();
+    let outcome = 'callback did not run';
+
+    database.setAuthorizer(() => {
+      try {
+        session[Symbol.dispose]();
+        outcome = 'no-op';
+      } catch (err) {
+        outcome = `${err.code}: ${err.message}`;
+      }
+      return constants.SQLITE_OK;
+    });
+
+    database.exec('INSERT INTO data VALUES (1)');
+    t.assert.strictEqual(outcome, 'no-op');
+  });
+});
+
 test('session - keeps its database alive after the db handle is dropped', async (t) => {
   const { gcUntil, onGC } = require('../common/gc');