FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: fix crash when a session outlives its database · nodejs/node@c2a5251 · GitHub

/ node Public

Commit c2a5251

Browse files
3zrv
authored and
aduh95
committed
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>
1 parent d7d4f0e commit c2a5251

3 files changed

Lines changed: 40 additions & 8 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2124,7 +2124,7 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo<Value>& args) {
21242124
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
21252125

21262126
BaseObjectPtr<Session> session =
2127-
Session::Create(env, BaseObjectWeakPtr<DatabaseSync>(db), pSession);
2127+
Session::Create(env, BaseObjectPtr<DatabaseSync>(db), pSession);
21282128
args.GetReturnValue().Set(session->object());
21292129
}
21302130

@@ -3803,7 +3803,7 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
38033803

38043804
Session::Session(Environment* env,
38053805
Local<Object> object,
3806-
BaseObjectWeakPtr<DatabaseSync> database,
3806+
BaseObjectPtr<DatabaseSync> database,
38073807
sqlite3_session* session)
38083808
: BaseObject(env, object),
38093809
session_(session),
@@ -3816,7 +3816,7 @@ Session::~Session() {
38163816
}
38173817

38183818
BaseObjectPtr<Session> Session::Create(Environment* env,
3819-
BaseObjectWeakPtr<DatabaseSync> database,
3819+
BaseObjectPtr<DatabaseSync> database,
38203820
sqlite3_session* session) {
38213821
Local<Object> obj;
38223822
if (!GetConstructorTemplate(env)
@@ -3850,7 +3850,9 @@ Local<FunctionTemplate> Session::GetConstructorTemplate(Environment* env) {
38503850
return tmpl;
38513851
}
38523852

3853-
void Session::MemoryInfo(MemoryTracker* tracker) const {}
3853+
void Session::MemoryInfo(MemoryTracker* tracker) const {
3854+
tracker->TrackField("database", database_);
3855+
}
38543856

38553857
template <Sqlite3ChangesetGenFunc sqliteChangesetFunc>
38563858
void Session::Changeset(const FunctionCallbackInfo<Value>& args) {

‎src/node_sqlite.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ class Session : public BaseObject {
339339
public:
340340
Session(Environment* env,
341341
v8::Local<v8::Object> object,
342-
BaseObjectWeakPtr<DatabaseSync> database,
342+
BaseObjectPtr<DatabaseSync> database,
343343
sqlite3_session* session);
344344
~Session() override;
345345
template <Sqlite3ChangesetGenFunc sqliteChangesetFunc>
@@ -349,7 +349,7 @@ class Session : public BaseObject {
349349
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
350350
Environment* env);
351351
static BaseObjectPtr<Session> Create(Environment* env,
352-
BaseObjectWeakPtr<DatabaseSync> database,
352+
BaseObjectPtr<DatabaseSync> database,
353353
sqlite3_session* session);
354354

355355
void MemoryInfo(MemoryTracker* tracker) const override;
@@ -359,7 +359,7 @@ class Session : public BaseObject {
359359
private:
360360
void Delete();
361361
sqlite3_session* session_;
362-
BaseObjectWeakPtr<DatabaseSync> database_; // The Parent Database
362+
BaseObjectPtr<DatabaseSync> database_; // The Parent Database
363363
};
364364

365365
class SQLTagStore : public BaseObject {

‎test/parallel/test-sqlite-session.js‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Flags: --experimental-sqlite
1+
// Flags: --expose-gc --experimental-sqlite
22
'use strict';
33
const { skipIfSQLiteMissing } = require('../common');
44
skipIfSQLiteMissing();
@@ -589,6 +589,36 @@ test('session.close() - closing twice', (t) => {
589589
});
590590
});
591591

592+
test('session - keeps its database alive after the db handle is dropped', async (t) => {
593+
const { gcUntil, onGC } = require('../common/gc');
594+
595+
// The DatabaseSync handle is created in a nested scope and never referenced
596+
// again, so the returned session is the only thing keeping it reachable.
597+
let dbCollected = false;
598+
const session = (() => {
599+
const database = new DatabaseSync(':memory:');
600+
database.exec('CREATE TABLE data(key INTEGER PRIMARY KEY, value TEXT)');
601+
onGC(database, { ongc: () => { dbCollected = true; } });
602+
const s = database.createSession();
603+
database.exec("INSERT INTO data VALUES (1, 'hello')");
604+
return s;
605+
})();
606+
607+
// The session must keep the database alive across GC. Previously it held
608+
// only a weak reference, so the database could be collected and using the
609+
// session afterwards dereferenced a dangling pointer and crashed.
610+
await gcUntil('database is collected', () => dbCollected, 5).then(
611+
() => { throw new Error('session did not keep its database alive'); },
612+
() => {}, // Expected: the database is never collected, so gcUntil rejects.
613+
);
614+
t.assert.strictEqual(dbCollected, false);
615+
616+
// The database is still open and usable through the still-alive session.
617+
const changeset = session.changeset();
618+
t.assert.ok(changeset.byteLength > 0);
619+
session.close();
620+
});
621+
592622
test('session supports ERM', (t) => {
593623
const database = new DatabaseSync(':memory:');
594624
let afterDisposeSession;

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL