From bfa602068396a63d2a7790f7c2444fff0b518305 Mon Sep 17 00:00:00 2001 From: Mohamed Sayed Date: Mon, 8 Jun 2026 19:21:31 +0300 Subject: [PATCH 1/2] sqlite: fix crash when a session outlives its database Signed-off-by: Mohamed Sayed --- src/node_sqlite.cc | 8 ++++++-- test/parallel/test-sqlite-session.js | 30 +++++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 87cd52d7283f71..233b2db4346713 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -3825,7 +3825,9 @@ void Session::Changeset(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( - env, !session->database_->IsOpen(), "database is not open"); + env, + !session->database_ || !session->database_->IsOpen(), + "database is not open"); THROW_AND_RETURN_ON_BAD_STATE( env, session->session_ == nullptr, "session is not open"); @@ -3849,7 +3851,9 @@ void Session::Close(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( - env, !session->database_->IsOpen(), "database is not open"); + env, + !session->database_ || !session->database_->IsOpen(), + "database is not open"); THROW_AND_RETURN_ON_BAD_STATE( env, session->session_ == nullptr, "session is not open"); diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index 189835ce4c3003..b73fa5cd199ed3 100644 --- a/test/parallel/test-sqlite-session.js +++ b/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,34 @@ test('session.close() - closing twice', (t) => { }); }); +test('session - using session after database is garbage collected throws', async (t) => { + const { gcUntil, onGC } = require('../common/gc'); + + // The session only holds a weak reference to its database. Create the + // database in a nested scope so that no strong reference to it survives, + // allowing it to be garbage collected while the session is still alive. + let collected = false; + const session = (() => { + const database = new DatabaseSync(':memory:'); + database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY, value TEXT)'); + onGC(database, { ongc: () => { collected = true; } }); + return database.createSession(); + })(); + + await gcUntil('database is garbage collected', () => collected); + + // Before the fix, these dereferenced a dangling weak pointer to the + // collected database and crashed the process. They must throw instead. + for (const method of ['changeset', 'patchset', 'close']) { + t.assert.throws(() => { + session[method](); + }, { + name: 'Error', + message: 'database is not open', + }); + } +}); + test('session supports ERM', (t) => { const database = new DatabaseSync(':memory:'); let afterDisposeSession; From a827c3ca80a63a5d7b550bf174afe83b668522e8 Mon Sep 17 00:00:00 2001 From: Mohamed Sayed Date: Mon, 8 Jun 2026 20:26:15 +0300 Subject: [PATCH 2/2] fix: keep database alive while a session is open Signed-off-by: Mohamed Sayed --- src/node_sqlite.cc | 18 ++++++------- src/node_sqlite.h | 6 ++--- test/parallel/test-sqlite-session.js | 38 +++++++++++++++------------- 3 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 233b2db4346713..4a503cc20970d8 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -2103,7 +2103,7 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void()); BaseObjectPtr session = - Session::Create(env, BaseObjectWeakPtr(db), pSession); + Session::Create(env, BaseObjectPtr(db), pSession); args.GetReturnValue().Set(session->object()); } @@ -3770,7 +3770,7 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo& args) { Session::Session(Environment* env, Local object, - BaseObjectWeakPtr database, + BaseObjectPtr database, sqlite3_session* session) : BaseObject(env, object), session_(session), @@ -3783,7 +3783,7 @@ Session::~Session() { } BaseObjectPtr Session::Create(Environment* env, - BaseObjectWeakPtr database, + BaseObjectPtr database, sqlite3_session* session) { Local obj; if (!GetConstructorTemplate(env) @@ -3817,7 +3817,9 @@ Local Session::GetConstructorTemplate(Environment* env) { return tmpl; } -void Session::MemoryInfo(MemoryTracker* tracker) const {} +void Session::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("database", database_); +} template void Session::Changeset(const FunctionCallbackInfo& args) { @@ -3825,9 +3827,7 @@ void Session::Changeset(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( - env, - !session->database_ || !session->database_->IsOpen(), - "database is not open"); + env, !session->database_->IsOpen(), "database is not open"); THROW_AND_RETURN_ON_BAD_STATE( env, session->session_ == nullptr, "session is not open"); @@ -3851,9 +3851,7 @@ void Session::Close(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( - env, - !session->database_ || !session->database_->IsOpen(), - "database is not open"); + env, !session->database_->IsOpen(), "database is not open"); THROW_AND_RETURN_ON_BAD_STATE( env, session->session_ == nullptr, "session is not open"); diff --git a/src/node_sqlite.h b/src/node_sqlite.h index e7281ed266af5d..84c0e26e61ad8f 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -339,7 +339,7 @@ class Session : public BaseObject { public: Session(Environment* env, v8::Local object, - BaseObjectWeakPtr database, + BaseObjectPtr database, sqlite3_session* session); ~Session() override; template @@ -349,7 +349,7 @@ class Session : public BaseObject { static v8::Local GetConstructorTemplate( Environment* env); static BaseObjectPtr Create(Environment* env, - BaseObjectWeakPtr database, + BaseObjectPtr 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 database_; // The Parent Database + BaseObjectPtr database_; // The Parent Database }; class SQLTagStore : public BaseObject { diff --git a/test/parallel/test-sqlite-session.js b/test/parallel/test-sqlite-session.js index b73fa5cd199ed3..a91b9af2e8908f 100644 --- a/test/parallel/test-sqlite-session.js +++ b/test/parallel/test-sqlite-session.js @@ -589,32 +589,34 @@ test('session.close() - closing twice', (t) => { }); }); -test('session - using session after database is garbage collected throws', async (t) => { +test('session - keeps its database alive after the db handle is dropped', async (t) => { const { gcUntil, onGC } = require('../common/gc'); - // The session only holds a weak reference to its database. Create the - // database in a nested scope so that no strong reference to it survives, - // allowing it to be garbage collected while the session is still alive. - let collected = false; + // 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: () => { collected = true; } }); - return database.createSession(); + onGC(database, { ongc: () => { dbCollected = true; } }); + const s = database.createSession(); + database.exec("INSERT INTO data VALUES (1, 'hello')"); + return s; })(); - await gcUntil('database is garbage collected', () => collected); + // 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); - // Before the fix, these dereferenced a dangling weak pointer to the - // collected database and crashed the process. They must throw instead. - for (const method of ['changeset', 'patchset', 'close']) { - t.assert.throws(() => { - session[method](); - }, { - name: 'Error', - message: 'database is not open', - }); - } + // 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) => {