sqlite: fix crash when a session outlives its database

Keep the database alive while a session is open so that closing the
database before the session does not leave the session pointing at
freed memory.

Signed-off-by: Mohamed Sayed <k@3zrv.com>
PR-URL: #63797
Fixes: #63796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>

Author: 3zrv

src/node_sqlite.cc

@@ -2124,7 +2124,7 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo<Value>& args) {
   CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
 
   BaseObjectPtr<Session> session =
-      Session::Create(env, BaseObjectWeakPtr<DatabaseSync>(db), pSession);
+      Session::Create(env, BaseObjectPtr<DatabaseSync>(db), pSession);
   args.GetReturnValue().Set(session->object());
 }
 
@@ -3803,7 +3803,7 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
 
 Session::Session(Environment* env,
                  Local<Object> object,
-                 BaseObjectWeakPtr<DatabaseSync> database,
+                 BaseObjectPtr<DatabaseSync> database,
                  sqlite3_session* session)
     : BaseObject(env, object),
       session_(session),
@@ -3816,7 +3816,7 @@ Session::~Session() {
 }
 
 BaseObjectPtr<Session> Session::Create(Environment* env,
-                                       BaseObjectWeakPtr<DatabaseSync> database,
+                                       BaseObjectPtr<DatabaseSync> database,
                                        sqlite3_session* session) {
   Local<Object> obj;
   if (!GetConstructorTemplate(env)
@@ -3850,7 +3850,9 @@ Local<FunctionTemplate> Session::GetConstructorTemplate(Environment* env) {
   return tmpl;
 }
 
-void Session::MemoryInfo(MemoryTracker* tracker) const {}
+void Session::MemoryInfo(MemoryTracker* tracker) const {
+  tracker->TrackField("database", database_);
+}
 
 template <Sqlite3ChangesetGenFunc sqliteChangesetFunc>
 void Session::Changeset(const FunctionCallbackInfo<Value>& args) {

src/node_sqlite.h

@@ -339,7 +339,7 @@ class Session : public BaseObject {
  public:
   Session(Environment* env,
           v8::Local<v8::Object> object,
-          BaseObjectWeakPtr<DatabaseSync> database,
+          BaseObjectPtr<DatabaseSync> database,
           sqlite3_session* session);
   ~Session() override;
   template <Sqlite3ChangesetGenFunc sqliteChangesetFunc>
@@ -349,7 +349,7 @@ class Session : public BaseObject {
   static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
       Environment* env);
   static BaseObjectPtr<Session> Create(Environment* env,
-                                       BaseObjectWeakPtr<DatabaseSync> database,
+                                       BaseObjectPtr<DatabaseSync> database,
                                        sqlite3_session* session);
 
   void MemoryInfo(MemoryTracker* tracker) const override;
@@ -359,7 +359,7 @@ class Session : public BaseObject {
  private:
   void Delete();
   sqlite3_session* session_;
-  BaseObjectWeakPtr<DatabaseSync> database_;  // The Parent Database
+  BaseObjectPtr<DatabaseSync> database_;  // The Parent Database
 };
 
 class SQLTagStore : public BaseObject {

test/parallel/test-sqlite-session.js

@@ -1,4 +1,4 @@
-// Flags: --experimental-sqlite
+// Flags: --expose-gc --experimental-sqlite
 'use strict';
 const { skipIfSQLiteMissing } = require('../common');
 skipIfSQLiteMissing();
@@ -589,6 +589,36 @@ test('session.close() - closing twice', (t) => {
   });
 });
 
+test('session - keeps its database alive after the db handle is dropped', async (t) => {
+  const { gcUntil, onGC } = require('../common/gc');
+
+  // The DatabaseSync handle is created in a nested scope and never referenced
+  // again, so the returned session is the only thing keeping it reachable.
+  let dbCollected = false;
+  const session = (() => {
+    const database = new DatabaseSync(':memory:');
+    database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY, value TEXT)');
+    onGC(database, { ongc: () => { dbCollected = true; } });
+    const s = database.createSession();
+    database.exec("INSERT INTO data VALUES (1, 'hello')");
+    return s;
+  })();
+
+  // The session must keep the database alive across GC. Previously it held
+  // only a weak reference, so the database could be collected and using the
+  // session afterwards dereferenced a dangling pointer and crashed.
+  await gcUntil('database is collected', () => dbCollected, 5).then(
+    () => { throw new Error('session did not keep its database alive'); },
+    () => {}, // Expected: the database is never collected, so gcUntil rejects.
+  );
+  t.assert.strictEqual(dbCollected, false);
+
+  // The database is still open and usable through the still-alive session.
+  const changeset = session.changeset();
+  t.assert.ok(changeset.byteLength > 0);
+  session.close();
+});
+
 test('session supports ERM', (t) => {
   const database = new DatabaseSync(':memory:');
   let afterDisposeSession;