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: #65460
Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
PR-URL: #65465
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Author: TrevorBurnham

src/node_sqlite.cc

@@ -1024,6 +1024,15 @@ void DatabaseSync::RemoveBackup(BackupJob* job) {
   backups_.erase(job);
 }
 
+std::vector<BaseObjectPtr<Session>> DatabaseSync::PinSessions() const {
+  std::vector<BaseObjectPtr<Session>> pinned;
+  pinned.reserve(sessions_.size());
+  for (Session* session : sessions_) {
+    pinned.emplace_back(session);
+  }
+  return pinned;
+}
+
 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<Object> payload = Object::New(isolate, Null(isolate), keys, values, 3);
 
-  CallbackDepthGuard guard(db);
   ch->Publish(env, payload);
 
   return 0;

src/node_sqlite.h

@@ -289,6 +289,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. Returns a strong reference to every
+  // attached session so that a callback can hold them for its duration.
+  std::vector<BaseObjectPtr<Session>> PinSessions() 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.
@@ -505,9 +512,13 @@ 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) {
+  explicit CallbackDepthGuard(DatabaseSync* db)
+      : db_(db), pinned_sessions_(db->PinSessions()) {
     db_->IncrementCallbackDepth();
   }
   ~CallbackDepthGuard() { db_->DecrementCallbackDepth(); }
@@ -516,6 +527,7 @@ class CallbackDepthGuard {
 
  private:
   DatabaseSync* db_;
+  std::vector<BaseObjectPtr<Session>> pinned_sessions_;
 };
 
 class TraceEventSuppressionGuard {

test/parallel/test-sqlite-session.js

@@ -682,6 +682,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;