Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 32 additions & 0 deletions src/node_sqlite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1718,6 +1718,10 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo<Value>& args) {
}
}

// Reading the options bag can run a getter that closes the connection.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we can just check only once after arguments validation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

  • test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
  • test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

Utf8Value sql(env->isolate(), args[0].As<String>());
sqlite3_stmt* s = nullptr;

Expand Down Expand Up @@ -1908,6 +1912,10 @@ void DatabaseSync::CustomFunction(const FunctionCallbackInfo<Value>& args) {
argc = js_len.As<Int32>()->Value();
}

// Reading the options bag can run a getter that closes the connection.
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

UserDefinedFunction* user_data = new UserDefinedFunction(
env, fn, BaseObjectWeakPtr<DatabaseSync>(db), use_bigint_args);
int text_rep = SQLITE_UTF8;
Expand Down Expand Up @@ -2070,6 +2078,13 @@ void DatabaseSync::Deserialize(const FunctionCallbackInfo<Value>& args) {
}
}

// Reading the options bag can run a getter that closes the connection.
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_ON_BAD_STATE(
env,
db->IsInCallback(),
"database cannot be deserialized while in a callback");

// sqlite3_malloc64 is required because SQLITE_DESERIALIZE_FREEONCLOSE
// transfers ownership to SQLite, which calls sqlite3_free() on close.
// See: https://www.sqlite.org/c3ref/deserialize.html
Expand Down Expand Up @@ -2229,6 +2244,10 @@ void DatabaseSync::AggregateFunction(const FunctionCallbackInfo<Value>& args) {
argc = std::max({argc, js_len.As<Int32>()->Value() - 1, 0});
}

// Reading the options bag can run a getter that closes the connection.
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

int text_rep = SQLITE_UTF8;
if (direct_only) {
text_rep |= SQLITE_DIRECTONLY;
Expand Down Expand Up @@ -2441,6 +2460,9 @@ void Backup(const FunctionCallbackInfo<Value>& args) {
}
}

// Reading the options bag can run a getter that closes the connection.
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");

Local<Promise::Resolver> resolver;
if (!Promise::Resolver::New(env->context()).ToLocal(&resolver)) {
return;
Expand Down Expand Up @@ -2584,6 +2606,10 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo<Value>& args) {
}
}

// Reading the options bag can run a getter that closes the connection.
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

// Keep the database alive during sqlite3changeset_apply(), which may
// call conflict or filter callbacks that trigger JavaScript execution.
// If the JavaScript callback drops all references to the database,
Expand Down Expand Up @@ -3968,6 +3994,12 @@ BaseObjectPtr<StatementSync> SQLTagStore::PrepareStatement(
}
}

// Reading the template parts can run a getter that closes the connection.
if (!session->database_->IsOpen()) {
THROW_ERR_INVALID_STATE(env, "database is not open");
return BaseObjectPtr<StatementSync>();
}

BaseObjectPtr<StatementSync> stmt = nullptr;
if (session->sql_tags_.Exists(sql)) {
stmt = session->sql_tags_.Get(sql);
Expand Down
134 changes: 134 additions & 0 deletions test/parallel/test-sqlite-close-from-options-getter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
'use strict';

const { skipIfSQLiteMissing } = require('../common');
skipIfSQLiteMissing();
const tmpdir = require('../common/tmpdir');
const assert = require('node:assert');
const { join } = require('node:path');
const { test } = require('node:test');
const { backup, DatabaseSync } = require('node:sqlite');

tmpdir.refresh();

const closedError = {
code: 'ERR_INVALID_STATE',
message: 'database is not open',
};

// Reading the options bag runs a user getter, so the connection validated on
// entry can already be closed by the time the method reaches SQLite.
test('function() with a getter that closes the database', () => {
const db = new DatabaseSync(':memory:');
const options = {
get useBigIntArguments() {
db.close();
return false;
},
};

assert.throws(() => {
db.function('custom', options, () => 1);
}, closedError);
});

test('aggregate() with a getter that closes the database', () => {
const db = new DatabaseSync(':memory:');
const options = {
start: 0,
step: (acc, value) => acc + value,
get useBigIntArguments() {
db.close();
return false;
},
};

assert.throws(() => {
db.aggregate('custom', options);
}, closedError);
});

test('deserialize() with a getter that closes the database', () => {
const source = new DatabaseSync(':memory:');
source.exec('CREATE TABLE data (value INTEGER)');
const serialized = source.serialize();
source.close();

const db = new DatabaseSync(':memory:');
const options = {
get dbName() {
db.close();
return 'main';
},
};

assert.throws(() => {
db.deserialize(serialized, options);
}, closedError);
});

test('prepare() with a getter that closes the database', () => {
const db = new DatabaseSync(':memory:');
const options = {
get persistent() {
db.close();
return false;
},
};

assert.throws(() => {
db.prepare('SELECT 1', options);
}, closedError);
});

test('applyChangeset() with a getter that closes the database', () => {
const source = new DatabaseSync(':memory:');
source.exec('CREATE TABLE data (value INTEGER PRIMARY KEY)');
const session = source.createSession();
source.exec('INSERT INTO data VALUES (1)');
const changeset = session.changeset();
source.close();

const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE data (value INTEGER PRIMARY KEY)');
const options = {
get onConflict() {
db.close();
return undefined;
},
};

assert.throws(() => {
db.applyChangeset(changeset, options);
}, closedError);
});

test('tag store with a getter that closes the database', () => {
const db = new DatabaseSync(':memory:');
const sql = db.createTagStore(10);
const strings = ['SELECT ', ''];
Object.defineProperty(strings, 0, {
get() {
db.close();
return 'SELECT ';
},
});

assert.throws(() => {
sql.get(strings, 1);
}, closedError);
});

test('backup() with a getter that closes the database', () => {
const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE data (value INTEGER)');
const options = {
get rate() {
db.close();
return 1;
},
};

assert.throws(() => {
backup(db, join(tmpdir.path, 'backup.db'), options);
}, closedError);
});
Loading