sqlite: keep sessions alive across SQLite callbacks

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

@@ -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. 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.
@@ -508,9 +515,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(); }
@@ -519,6 +530,7 @@ class CallbackDepthGuard {
 
  private:
   DatabaseSync* db_;
+  std::vector<BaseObjectPtr<Session>> pinned_sessions_;
 };
 
 class TraceEventSuppressionGuard {

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;