From 14cd548ce26abde544c3f37358b89da8a244b5bc Mon Sep 17 00:00:00 2001 From: Matthew McEachen Date: Thu, 7 May 2026 17:56:07 -0700 Subject: [PATCH 1/3] sqlite: fix crash on db.close() from inside a user function Calling db.close() from inside a user-defined function callback while sqlite3_step is on the call stack caused two distinct crashes: 1. DatabaseSync::Close ran sqlite3_finalize on the statement whose sqlite3_step frame was still active, freeing the VM that step was executing. The outer step then operated on freed memory. 2. Even if (1) is avoided, StatementExecutionHelper::Run dereferenced db->Connection() via sqlite3_last_insert_rowid / sqlite3_changes64 after step returned. The reentrant close zeroed connection_, so the deref crashed. Add a MarkStepping() RAII guard wrapped around every sqlite3_step caller. If Finalize() is called while stepping_, defer it; the guard's destructor runs the deferred finalize after step returns. Add a connection-null check in StatementExecutionHelper::Run before the connection-dependent reads, throwing ERR_INVALID_STATE. Fixes: https://github.com/nodejs/node/issues/63180 Signed-off-by: Matthew McEachen --- src/node_sqlite.cc | 23 ++++- src/node_sqlite.h | 15 ++++ test/parallel/test-sqlite-custom-functions.js | 84 +++++++++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 35dbe1ffc50772..f8674d321da2f9 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -2573,6 +2573,13 @@ StatementSync::~StatementSync() { } void StatementSync::Finalize() { + if (statement_ == nullptr) { + return; + } + if (stepping_) { + finalize_pending_ = true; + return; + } sqlite3_finalize(statement_); statement_ = nullptr; InvalidateColumnNameCache(); @@ -2898,6 +2905,10 @@ MaybeLocal StatementExecutionHelper::Run(Environment* env, EscapableHandleScope scope(isolate); sqlite3_step(stmt); int r = sqlite3_reset(stmt); + if (db->Connection() == nullptr) { + THROW_ERR_INVALID_STATE(env, "database connection was closed mid-query"); + return MaybeLocal(); + } CHECK_ERROR_OR_THROW(isolate, db, r, SQLITE_OK, MaybeLocal()); sqlite3_int64 last_insert_rowid = sqlite3_last_insert_rowid(db->Connection()); @@ -3026,9 +3037,9 @@ void StatementSync::All(const FunctionCallbackInfo& args) { return; } - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); - Local result; + auto step = stmt->MarkStepping(); + auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); if (StatementExecutionHelper::All(env, stmt->db_.get(), stmt->statement_, @@ -3076,6 +3087,7 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Get(env, stmt->db_.get(), stmt->statement_, @@ -3100,6 +3112,7 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Run( env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_) .ToLocal(&result)) { @@ -3364,6 +3377,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Run( env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_) .ToLocal(&result)) { @@ -3435,6 +3449,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Get(env, stmt->db_.get(), stmt->statement_, @@ -3473,8 +3488,9 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { } } - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); Local result; + auto step = stmt->MarkStepping(); + auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); if (StatementExecutionHelper::All(env, stmt->db_.get(), stmt->statement_, @@ -3701,6 +3717,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { iter->statement_reset_generation_ != iter->stmt_->reset_generation_, "iterator was invalidated"); + auto step = iter->stmt_->MarkStepping(); int r = sqlite3_step(iter->stmt_->statement_); if (r != SQLITE_ROW) { CHECK_ERROR_OR_THROW( diff --git a/src/node_sqlite.h b/src/node_sqlite.h index e7281ed266af5d..010e1b20cbd361 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -284,6 +284,19 @@ class StatementSync : public BaseObject { void Finalize(); bool IsFinalized(); + // RAII guard: defer Finalize() if called while sqlite3_step is on the + // stack for this statement. Wrap every sqlite3_step caller. + inline auto MarkStepping() { + stepping_ = true; + return OnScopeLeave([this]() { + stepping_ = false; + if (finalize_pending_) { + finalize_pending_ = false; + Finalize(); + } + }); + } + SET_MEMORY_INFO_NAME(StatementSync) SET_SELF_SIZE(StatementSync) @@ -295,6 +308,8 @@ class StatementSync : public BaseObject { bool use_big_ints_; bool allow_bare_named_params_; bool allow_unknown_named_params_; + bool stepping_ = false; + bool finalize_pending_ = false; uint64_t reset_generation_ = 0; std::optional> bare_named_params_; inline int ResetStatement(); diff --git a/test/parallel/test-sqlite-custom-functions.js b/test/parallel/test-sqlite-custom-functions.js index 6b5f974ede893e..d46af1f1e31e18 100644 --- a/test/parallel/test-sqlite-custom-functions.js +++ b/test/parallel/test-sqlite-custom-functions.js @@ -411,4 +411,88 @@ suite('DatabaseSync.prototype.function()', () => { message: /database is not open/, }); }); + + suite('reentrant db.close() from inside a user function callback', () => { + function setup() { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.prepare('INSERT INTO t VALUES (1, 10)').run(); + db.prepare('INSERT INTO t VALUES (2, 20)').run(); + db.function('close_db', (x) => { + try { db.close(); } catch { /* already closed on later rows */ } + return x; + }); + return db; + } + + test('.all() completes without crashing', () => { + const db = setup(); + const rows = db.prepare('SELECT close_db(v) AS v FROM t').all(); + assert.ok(Array.isArray(rows)); + }); + + test('.get() completes without crashing', () => { + const db = setup(); + const row = db.prepare('SELECT close_db(v) AS v FROM t').get(); + assert.deepStrictEqual(row, { __proto__: null, v: 10 }); + }); + + test('.run() throws ERR_INVALID_STATE rather than crashing', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.function('close_db', (x) => { + try { db.close(); } catch { /* */ } + return x; + }); + assert.throws( + () => db.prepare('INSERT INTO t SELECT close_db(1), close_db(2)').run(), + { code: 'ERR_INVALID_STATE', message: /closed mid-query/ }, + ); + }); + + test('iterator returns first row, then throws on next()', () => { + const db = setup(); + const iter = db.prepare('SELECT close_db(v) AS v FROM t').iterate(); + assert.strictEqual(iter.next().done, false); + assert.throws(() => iter.next(), { + code: 'ERR_INVALID_STATE', + message: /statement has been finalized/, + }); + }); + + test('SQL tag store .run() throws ERR_INVALID_STATE', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.function('close_db', (x) => { + try { db.close(); } catch { /* */ } + return x; + }); + const sql = db.createTagStore(4); + assert.throws( + () => sql.run`INSERT INTO t SELECT close_db(${1}), close_db(${2})`, + { code: 'ERR_INVALID_STATE', message: /closed mid-query/ }, + ); + }); + + test('SQL tag store .all() completes without crashing', () => { + const db = setup(); + const sql = db.createTagStore(4); + const rows = sql.all`SELECT close_db(v) AS v FROM t`; + assert.ok(Array.isArray(rows)); + }); + + test('SQL tag store .get() completes without crashing', () => { + const db = setup(); + const sql = db.createTagStore(4); + const row = sql.get`SELECT close_db(v) AS v FROM t`; + assert.deepStrictEqual(row, { __proto__: null, v: 10 }); + }); + + test('a fresh database works after the reentrant close', () => { + setup().prepare('SELECT close_db(v) FROM t').all(); + const db2 = new DatabaseSync(':memory:'); + assert.deepStrictEqual(db2.prepare('SELECT 1 AS x').get(), + { __proto__: null, x: 1 }); + }); + }); }); From 20d7ffa2eac8256f4dc39f85d4df95685ccb25b1 Mon Sep 17 00:00:00 2001 From: Matthew McEachen Date: Fri, 8 May 2026 20:22:41 -0700 Subject: [PATCH 2/3] sqlite: reject forbidden ops inside user-defined function callbacks Per SQLite's sqlite3_create_function() contract, closing the database or finalizing/resetting a statement is forbidden while a user-supplied callback is on the stack. The previous fix detected the close case and deferred the finalize after sqlite3_step returned. Per reviewer feedback, prevent the forbidden operations up front instead: track a user-callback depth on DatabaseSync (via an RAII guard wrapped around every JS call from xFunc, the aggregate xStep/xValue/xFinal/xInverse and start callbacks, and the authorizer callback), and have db.close(), the statement execution methods, the iterator's next/return, and the SQL tag store's run/get/all/iterate/clear throw ERR_INVALID_STATE when called from inside such a callback. Removes the now-unreachable deferred-finalize machinery (stepping_, finalize_pending_, MarkStepping) and the connection-null check in StatementExecutionHelper::Run. Adds tests for scalar, aggregate, and authorizer callbacks, and for reentrant iter.next(). Signed-off-by: Matthew McEachen --- src/node_sqlite.cc | 115 +++++++-- src/node_sqlite.h | 30 +-- test/parallel/test-sqlite-custom-functions.js | 240 +++++++++++++++--- 3 files changed, 300 insertions(+), 85 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index f8674d321da2f9..928f4fe3b3d298 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -386,13 +386,16 @@ class CustomAggregate { } Local ret; - if (!(self->*mptr) - .Get(isolate) - ->Call(env->context(), recv, argc + 1, js_argv.data()) - .ToLocal(&ret)) { - self->db_->SetIgnoreNextSQLiteError(true); - sqlite3_result_error(ctx, "", 0); - return; + { + auto guard = self->db_->EnterUserFunctionCallback(); + if (!(self->*mptr) + .Get(isolate) + ->Call(env->context(), recv, argc + 1, js_argv.data()) + .ToLocal(&ret)) { + self->db_->SetIgnoreNextSQLiteError(true); + sqlite3_result_error(ctx, "", 0); + return; + } } agg->value.Reset(isolate, ret); @@ -422,6 +425,7 @@ class CustomAggregate { Local::New(env->isolate(), self->result_fn_); Local js_arg[] = {Local::New(isolate, agg->value)}; + auto guard = self->db_->EnterUserFunctionCallback(); if (!fn->Call(env->context(), Null(isolate), 1, js_arg) .ToLocal(&result)) { self->db_->SetIgnoreNextSQLiteError(true); @@ -455,6 +459,7 @@ class CustomAggregate { Local start_v = Local::New(isolate, start_); if (start_v->IsFunction()) { auto fn = start_v.As(); + auto guard = db_->EnterUserFunctionCallback(); MaybeLocal retval = fn->Call(env_->context(), Null(isolate), 0, nullptr); if (!retval.ToLocal(&start_v)) { @@ -698,6 +703,7 @@ void UserDefinedFunction::xFunc(sqlite3_context* ctx, js_argv.emplace_back(local); } + auto guard = self->db_->EnterUserFunctionCallback(); MaybeLocal retval = fn->Call(env->context(), recv, argc, js_argv.data()); Local result; @@ -1420,6 +1426,10 @@ void DatabaseSync::Close(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + db->IsInUserFunctionCallback(), + "database cannot be closed inside a user-defined function callback"); db->FinalizeStatements(); db->DeleteSessions(); int r = sqlite3_close_v2(db->connection_); @@ -1808,6 +1818,11 @@ void DatabaseSync::Deserialize(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + db->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); if (!args[0]->IsUint8Array()) { THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -2505,8 +2520,12 @@ int DatabaseSync::AuthorizerCallback(void* user_data, NullableSQLiteStringToValue(isolate, param4).ToLocalChecked(), }); - MaybeLocal retval = callback->Call( - context, Undefined(isolate), js_argv.size(), js_argv.data()); + MaybeLocal retval; + { + auto guard = db->EnterUserFunctionCallback(); + retval = callback->Call( + context, Undefined(isolate), js_argv.size(), js_argv.data()); + } Local result; @@ -2576,10 +2595,6 @@ void StatementSync::Finalize() { if (statement_ == nullptr) { return; } - if (stepping_) { - finalize_pending_ = true; - return; - } sqlite3_finalize(statement_); statement_ = nullptr; InvalidateColumnNameCache(); @@ -2905,10 +2920,6 @@ MaybeLocal StatementExecutionHelper::Run(Environment* env, EscapableHandleScope scope(isolate); sqlite3_step(stmt); int r = sqlite3_reset(stmt); - if (db->Connection() == nullptr) { - THROW_ERR_INVALID_STATE(env, "database connection was closed mid-query"); - return MaybeLocal(); - } CHECK_ERROR_OR_THROW(isolate, db, r, SQLITE_OK, MaybeLocal()); sqlite3_int64 last_insert_rowid = sqlite3_last_insert_rowid(db->Connection()); @@ -3029,6 +3040,11 @@ void StatementSync::All(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + stmt->db_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); Isolate* isolate = env->isolate(); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void()); @@ -3038,7 +3054,6 @@ void StatementSync::All(const FunctionCallbackInfo& args) { } Local result; - auto step = stmt->MarkStepping(); auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3056,6 +3071,11 @@ void StatementSync::Iterate(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + stmt->db_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3079,6 +3099,11 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + stmt->db_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3087,7 +3112,6 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { } Local result; - auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Get(env, stmt->db_.get(), stmt->statement_, @@ -3104,6 +3128,11 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + stmt->db_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3112,7 +3141,6 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { } Local result; - auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Run( env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_) .ToLocal(&result)) { @@ -3358,6 +3386,11 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + session->database_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3366,7 +3399,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { } uint32_t n_params = args.Length() - 1; - int r = sqlite3_reset(stmt->statement_); + int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); int param_count = sqlite3_bind_parameter_count(stmt->statement_); for (int i = 0; i < static_cast(n_params) && i < param_count; ++i) { @@ -3377,7 +3410,6 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { } Local result; - auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Run( env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_) .ToLocal(&result)) { @@ -3392,6 +3424,11 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + session->database_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3400,7 +3437,7 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo& args) { } uint32_t n_params = args.Length() - 1; - int r = sqlite3_reset(stmt->statement_); + int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); int param_count = sqlite3_bind_parameter_count(stmt->statement_); for (int i = 0; i < static_cast(n_params) && i < param_count; ++i) { @@ -3427,6 +3464,11 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + session->database_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3437,7 +3479,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { uint32_t n_params = args.Length() - 1; Isolate* isolate = env->isolate(); - int r = sqlite3_reset(stmt->statement_); + int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void()); int param_count = sqlite3_bind_parameter_count(stmt->statement_); @@ -3449,7 +3491,6 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { } Local result; - auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Get(env, stmt->db_.get(), stmt->statement_, @@ -3467,6 +3508,11 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + session->database_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3477,7 +3523,7 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { uint32_t n_params = args.Length() - 1; Isolate* isolate = env->isolate(); - int r = sqlite3_reset(stmt->statement_); + int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void()); int param_count = sqlite3_bind_parameter_count(stmt->statement_); @@ -3489,7 +3535,6 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { } Local result; - auto step = stmt->MarkStepping(); auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3504,6 +3549,11 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { void SQLTagStore::Clear(const FunctionCallbackInfo& args) { SQLTagStore* store; ASSIGN_OR_RETURN_UNWRAP(&store, args.This()); + Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE( + env, + store->database_->IsInUserFunctionCallback(), + "tag store cannot be cleared inside a user-defined function callback"); store->sql_tags_.Clear(); } @@ -3695,6 +3745,11 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, iter->stmt_->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + iter->stmt_->db_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); Isolate* isolate = env->isolate(); auto iter_template = getLazyIterTemplate(env); @@ -3717,7 +3772,6 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { iter->statement_reset_generation_ != iter->stmt_->reset_generation_, "iterator was invalidated"); - auto step = iter->stmt_->MarkStepping(); int r = sqlite3_step(iter->stmt_->statement_); if (r != SQLITE_ROW) { CHECK_ERROR_OR_THROW( @@ -3772,6 +3826,11 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, iter->stmt_->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_ON_BAD_STATE( + env, + iter->stmt_->db_->IsInUserFunctionCallback(), + "database operation is not allowed inside a user-defined function " + "callback"); Isolate* isolate = env->isolate(); sqlite3_reset(iter->stmt_->statement_); diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 010e1b20cbd361..f8f629c0387292 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -221,6 +221,20 @@ class DatabaseSync : public BaseObject { } sqlite3* Connection(); + // SQLite forbids closing the database, finalizing/resetting the running + // statement, or recursively stepping while a user-supplied callback + // (scalar or aggregate function, or authorizer) is on the stack. Wrap + // every such callback with the RAII guard returned by + // EnterUserFunctionCallback(); JS-callable methods that would perform a + // forbidden operation must check IsInUserFunctionCallback() and throw. + inline auto EnterUserFunctionCallback() { + user_function_callback_depth_++; + return OnScopeLeave([this]() { user_function_callback_depth_--; }); + } + bool IsInUserFunctionCallback() const { + return user_function_callback_depth_ > 0; + } + // In some situations, such as when using custom functions, it is possible // that SQLite reports an error while JavaScript already has a pending // exception. In this case, the SQLite error should be ignored. These methods @@ -241,6 +255,7 @@ class DatabaseSync : public BaseObject { bool enable_load_extension_; sqlite3* connection_; bool ignore_next_sqlite_error_; + int user_function_callback_depth_ = 0; std::set backups_; std::set sessions_; @@ -284,19 +299,6 @@ class StatementSync : public BaseObject { void Finalize(); bool IsFinalized(); - // RAII guard: defer Finalize() if called while sqlite3_step is on the - // stack for this statement. Wrap every sqlite3_step caller. - inline auto MarkStepping() { - stepping_ = true; - return OnScopeLeave([this]() { - stepping_ = false; - if (finalize_pending_) { - finalize_pending_ = false; - Finalize(); - } - }); - } - SET_MEMORY_INFO_NAME(StatementSync) SET_SELF_SIZE(StatementSync) @@ -308,8 +310,6 @@ class StatementSync : public BaseObject { bool use_big_ints_; bool allow_bare_named_params_; bool allow_unknown_named_params_; - bool stepping_ = false; - bool finalize_pending_ = false; uint64_t reset_generation_ = 0; std::optional> bare_named_params_; inline int ResetStatement(); diff --git a/test/parallel/test-sqlite-custom-functions.js b/test/parallel/test-sqlite-custom-functions.js index d46af1f1e31e18..d8b94626e951e4 100644 --- a/test/parallel/test-sqlite-custom-functions.js +++ b/test/parallel/test-sqlite-custom-functions.js @@ -1,5 +1,5 @@ 'use strict'; -const { skipIfSQLiteMissing, mustCall } = require('../common'); +const { skipIfSQLiteMissing, mustCall, mustCallAtLeast } = require('../common'); skipIfSQLiteMissing(); const assert = require('node:assert'); const { DatabaseSync } = require('node:sqlite'); @@ -412,87 +412,243 @@ suite('DatabaseSync.prototype.function()', () => { }); }); - suite('reentrant db.close() from inside a user function callback', () => { - function setup() { + suite('reentrant database operations from inside a user function callback', () => { + const reentrantCloseError = { + code: 'ERR_INVALID_STATE', + message: /database cannot be closed inside a user-defined function callback/, + }; + const reentrantOpError = { + code: 'ERR_INVALID_STATE', + message: /database operation is not allowed inside a user-defined function callback/, + }; + + function newDbWithRows() { const db = new DatabaseSync(':memory:'); db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); db.prepare('INSERT INTO t VALUES (1, 10)').run(); db.prepare('INSERT INTO t VALUES (2, 20)').run(); - db.function('close_db', (x) => { - try { db.close(); } catch { /* already closed on later rows */ } - return x; - }); return db; } - test('.all() completes without crashing', () => { - const db = setup(); + test('.all() rejects db.close() and completes without crashing', () => { + const db = newDbWithRows(); + db.function('close_db', mustCall((x) => { + assert.throws(() => db.close(), reentrantCloseError); + return x; + }, 2)); const rows = db.prepare('SELECT close_db(v) AS v FROM t').all(); - assert.ok(Array.isArray(rows)); + assert.deepStrictEqual(rows, [ + { __proto__: null, v: 10 }, + { __proto__: null, v: 20 }, + ]); + assert.strictEqual(db.isOpen, true); }); - test('.get() completes without crashing', () => { - const db = setup(); + test('.get() rejects db.close() and completes without crashing', () => { + const db = newDbWithRows(); + db.function('close_db', mustCall((x) => { + assert.throws(() => db.close(), reentrantCloseError); + return x; + })); const row = db.prepare('SELECT close_db(v) AS v FROM t').get(); assert.deepStrictEqual(row, { __proto__: null, v: 10 }); + assert.strictEqual(db.isOpen, true); }); - test('.run() throws ERR_INVALID_STATE rather than crashing', () => { + test('.run() rejects db.close() and completes without crashing', () => { const db = new DatabaseSync(':memory:'); db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); - db.function('close_db', (x) => { - try { db.close(); } catch { /* */ } + db.function('close_db', mustCall((x) => { + assert.throws(() => db.close(), reentrantCloseError); return x; - }); - assert.throws( - () => db.prepare('INSERT INTO t SELECT close_db(1), close_db(2)').run(), - { code: 'ERR_INVALID_STATE', message: /closed mid-query/ }, + }, 2)); + assert.deepStrictEqual( + db.prepare('INSERT INTO t SELECT close_db(1), close_db(2)').run(), + { changes: 1, lastInsertRowid: 1 }, ); + assert.strictEqual(db.isOpen, true); }); - test('iterator returns first row, then throws on next()', () => { - const db = setup(); + test('iterator rejects db.close() and completes without crashing', () => { + const db = newDbWithRows(); + db.function('close_db', mustCall((x) => { + assert.throws(() => db.close(), reentrantCloseError); + return x; + }, 2)); const iter = db.prepare('SELECT close_db(v) AS v FROM t').iterate(); - assert.strictEqual(iter.next().done, false); - assert.throws(() => iter.next(), { - code: 'ERR_INVALID_STATE', - message: /statement has been finalized/, + assert.deepStrictEqual(iter.next(), { + __proto__: null, + done: false, + value: { __proto__: null, v: 10 }, + }); + assert.deepStrictEqual(iter.next(), { + __proto__: null, + done: false, + value: { __proto__: null, v: 20 }, }); + assert.deepStrictEqual(iter.next(), { + __proto__: null, + done: true, + value: null, + }); + assert.strictEqual(db.isOpen, true); }); - test('SQL tag store .run() throws ERR_INVALID_STATE', () => { + test('SQL tag store .run() rejects db.close() and completes without crashing', () => { const db = new DatabaseSync(':memory:'); db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); - db.function('close_db', (x) => { - try { db.close(); } catch { /* */ } + db.function('close_db', mustCall((x) => { + assert.throws(() => db.close(), reentrantCloseError); return x; - }); + }, 2)); const sql = db.createTagStore(4); - assert.throws( - () => sql.run`INSERT INTO t SELECT close_db(${1}), close_db(${2})`, - { code: 'ERR_INVALID_STATE', message: /closed mid-query/ }, + assert.deepStrictEqual( + sql.run`INSERT INTO t SELECT close_db(${1}), close_db(${2})`, + { changes: 1, lastInsertRowid: 1 }, ); + assert.strictEqual(db.isOpen, true); }); - test('SQL tag store .all() completes without crashing', () => { - const db = setup(); + test('SQL tag store .all() rejects db.close() and completes without crashing', () => { + const db = newDbWithRows(); + db.function('close_db', mustCall((x) => { + assert.throws(() => db.close(), reentrantCloseError); + return x; + }, 2)); const sql = db.createTagStore(4); const rows = sql.all`SELECT close_db(v) AS v FROM t`; - assert.ok(Array.isArray(rows)); + assert.deepStrictEqual(rows, [ + { __proto__: null, v: 10 }, + { __proto__: null, v: 20 }, + ]); + assert.strictEqual(db.isOpen, true); }); - test('SQL tag store .get() completes without crashing', () => { - const db = setup(); + test('SQL tag store .get() rejects db.close() and completes without crashing', () => { + const db = newDbWithRows(); + db.function('close_db', mustCall((x) => { + assert.throws(() => db.close(), reentrantCloseError); + return x; + })); const sql = db.createTagStore(4); const row = sql.get`SELECT close_db(v) AS v FROM t`; assert.deepStrictEqual(row, { __proto__: null, v: 10 }); + assert.strictEqual(db.isOpen, true); + }); + + test('uncaught db.close() failure is propagated from the callback', () => { + const db = new DatabaseSync(':memory:'); + db.function('close_db', () => db.close()); + assert.throws( + () => db.prepare('SELECT close_db()').get(), + reentrantCloseError, + ); + assert.strictEqual(db.isOpen, true); + }); + + test('resetting a statement from a callback is rejected', () => { + const db = new DatabaseSync(':memory:'); + let stmt; + db.function('x', () => stmt.get()); + stmt = db.prepare('SELECT x()'); + assert.throws(() => stmt.get(), reentrantOpError); + assert.strictEqual(db.isOpen, true); + }); + + test('reentrant iter.next() from a callback is rejected', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.prepare('INSERT INTO t VALUES (1, 10)').run(); + let iter; + db.function('reenter', mustCall(() => { + assert.throws(() => iter.next(), reentrantOpError); + return 0; + })); + iter = db.prepare('SELECT reenter() FROM t').iterate(); + assert.strictEqual(iter.next().done, false); + assert.strictEqual(iter.next().done, true); + assert.strictEqual(db.isOpen, true); + }); + + test('aggregate step rejects db.close() and completes', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.prepare('INSERT INTO t VALUES (1, 10)').run(); + db.prepare('INSERT INTO t VALUES (2, 20)').run(); + db.aggregate('agg_close', { + start: 0, + step: mustCall((acc, v) => { + assert.throws(() => db.close(), reentrantCloseError); + return acc + v; + }, 2), + }); + assert.deepStrictEqual( + db.prepare('SELECT agg_close(v) AS s FROM t').get(), + { __proto__: null, s: 30 }, + ); + assert.strictEqual(db.isOpen, true); }); - test('a fresh database works after the reentrant close', () => { - setup().prepare('SELECT close_db(v) FROM t').all(); - const db2 = new DatabaseSync(':memory:'); - assert.deepStrictEqual(db2.prepare('SELECT 1 AS x').get(), - { __proto__: null, x: 1 }); + test('aggregate result rejects db.close() and completes', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.prepare('INSERT INTO t VALUES (1, 10)').run(); + db.aggregate('agg_result', { + start: 0, + step: (acc, v) => acc + v, + result: mustCall((acc) => { + assert.throws(() => db.close(), reentrantCloseError); + return acc; + }), + }); + assert.deepStrictEqual( + db.prepare('SELECT agg_result(v) AS s FROM t').get(), + { __proto__: null, s: 10 }, + ); + assert.strictEqual(db.isOpen, true); + }); + + test('aggregate start function rejects db.close() and completes', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.prepare('INSERT INTO t VALUES (1, 10)').run(); + db.aggregate('agg_start', { + start: mustCall(() => { + assert.throws(() => db.close(), reentrantCloseError); + return 0; + }), + step: (acc, v) => acc + v, + }); + assert.deepStrictEqual( + db.prepare('SELECT agg_start(v) AS s FROM t').get(), + { __proto__: null, s: 10 }, + ); + assert.strictEqual(db.isOpen, true); + }); + + test('authorizer callback rejects db.close()', () => { + const db = new DatabaseSync(':memory:'); + db.setAuthorizer(mustCallAtLeast(() => { + assert.throws(() => db.close(), reentrantCloseError); + return 0; + }, 1)); + assert.deepStrictEqual( + db.prepare('SELECT 1 AS x').get(), + { __proto__: null, x: 1 }, + ); + assert.strictEqual(db.isOpen, true); + }); + + test('SQL tag store clear from a callback is rejected', () => { + const db = new DatabaseSync(':memory:'); + const sql = db.createTagStore(4); + assert.deepStrictEqual(sql.get`SELECT 1 AS one`, { __proto__: null, one: 1 }); + db.function('x', () => sql.clear()); + assert.throws(() => db.prepare('SELECT x()').get(), { + code: 'ERR_INVALID_STATE', + message: /tag store cannot be cleared inside a user-defined function callback/, + }); + assert.strictEqual(sql.size, 1); }); }); }); From c02e2c093f8fcd5c8022d2061faba22d324443d2 Mon Sep 17 00:00:00 2001 From: Matthew McEachen Date: Fri, 8 May 2026 20:51:36 -0700 Subject: [PATCH 3/3] sqlite: narrow user-callback guard to per-statement reentry The previous commit's db-wide IsInUserFunctionCallback guard was too broad: it rejected legitimate cross-statement use from inside a user function (the common "lookup" pattern), e.g. const lookup = db.prepare('SELECT v FROM lookup WHERE id = ?'); db.function('lookup_v', (id) => lookup.get(id).v); SQLite only forbids reentry into the *currently running* statement (recursive sqlite3_step, sqlite3_reset, or sqlite3_finalize), not operations on other statements on the same connection. Track per-statement stepping_ on StatementSync, set via a MarkStepping RAII guard wrapped around each sqlite3_step caller. JS methods that would step, reset, or finalize a statement (StatementSync run/get/all/ iterate, iterator next/return, SQLTagStore run/get/all/iterate) check that flag on the specific statement instead of the db-wide depth. Keep the db-wide IsInUserFunctionCallback check only where the operation is broadly invasive: db.close, db.deserialize, and SQL tag store .clear, all of which finalize tracked statements (potentially the running one). Drop the authorizer scope: SQLite's authorizer rules are stricter than user-defined functions (prepare/exec are forbidden too) and warrant a separate change rather than partial coverage here. Add tests for the legitimate cross-statement and cross-tag-store patterns (now passing), and for db[Symbol.dispose]() being a documented no-op when invoked from a callback. Signed-off-by: Matthew McEachen --- src/node_sqlite.cc | 77 ++++------ src/node_sqlite.h | 29 +++- test/parallel/test-sqlite-custom-functions.js | 141 +++++++++++++++--- 3 files changed, 173 insertions(+), 74 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 928f4fe3b3d298..8dbf8779c8627b 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -2520,12 +2520,8 @@ int DatabaseSync::AuthorizerCallback(void* user_data, NullableSQLiteStringToValue(isolate, param4).ToLocalChecked(), }); - MaybeLocal retval; - { - auto guard = db->EnterUserFunctionCallback(); - retval = callback->Call( - context, Undefined(isolate), js_argv.size(), js_argv.data()); - } + MaybeLocal retval = callback->Call( + context, Undefined(isolate), js_argv.size(), js_argv.data()); Local result; @@ -3041,10 +3037,7 @@ void StatementSync::All(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); THROW_AND_RETURN_ON_BAD_STATE( - env, - stmt->db_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); + env, stmt->IsStepping(), "statement is currently being executed"); Isolate* isolate = env->isolate(); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void()); @@ -3054,6 +3047,7 @@ void StatementSync::All(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3072,10 +3066,7 @@ void StatementSync::Iterate(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); THROW_AND_RETURN_ON_BAD_STATE( - env, - stmt->db_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); + env, stmt->IsStepping(), "statement is currently being executed"); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3100,10 +3091,7 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); THROW_AND_RETURN_ON_BAD_STATE( - env, - stmt->db_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); + env, stmt->IsStepping(), "statement is currently being executed"); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3112,6 +3100,7 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Get(env, stmt->db_.get(), stmt->statement_, @@ -3129,10 +3118,7 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); THROW_AND_RETURN_ON_BAD_STATE( - env, - stmt->db_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); + env, stmt->IsStepping(), "statement is currently being executed"); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3141,6 +3127,7 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Run( env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_) .ToLocal(&result)) { @@ -3386,11 +3373,6 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); - THROW_AND_RETURN_ON_BAD_STATE( - env, - session->database_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3398,6 +3380,9 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsStepping(), "statement is currently being executed"); + uint32_t n_params = args.Length() - 1; int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3410,6 +3395,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Run( env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_) .ToLocal(&result)) { @@ -3424,11 +3410,6 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); - THROW_AND_RETURN_ON_BAD_STATE( - env, - session->database_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3436,6 +3417,9 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsStepping(), "statement is currently being executed"); + uint32_t n_params = args.Length() - 1; int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3464,11 +3448,6 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); - THROW_AND_RETURN_ON_BAD_STATE( - env, - session->database_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3476,6 +3455,9 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsStepping(), "statement is currently being executed"); + uint32_t n_params = args.Length() - 1; Isolate* isolate = env->isolate(); @@ -3491,6 +3473,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); if (StatementExecutionHelper::Get(env, stmt->db_.get(), stmt->statement_, @@ -3508,11 +3491,6 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); - THROW_AND_RETURN_ON_BAD_STATE( - env, - session->database_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); BaseObjectPtr stmt = PrepareStatement(args); @@ -3520,6 +3498,9 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_ON_BAD_STATE( + env, stmt->IsStepping(), "statement is currently being executed"); + uint32_t n_params = args.Length() - 1; Isolate* isolate = env->isolate(); @@ -3535,6 +3516,7 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { } Local result; + auto step = stmt->MarkStepping(); auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3746,10 +3728,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, iter->stmt_->IsFinalized(), "statement has been finalized"); THROW_AND_RETURN_ON_BAD_STATE( - env, - iter->stmt_->db_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); + env, iter->stmt_->IsStepping(), "statement is currently being executed"); Isolate* isolate = env->isolate(); auto iter_template = getLazyIterTemplate(env); @@ -3772,6 +3751,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { iter->statement_reset_generation_ != iter->stmt_->reset_generation_, "iterator was invalidated"); + auto step = iter->stmt_->MarkStepping(); int r = sqlite3_step(iter->stmt_->statement_); if (r != SQLITE_ROW) { CHECK_ERROR_OR_THROW( @@ -3827,10 +3807,7 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, iter->stmt_->IsFinalized(), "statement has been finalized"); THROW_AND_RETURN_ON_BAD_STATE( - env, - iter->stmt_->db_->IsInUserFunctionCallback(), - "database operation is not allowed inside a user-defined function " - "callback"); + env, iter->stmt_->IsStepping(), "statement is currently being executed"); Isolate* isolate = env->isolate(); sqlite3_reset(iter->stmt_->statement_); diff --git a/src/node_sqlite.h b/src/node_sqlite.h index f8f629c0387292..9c672e60337a71 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -221,12 +221,16 @@ class DatabaseSync : public BaseObject { } sqlite3* Connection(); - // SQLite forbids closing the database, finalizing/resetting the running - // statement, or recursively stepping while a user-supplied callback - // (scalar or aggregate function, or authorizer) is on the stack. Wrap - // every such callback with the RAII guard returned by - // EnterUserFunctionCallback(); JS-callable methods that would perform a - // forbidden operation must check IsInUserFunctionCallback() and throw. + // SQLite forbids closing the database while a user-defined scalar or + // aggregate function callback is on the stack. Wrap every such + // callback with the RAII guard returned by EnterUserFunctionCallback(). + // db.close()/deserialize() and SQL tag store .clear() check + // IsInUserFunctionCallback() and refuse to run, since they would + // finalize statements (potentially the running one). Reentry into the + // *running* statement (recursive step, reset, or finalize) is + // detected separately via the per-statement + // StatementSync::IsStepping() flag, which leaves cross-statement use + // (the "lookup" pattern) unaffected. inline auto EnterUserFunctionCallback() { user_function_callback_depth_++; return OnScopeLeave([this]() { user_function_callback_depth_--; }); @@ -298,6 +302,18 @@ class StatementSync : public BaseObject { bool GetCachedColumnNames(v8::LocalVector* keys); void Finalize(); bool IsFinalized(); + bool IsStepping() const { return stepping_; } + + // RAII guard: marks this statement as being stepped while alive. + // JS-callable methods that would step, reset, or finalize this + // statement check IsStepping() and throw — that's the + // sqlite3_step / sqlite3_reset / sqlite3_finalize reentry SQLite + // forbids while the statement's user-defined function callback is + // on the stack. + inline auto MarkStepping() { + stepping_ = true; + return OnScopeLeave([this]() { stepping_ = false; }); + } SET_MEMORY_INFO_NAME(StatementSync) SET_SELF_SIZE(StatementSync) @@ -310,6 +326,7 @@ class StatementSync : public BaseObject { bool use_big_ints_; bool allow_bare_named_params_; bool allow_unknown_named_params_; + bool stepping_ = false; uint64_t reset_generation_ = 0; std::optional> bare_named_params_; inline int ResetStatement(); diff --git a/test/parallel/test-sqlite-custom-functions.js b/test/parallel/test-sqlite-custom-functions.js index d8b94626e951e4..4d887bc63e3820 100644 --- a/test/parallel/test-sqlite-custom-functions.js +++ b/test/parallel/test-sqlite-custom-functions.js @@ -1,5 +1,5 @@ 'use strict'; -const { skipIfSQLiteMissing, mustCall, mustCallAtLeast } = require('../common'); +const { skipIfSQLiteMissing, mustCall } = require('../common'); skipIfSQLiteMissing(); const assert = require('node:assert'); const { DatabaseSync } = require('node:sqlite'); @@ -417,9 +417,9 @@ suite('DatabaseSync.prototype.function()', () => { code: 'ERR_INVALID_STATE', message: /database cannot be closed inside a user-defined function callback/, }; - const reentrantOpError = { + const reentrantStmtError = { code: 'ERR_INVALID_STATE', - message: /database operation is not allowed inside a user-defined function callback/, + message: /statement is currently being executed/, }; function newDbWithRows() { @@ -551,7 +551,7 @@ suite('DatabaseSync.prototype.function()', () => { let stmt; db.function('x', () => stmt.get()); stmt = db.prepare('SELECT x()'); - assert.throws(() => stmt.get(), reentrantOpError); + assert.throws(() => stmt.get(), reentrantStmtError); assert.strictEqual(db.isOpen, true); }); @@ -561,7 +561,7 @@ suite('DatabaseSync.prototype.function()', () => { db.prepare('INSERT INTO t VALUES (1, 10)').run(); let iter; db.function('reenter', mustCall(() => { - assert.throws(() => iter.next(), reentrantOpError); + assert.throws(() => iter.next(), reentrantStmtError); return 0; })); iter = db.prepare('SELECT reenter() FROM t').iterate(); @@ -626,19 +626,6 @@ suite('DatabaseSync.prototype.function()', () => { assert.strictEqual(db.isOpen, true); }); - test('authorizer callback rejects db.close()', () => { - const db = new DatabaseSync(':memory:'); - db.setAuthorizer(mustCallAtLeast(() => { - assert.throws(() => db.close(), reentrantCloseError); - return 0; - }, 1)); - assert.deepStrictEqual( - db.prepare('SELECT 1 AS x').get(), - { __proto__: null, x: 1 }, - ); - assert.strictEqual(db.isOpen, true); - }); - test('SQL tag store clear from a callback is rejected', () => { const db = new DatabaseSync(':memory:'); const sql = db.createTagStore(4); @@ -650,5 +637,123 @@ suite('DatabaseSync.prototype.function()', () => { }); assert.strictEqual(sql.size, 1); }); + + // Regression: a user function may run other prepared statements on + // the same connection (the "lookup" pattern). Only the *currently + // running* statement is unsafe to step/reset/finalize. + test('callback can run a different prepared statement on the same db', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE lookup (id INTEGER PRIMARY KEY, v INTEGER)'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY)'); + db.prepare('INSERT INTO lookup VALUES (1, 100), (2, 200)').run(); + db.prepare('INSERT INTO t VALUES (1), (2)').run(); + const lookup = db.prepare('SELECT v FROM lookup WHERE id = ?'); + db.function('lookup_v', mustCall((id) => lookup.get(id).v, 2)); + assert.deepStrictEqual( + db.prepare('SELECT lookup_v(id) AS v FROM t ORDER BY id').all(), + [ + { __proto__: null, v: 100 }, + { __proto__: null, v: 200 }, + ], + ); + }); + + test('callback can use SQL tag store with different SQL', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)'); + db.prepare('INSERT INTO t VALUES (1, 10), (2, 20)').run(); + const sql = db.createTagStore(4); + db.function('double_v', mustCall((id) => { + return sql.get`SELECT v * 2 AS d FROM t WHERE id = ${id}`.d; + }, 2)); + assert.deepStrictEqual( + db.prepare('SELECT double_v(id) AS d FROM t ORDER BY id').all(), + [ + { __proto__: null, d: 20 }, + { __proto__: null, d: 40 }, + ], + ); + }); + + // Cover IsStepping rejection on every JS-callable statement method. + for (const method of ['all', 'run', 'iterate']) { + test(`stmt.${method}() same-stmt reentry is rejected`, () => { + const db = new DatabaseSync(':memory:'); + let stmt; + db.function('x', mustCall(() => { + assert.throws(() => stmt[method](), reentrantStmtError); + return 0; + })); + stmt = db.prepare('SELECT x()'); + if (method === 'iterate') { + stmt.iterate().next(); + } else { + stmt[method](); + } + }); + } + + test('iter.return() same-stmt reentry is rejected', () => { + const db = new DatabaseSync(':memory:'); + let iter; + db.function('x', mustCall(() => { + assert.throws(() => iter.return(), reentrantStmtError); + return 0; + })); + iter = db.prepare('SELECT x()').iterate(); + iter.next(); + }); + + // Cover IsStepping rejection on every SQLTagStore execution method. + for (const method of ['run', 'get', 'all', 'iterate']) { + test(`sql.${method}\`...\` same-stmt reentry is rejected`, () => { + const db = new DatabaseSync(':memory:'); + const sql = db.createTagStore(4); + db.function('x', mustCall(() => { + // Re-running the same tag returns the cached stmt — which is + // currently stepping — so the entry check throws. + assert.throws(() => sql[method]`SELECT x()`, reentrantStmtError); + return 0; + })); + if (method === 'iterate') { + sql.iterate`SELECT x()`.next(); + } else { + assert.ok(sql[method]`SELECT x()`); + } + }); + } + + test('db.deserialize() from a callback is rejected', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id INTEGER)'); + const snapshot = db.serialize(); + db.function('x', mustCall(() => { + assert.throws(() => db.deserialize(snapshot), { + code: 'ERR_INVALID_STATE', + message: /database operation is not allowed inside a user-defined function callback/, + }); + return 0; + })); + db.prepare('SELECT x()').get(); + assert.strictEqual(db.isOpen, true); + }); + + test('db[Symbol.dispose]() is a no-op when invoked from a callback', () => { + const db = new DatabaseSync(':memory:'); + db.function('do_dispose', mustCall(() => { + db[Symbol.dispose](); + return 1; + })); + // The dispose attempt is silently skipped; the outer query still + // completes and the database stays open. + assert.deepStrictEqual( + db.prepare('SELECT do_dispose() AS x').get(), + { __proto__: null, x: 1 }, + ); + assert.strictEqual(db.isOpen, true); + // Outside of any callback, dispose works normally. + db[Symbol.dispose](); + assert.strictEqual(db.isOpen, false); + }); }); });