From baff673fac8669c158f2cad43c9808579df2e568 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Fri, 21 Aug 2026 08:47:31 -0400 Subject: [PATCH 1/3] 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: https://github.com/nodejs/node/issues/65428 Signed-off-by: Trevor Burnham --- doc/api/sqlite.md | 10 ++- src/node_sqlite.cc | 12 ++++ test/parallel/test-sqlite-session.js | 97 +++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 4 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 3cf7f99c2eb5..8a875035f8cb 100644 --- a/doc/api/sqlite.md +++ b/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` diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index f8ab3ca51570..2fe697f907bc 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -169,6 +169,16 @@ inline MaybeLocal 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 @@ -4346,6 +4356,7 @@ void Session::Close(const FunctionCallbackInfo& 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"); + THROW_AND_RETURN_IF_SESSION_IN_CALLBACK(env, session); session->Delete(); } @@ -4359,6 +4370,7 @@ void Session::Dispose(const FunctionCallbackInfo& 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(); } diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index e72e056d869f..d3fc24833ad2 100644 --- a/test/parallel/test-sqlite-session.js +++ b/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,100 @@ 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(); + }); + } + + 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'); From eac9abce00d1aadcdb8bb9a0227f2018da7d15ee Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Fri, 21 Aug 2026 16:09:55 -0400 Subject: [PATCH 2/3] fixup! sqlite: reject closing a session from a callback Pin the deliberate over-rejection with a test: closing from a user-defined function is safe today but rejected anyway, because Node cannot tell whether SQLite is inside its pre-update hook. Signed-off-by: Trevor Burnham --- test/parallel/test-sqlite-session.js | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index d3fc24833ad2..551b0a8d6096 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -723,6 +723,34 @@ suite('session.close() - from a callback', () => { 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' }); + }); } it('leaves an already closed session disposable from a callback', (t) => { From 2e65c4c231bdb2e241107a6901240bf7e525334c Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Fri, 21 Aug 2026 16:29:37 -0400 Subject: [PATCH 3/3] fixup! sqlite: reject closing a session from a callback Cover the `using` form of the rejected disposal, whose block error is demoted to SuppressedError, and note why the callback check has to stay below the changeset check in Session::Close(). Signed-off-by: Trevor Burnham --- src/node_sqlite.cc | 2 ++ test/parallel/test-sqlite-session.js | 29 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 2fe697f907bc..f6d27008489b 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -4356,6 +4356,8 @@ void Session::Close(const FunctionCallbackInfo& 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(); diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index 551b0a8d6096..5fe0eada26c7 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -753,6 +753,35 @@ suite('session.close() - from a callback', () => { }); } + // 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)');