Skip to content

sqlite: fix segfault SQLTagStore when db handle is garbage collected #60462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
louwers wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

sqlite: fix segfault SQLTagStore when db handle is garbage collected #60462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
800d5a1
sqlite: fix segfault SQLTagStore when db handle is garbage collected
louwers Oct 28, 2025
37f0477
use INTEGER instead of NUMBER
louwers Oct 28, 2025
f2039eb
test: use --expose-gc
louwers Oct 28, 2025
b633db4
test: rename test and remove GitHub link
louwers Oct 28, 2025
b8df359
test: run linter
louwers Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/node_sqlite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,7 +832,7 @@ void DatabaseSync::CreateTagStore(const FunctionCallbackInfo<Value>& args) {
}

BaseObjectPtr<SQLTagStore> session =
SQLTagStore::Create(env, BaseObjectWeakPtr<DatabaseSync>(db), capacity);
SQLTagStore::Create(env, BaseObjectPtr<DatabaseSync>(db), capacity);
if (!session) {
// Handle error if creation failed
THROW_ERR_SQLITE_ERROR(env->isolate(), "Failed to create SQLTagStore");
Expand Down Expand Up @@ -2660,7 +2660,7 @@ void IllegalConstructor(const FunctionCallbackInfo<Value>& args) {

SQLTagStore::SQLTagStore(Environment* env,
Local<Object> object,
BaseObjectWeakPtr<DatabaseSync> database,
BaseObjectPtr<DatabaseSync> database,
int capacity)
: BaseObject(env, object),
database_(std::move(database)),
Expand Down Expand Up @@ -2709,7 +2709,7 @@ Local<FunctionTemplate> SQLTagStore::GetConstructorTemplate(Environment* env) {
}

BaseObjectPtr<SQLTagStore> SQLTagStore::Create(
Environment* env, BaseObjectWeakPtr<DatabaseSync> database, int capacity) {
Environment* env, BaseObjectPtr<DatabaseSync> database, int capacity) {
Local<Object> obj;
if (!GetConstructorTemplate(env)
->InstanceTemplate()
Expand Down
9 changes: 5 additions & 4 deletions src/node_sqlite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,11 +306,12 @@ class SQLTagStore : public BaseObject {
public:
SQLTagStore(Environment* env,
v8::Local<v8::Object> object,
BaseObjectWeakPtr<DatabaseSync> database,
BaseObjectPtr<DatabaseSync> database,
int capacity);
~SQLTagStore() override;
static BaseObjectPtr<SQLTagStore> Create(
Environment* env, BaseObjectWeakPtr<DatabaseSync> database, int capacity);
static BaseObjectPtr<SQLTagStore> Create(Environment* env,
BaseObjectPtr<DatabaseSync> database,
int capacity);
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
Environment* env);
static void All(const v8::FunctionCallbackInfo<v8::Value>& info);
Expand All @@ -329,7 +330,7 @@ class SQLTagStore : public BaseObject {
private:
static BaseObjectPtr<StatementSync> PrepareStatement(
const v8::FunctionCallbackInfo<v8::Value>& args);
BaseObjectWeakPtr<DatabaseSync> database_;
BaseObjectPtr<DatabaseSync> database_;
Copy link
Member

@addaleax addaleax Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the DatabaseSync object somehow refers back to the SQLTagStore object, this creates a memory leak.

Would it make sense to keep storing a BaseObjectWeakPtr but to keep it alive by also storing the associated JS object in an internal field of SQLTagStore?

Copy link
Contributor

@geeksilva97 geeksilva97 Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the DatabaseSync object somehow refers back to the SQLTagStore object, this creates a memory leak.

We're good. DatabaseSync holds no reference to SQLTagStore.

Copy link
Member

@addaleax addaleax Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geeksilva97 Why do we know that that's true?

DatabaseSync has an associated JS object and can refers to other JS objects (e.g. an authorizer callback) which in turn can refer to the SQLTagStore – what am I missing?

Copy link
Contributor

@geeksilva97 geeksilva97 Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I got it.

Are you referring to a circular dependency? If so, that's the part I'm unable to visualize. You mean like in JS-land, if one has an authorizer callback holding an sqltagstore in a closure or something?

Copy link
Contributor Author

@louwers louwers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to reproduce this scenario.

I don't know a lot about V8's garbage collector, but it seems to be able to do its job here.

Copy link
Member

@addaleax addaleax Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to a circular dependency?

Yes, but this dependency here (the one represented by the BaseObjectPtr) isn't visible to GC.

You mean like in JS-land, if one has an authorizer callback holding an sqltagstore in a closure or something?

Yup!

I was not able to reproduce this scenario.

Just going to quote myself here:

DatabaseSync has an associated JS object and can refers to other JS objects (e.g. an authorizer callback) which in turn can refer to the SQLTagStore – what am I missing?

Turning this into runnable code is pretty straightforward:

while (true) {
  const sql = new DatabaseSync(':memory:').createTagStore();

  sql.db.exec('CREATE TABLE test (data INTEGER)');
  sql.db.setAuthorizer(() => void sql.db);
  console.log(process.memoryUsage())
}

before this PR this crashes, now this leaks.

Copy link
Contributor Author

@louwers louwers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. Good catch!

Copy link
Contributor

@geeksilva97 geeksilva97 Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, @addaleax

LRUCache<std::string, BaseObjectPtr<StatementSync>> sql_tags_;
int capacity_;
friend class StatementExecutionHelper;
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-sqlite-template-tag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
'use strict';
// Flags: --expose-gc

require('../common');
const assert = require('assert');
const { DatabaseSync } = require('node:sqlite');
Expand Down Expand Up @@ -100,3 +102,14 @@ test('TagStore capacity, size, and clear', () => {
test('sql.db returns the associated DatabaseSync instance', () => {
assert.strictEqual(sql.db, db);
});

test('a tag store keeps the database alive by itself', () => {
const sql = new DatabaseSync(':memory:').createTagStore();

sql.db.exec('CREATE TABLE test (data INTEGER)');

global.gc();

// eslint-disable-next-line no-unused-expressions
sql.run`INSERT INTO test (data) VALUES (1)`;
});
Loading