From 726169cb77a049bd5e869236c237be21b11e3764 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Fri, 21 Aug 2026 13:13:03 -0400 Subject: [PATCH 1/2] sqlite: keep sessions alive across SQLite callbacks Session objects are weak and nothing else holds a strong reference to them, so a garbage collection can free one while SQLite is still using it. SQLite runs "PRAGMA table_xinfo" from inside its pre-update hook, which reaches JavaScript, so a GC during a callback invoked from there can collect a session the hook is still walking. Hold a strong reference to every attached session for the duration of each callback SQLite invokes. The trace callback now enters that guard before building its payload, since the allocation can itself trigger a garbage collection. Fixes: https://github.com/nodejs/node/issues/65460 Signed-off-by: Trevor Burnham --- src/node_sqlite.cc | 14 ++++++++- src/node_sqlite.h | 12 ++++++++ test/parallel/test-sqlite-session.js | 44 ++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index af211ab5fc0..4640adaa1fb 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1024,6 +1024,15 @@ void DatabaseSync::RemoveBackup(BackupJob* job) { backups_.erase(job); } +void DatabaseSync::PinSessions( + std::vector>* pinned) const { + if (sessions_.empty()) return; + pinned->reserve(sessions_.size()); + for (Session* session : sessions_) { + pinned->emplace_back(session); + } +} + void DatabaseSync::DeleteSessions() { // all attached sessions need to be deleted before the database is closed // https://www.sqlite.org/session/sqlite3session_create.html @@ -2832,6 +2841,10 @@ int DatabaseSync::TraceCallback(unsigned int type, return 0; } + // Entered before building the payload below, because allocating it can + // trigger a garbage collection that SQLite is not prepared for. + CallbackDepthGuard guard(db); + Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); @@ -2870,7 +2883,6 @@ int DatabaseSync::TraceCallback(unsigned int type, Local payload = Object::New(isolate, Null(isolate), keys, values, 3); - CallbackDepthGuard guard(db); ch->Publish(env, payload); return 0; diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 467aeebb33e..23fe498fab5 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -292,6 +292,13 @@ class DatabaseSync : public BaseObject { void DecrementCallbackDepth() { --callback_depth_; } bool IsInCallback() const { return callback_depth_ > 0; } + // SQLite reaches back into JavaScript from inside its pre-update hook, while + // it is still walking this connection's session list. Session objects are + // weak, so a garbage collection during such a callback could collect one and + // free memory SQLite is still using. Appends a strong reference to every + // attached session so that a callback can hold them for its duration. + void PinSessions(std::vector>* pinned) const; + // SQLite forbids an authorizer callback from doing anything that modifies // the database connection that invoked it, which includes preparing and // stepping statements. See https://www.sqlite.org/c3ref/set_authorizer.html. @@ -508,10 +515,14 @@ class SQLTagStore : public BaseObject { friend class StatementExecutionHelper; }; +// Guards a window in which SQLite hands control back to JavaScript. Construct +// it before allocating anything on the V8 heap, since the pinned sessions +// below are what keep a garbage collection during that window safe. class CallbackDepthGuard { public: explicit CallbackDepthGuard(DatabaseSync* db) : db_(db) { db_->IncrementCallbackDepth(); + db_->PinSessions(&pinned_sessions_); } ~CallbackDepthGuard() { db_->DecrementCallbackDepth(); } CallbackDepthGuard(const CallbackDepthGuard&) = delete; @@ -519,6 +530,7 @@ class CallbackDepthGuard { private: DatabaseSync* db_; + std::vector> pinned_sessions_; }; class TraceEventSuppressionGuard { diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index a8bbaa77d06..3c5ca56e481 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -664,6 +664,50 @@ test('session - keeps its database alive after the db handle is dropped', async session.close(); }); +// SQLite runs "PRAGMA table_xinfo" from inside its pre-update hook, while it is +// still walking the connection's session list. Session objects are weak, so a +// GC during a callback that the PRAGMA triggers could collect a session that +// JavaScript no longer references and free memory the walk is still using. +test('session - survives GC during an authorizer callback', (t) => { + const database = new DatabaseSync(':memory:'); + database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)'); + database.createSession(); // Never referenced again, so it is collectable. + + let ran = false; + database.setAuthorizer((actionCode, param1) => { + if (actionCode === constants.SQLITE_PRAGMA && param1 === 'table_xinfo') { + ran = true; + globalThis.gc(); + globalThis.gc(); + } + return constants.SQLITE_OK; + }); + + database.exec('INSERT INTO data VALUES (1)'); + t.assert.ok(ran, 'the authorizer callback never ran'); +}); + +test("session - survives GC during a 'sqlite.db.query' subscriber", (t) => { + const dc = require('node:diagnostics_channel'); + const database = new DatabaseSync(':memory:'); + database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)'); + database.createSession(); // Never referenced again, so it is collectable. + + let ran = false; + const handler = ({ sql }) => { + if (sql.includes('table_xinfo')) { + ran = true; + globalThis.gc(); + globalThis.gc(); + } + }; + dc.subscribe('sqlite.db.query', handler); + t.after(() => dc.unsubscribe('sqlite.db.query', handler)); + + database.exec('INSERT INTO data VALUES (1)'); + t.assert.ok(ran, 'the subscriber never ran'); +}); + test('session supports ERM', (t) => { const database = new DatabaseSync(':memory:'); let afterDisposeSession; From 7adc57293023778cafba8544a0c7f26bfd8618d2 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Fri, 21 Aug 2026 15:51:51 -0400 Subject: [PATCH 2/2] fixup! sqlite: keep sessions alive across SQLite callbacks Return the pinned sessions by value instead of appending to a caller's vector, which read as an append but reserved as if the target were empty. Signed-off-by: Trevor Burnham --- src/node_sqlite.cc | 10 +++++----- src/node_sqlite.h | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 4640adaa1fb..738b2d88e8a 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1024,13 +1024,13 @@ void DatabaseSync::RemoveBackup(BackupJob* job) { backups_.erase(job); } -void DatabaseSync::PinSessions( - std::vector>* pinned) const { - if (sessions_.empty()) return; - pinned->reserve(sessions_.size()); +std::vector> DatabaseSync::PinSessions() const { + std::vector> pinned; + pinned.reserve(sessions_.size()); for (Session* session : sessions_) { - pinned->emplace_back(session); + pinned.emplace_back(session); } + return pinned; } void DatabaseSync::DeleteSessions() { diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 23fe498fab5..aae0b2c6742 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -295,9 +295,9 @@ class DatabaseSync : public BaseObject { // SQLite reaches back into JavaScript from inside its pre-update hook, while // it is still walking this connection's session list. Session objects are // weak, so a garbage collection during such a callback could collect one and - // free memory SQLite is still using. Appends a strong reference to every + // free memory SQLite is still using. Returns a strong reference to every // attached session so that a callback can hold them for its duration. - void PinSessions(std::vector>* pinned) const; + std::vector> PinSessions() const; // SQLite forbids an authorizer callback from doing anything that modifies // the database connection that invoked it, which includes preparing and @@ -520,9 +520,9 @@ class SQLTagStore : public BaseObject { // below are what keep a garbage collection during that window safe. class CallbackDepthGuard { public: - explicit CallbackDepthGuard(DatabaseSync* db) : db_(db) { + explicit CallbackDepthGuard(DatabaseSync* db) + : db_(db), pinned_sessions_(db->PinSessions()) { db_->IncrementCallbackDepth(); - db_->PinSessions(&pinned_sessions_); } ~CallbackDepthGuard() { db_->DecrementCallbackDepth(); } CallbackDepthGuard(const CallbackDepthGuard&) = delete;