Skip to content

Commit c6ce251

Browse files
mceachenclaude
mceachen
and
claude
committed
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(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 14cd548 commit c6ce251

3 files changed

Lines changed: 300 additions & 85 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 87 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -386,13 +386,16 @@ class CustomAggregate {
386386
}
387387

388388
Local<Value> ret;
389-
if (!(self->*mptr)
390-
.Get(isolate)
391-
->Call(env->context(), recv, argc + 1, js_argv.data())
392-
.ToLocal(&ret)) {
393-
self->db_->SetIgnoreNextSQLiteError(true);
394-
sqlite3_result_error(ctx, "", 0);
395-
return;
389+
{
390+
auto guard = self->db_->EnterUserFunctionCallback();
391+
if (!(self->*mptr)
392+
.Get(isolate)
393+
->Call(env->context(), recv, argc + 1, js_argv.data())
394+
.ToLocal(&ret)) {
395+
self->db_->SetIgnoreNextSQLiteError(true);
396+
sqlite3_result_error(ctx, "", 0);
397+
return;
398+
}
396399
}
397400

398401
agg->value.Reset(isolate, ret);
@@ -422,6 +425,7 @@ class CustomAggregate {
422425
Local<Function>::New(env->isolate(), self->result_fn_);
423426
Local<Value> js_arg[] = {Local<Value>::New(isolate, agg->value)};
424427

428+
auto guard = self->db_->EnterUserFunctionCallback();
425429
if (!fn->Call(env->context(), Null(isolate), 1, js_arg)
426430
.ToLocal(&result)) {
427431
self->db_->SetIgnoreNextSQLiteError(true);
@@ -455,6 +459,7 @@ class CustomAggregate {
455459
Local<Value> start_v = Local<Value>::New(isolate, start_);
456460
if (start_v->IsFunction()) {
457461
auto fn = start_v.As<Function>();
462+
auto guard = db_->EnterUserFunctionCallback();
458463
MaybeLocal<Value> retval =
459464
fn->Call(env_->context(), Null(isolate), 0, nullptr);
460465
if (!retval.ToLocal(&start_v)) {
@@ -698,6 +703,7 @@ void UserDefinedFunction::xFunc(sqlite3_context* ctx,
698703
js_argv.emplace_back(local);
699704
}
700705

706+
auto guard = self->db_->EnterUserFunctionCallback();
701707
MaybeLocal<Value> retval =
702708
fn->Call(env->context(), recv, argc, js_argv.data());
703709
Local<Value> result;
@@ -1420,6 +1426,10 @@ void DatabaseSync::Close(const FunctionCallbackInfo<Value>& args) {
14201426
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
14211427
Environment* env = Environment::GetCurrent(args);
14221428
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
1429+
THROW_AND_RETURN_ON_BAD_STATE(
1430+
env,
1431+
db->IsInUserFunctionCallback(),
1432+
"database cannot be closed inside a user-defined function callback");
14231433
db->FinalizeStatements();
14241434
db->DeleteSessions();
14251435
int r = sqlite3_close_v2(db->connection_);
@@ -1808,6 +1818,11 @@ void DatabaseSync::Deserialize(const FunctionCallbackInfo<Value>& args) {
18081818
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
18091819
Environment* env = Environment::GetCurrent(args);
18101820
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
1821+
THROW_AND_RETURN_ON_BAD_STATE(
1822+
env,
1823+
db->IsInUserFunctionCallback(),
1824+
"database operation is not allowed inside a user-defined function "
1825+
"callback");
18111826

18121827
if (!args[0]->IsUint8Array()) {
18131828
THROW_ERR_INVALID_ARG_TYPE(env->isolate(),
@@ -2505,8 +2520,12 @@ int DatabaseSync::AuthorizerCallback(void* user_data,
25052520
NullableSQLiteStringToValue(isolate, param4).ToLocalChecked(),
25062521
});
25072522

2508-
MaybeLocal<Value> retval = callback->Call(
2509-
context, Undefined(isolate), js_argv.size(), js_argv.data());
2523+
MaybeLocal<Value> retval;
2524+
{
2525+
auto guard = db->EnterUserFunctionCallback();
2526+
retval = callback->Call(
2527+
context, Undefined(isolate), js_argv.size(), js_argv.data());
2528+
}
25102529

25112530
Local<Value> result;
25122531

@@ -2576,10 +2595,6 @@ void StatementSync::Finalize() {
25762595
if (statement_ == nullptr) {
25772596
return;
25782597
}
2579-
if (stepping_) {
2580-
finalize_pending_ = true;
2581-
return;
2582-
}
25832598
sqlite3_finalize(statement_);
25842599
statement_ = nullptr;
25852600
InvalidateColumnNameCache();
@@ -2905,10 +2920,6 @@ MaybeLocal<Object> StatementExecutionHelper::Run(Environment* env,
29052920
EscapableHandleScope scope(isolate);
29062921
sqlite3_step(stmt);
29072922
int r = sqlite3_reset(stmt);
2908-
if (db->Connection() == nullptr) {
2909-
THROW_ERR_INVALID_STATE(env, "database connection was closed mid-query");
2910-
return MaybeLocal<Object>();
2911-
}
29122923
CHECK_ERROR_OR_THROW(isolate, db, r, SQLITE_OK, MaybeLocal<Object>());
29132924

29142925
sqlite3_int64 last_insert_rowid = sqlite3_last_insert_rowid(db->Connection());
@@ -3029,6 +3040,11 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
30293040
Environment* env = Environment::GetCurrent(args);
30303041
THROW_AND_RETURN_ON_BAD_STATE(
30313042
env, stmt->IsFinalized(), "statement has been finalized");
3043+
THROW_AND_RETURN_ON_BAD_STATE(
3044+
env,
3045+
stmt->db_->IsInUserFunctionCallback(),
3046+
"database operation is not allowed inside a user-defined function "
3047+
"callback");
30323048
Isolate* isolate = env->isolate();
30333049
int r = stmt->ResetStatement();
30343050
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
@@ -3038,7 +3054,6 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
30383054
}
30393055

30403056
Local<Value> result;
3041-
auto step = stmt->MarkStepping();
30423057
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
30433058
if (StatementExecutionHelper::All(env,
30443059
stmt->db_.get(),
@@ -3056,6 +3071,11 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
30563071
Environment* env = Environment::GetCurrent(args);
30573072
THROW_AND_RETURN_ON_BAD_STATE(
30583073
env, stmt->IsFinalized(), "statement has been finalized");
3074+
THROW_AND_RETURN_ON_BAD_STATE(
3075+
env,
3076+
stmt->db_->IsInUserFunctionCallback(),
3077+
"database operation is not allowed inside a user-defined function "
3078+
"callback");
30593079
int r = stmt->ResetStatement();
30603080
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
30613081

@@ -3079,6 +3099,11 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
30793099
Environment* env = Environment::GetCurrent(args);
30803100
THROW_AND_RETURN_ON_BAD_STATE(
30813101
env, stmt->IsFinalized(), "statement has been finalized");
3102+
THROW_AND_RETURN_ON_BAD_STATE(
3103+
env,
3104+
stmt->db_->IsInUserFunctionCallback(),
3105+
"database operation is not allowed inside a user-defined function "
3106+
"callback");
30823107
int r = stmt->ResetStatement();
30833108
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
30843109

@@ -3087,7 +3112,6 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
30873112
}
30883113

30893114
Local<Value> result;
3090-
auto step = stmt->MarkStepping();
30913115
if (StatementExecutionHelper::Get(env,
30923116
stmt->db_.get(),
30933117
stmt->statement_,
@@ -3104,6 +3128,11 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
31043128
Environment* env = Environment::GetCurrent(args);
31053129
THROW_AND_RETURN_ON_BAD_STATE(
31063130
env, stmt->IsFinalized(), "statement has been finalized");
3131+
THROW_AND_RETURN_ON_BAD_STATE(
3132+
env,
3133+
stmt->db_->IsInUserFunctionCallback(),
3134+
"database operation is not allowed inside a user-defined function "
3135+
"callback");
31073136
int r = stmt->ResetStatement();
31083137
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31093138

@@ -3112,7 +3141,6 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
31123141
}
31133142

31143143
Local<Object> result;
3115-
auto step = stmt->MarkStepping();
31163144
if (StatementExecutionHelper::Run(
31173145
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
31183146
.ToLocal(&result)) {
@@ -3358,6 +3386,11 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
33583386

33593387
THROW_AND_RETURN_ON_BAD_STATE(
33603388
env, !session->database_->IsOpen(), "database is not open");
3389+
THROW_AND_RETURN_ON_BAD_STATE(
3390+
env,
3391+
session->database_->IsInUserFunctionCallback(),
3392+
"database operation is not allowed inside a user-defined function "
3393+
"callback");
33613394

33623395
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
33633396

@@ -3366,7 +3399,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
33663399
}
33673400

33683401
uint32_t n_params = args.Length() - 1;
3369-
int r = sqlite3_reset(stmt->statement_);
3402+
int r = stmt->ResetStatement();
33703403
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
33713404
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
33723405
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
@@ -3377,7 +3410,6 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
33773410
}
33783411

33793412
Local<Object> result;
3380-
auto step = stmt->MarkStepping();
33813413
if (StatementExecutionHelper::Run(
33823414
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
33833415
.ToLocal(&result)) {
@@ -3392,6 +3424,11 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
33923424

33933425
THROW_AND_RETURN_ON_BAD_STATE(
33943426
env, !session->database_->IsOpen(), "database is not open");
3427+
THROW_AND_RETURN_ON_BAD_STATE(
3428+
env,
3429+
session->database_->IsInUserFunctionCallback(),
3430+
"database operation is not allowed inside a user-defined function "
3431+
"callback");
33953432

33963433
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
33973434

@@ -3400,7 +3437,7 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
34003437
}
34013438

34023439
uint32_t n_params = args.Length() - 1;
3403-
int r = sqlite3_reset(stmt->statement_);
3440+
int r = stmt->ResetStatement();
34043441
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
34053442
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
34063443
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
@@ -3427,6 +3464,11 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34273464

34283465
THROW_AND_RETURN_ON_BAD_STATE(
34293466
env, !session->database_->IsOpen(), "database is not open");
3467+
THROW_AND_RETURN_ON_BAD_STATE(
3468+
env,
3469+
session->database_->IsInUserFunctionCallback(),
3470+
"database operation is not allowed inside a user-defined function "
3471+
"callback");
34303472

34313473
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
34323474

@@ -3437,7 +3479,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34373479
uint32_t n_params = args.Length() - 1;
34383480
Isolate* isolate = env->isolate();
34393481

3440-
int r = sqlite3_reset(stmt->statement_);
3482+
int r = stmt->ResetStatement();
34413483
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
34423484

34433485
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
@@ -3449,7 +3491,6 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34493491
}
34503492

34513493
Local<Value> result;
3452-
auto step = stmt->MarkStepping();
34533494
if (StatementExecutionHelper::Get(env,
34543495
stmt->db_.get(),
34553496
stmt->statement_,
@@ -3467,6 +3508,11 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
34673508

34683509
THROW_AND_RETURN_ON_BAD_STATE(
34693510
env, !session->database_->IsOpen(), "database is not open");
3511+
THROW_AND_RETURN_ON_BAD_STATE(
3512+
env,
3513+
session->database_->IsInUserFunctionCallback(),
3514+
"database operation is not allowed inside a user-defined function "
3515+
"callback");
34703516

34713517
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
34723518

@@ -3477,7 +3523,7 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
34773523
uint32_t n_params = args.Length() - 1;
34783524
Isolate* isolate = env->isolate();
34793525

3480-
int r = sqlite3_reset(stmt->statement_);
3526+
int r = stmt->ResetStatement();
34813527
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
34823528

34833529
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
@@ -3489,7 +3535,6 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
34893535
}
34903536

34913537
Local<Value> result;
3492-
auto step = stmt->MarkStepping();
34933538
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
34943539
if (StatementExecutionHelper::All(env,
34953540
stmt->db_.get(),
@@ -3504,6 +3549,11 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35043549
void SQLTagStore::Clear(const FunctionCallbackInfo<Value>& args) {
35053550
SQLTagStore* store;
35063551
ASSIGN_OR_RETURN_UNWRAP(&store, args.This());
3552+
Environment* env = Environment::GetCurrent(args);
3553+
THROW_AND_RETURN_ON_BAD_STATE(
3554+
env,
3555+
store->database_->IsInUserFunctionCallback(),
3556+
"tag store cannot be cleared inside a user-defined function callback");
35073557
store->sql_tags_.Clear();
35083558
}
35093559

@@ -3695,6 +3745,11 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
36953745
Environment* env = Environment::GetCurrent(args);
36963746
THROW_AND_RETURN_ON_BAD_STATE(
36973747
env, iter->stmt_->IsFinalized(), "statement has been finalized");
3748+
THROW_AND_RETURN_ON_BAD_STATE(
3749+
env,
3750+
iter->stmt_->db_->IsInUserFunctionCallback(),
3751+
"database operation is not allowed inside a user-defined function "
3752+
"callback");
36983753
Isolate* isolate = env->isolate();
36993754

37003755
auto iter_template = getLazyIterTemplate(env);
@@ -3717,7 +3772,6 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37173772
iter->statement_reset_generation_ != iter->stmt_->reset_generation_,
37183773
"iterator was invalidated");
37193774

3720-
auto step = iter->stmt_->MarkStepping();
37213775
int r = sqlite3_step(iter->stmt_->statement_);
37223776
if (r != SQLITE_ROW) {
37233777
CHECK_ERROR_OR_THROW(
@@ -3772,6 +3826,11 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
37723826
Environment* env = Environment::GetCurrent(args);
37733827
THROW_AND_RETURN_ON_BAD_STATE(
37743828
env, iter->stmt_->IsFinalized(), "statement has been finalized");
3829+
THROW_AND_RETURN_ON_BAD_STATE(
3830+
env,
3831+
iter->stmt_->db_->IsInUserFunctionCallback(),
3832+
"database operation is not allowed inside a user-defined function "
3833+
"callback");
37753834
Isolate* isolate = env->isolate();
37763835

37773836
sqlite3_reset(iter->stmt_->statement_);

‎src/node_sqlite.h‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,20 @@ class DatabaseSync : public BaseObject {
221221
}
222222
sqlite3* Connection();
223223

224+
// SQLite forbids closing the database, finalizing/resetting the running
225+
// statement, or recursively stepping while a user-supplied callback
226+
// (scalar or aggregate function, or authorizer) is on the stack. Wrap
227+
// every such callback with the RAII guard returned by
228+
// EnterUserFunctionCallback(); JS-callable methods that would perform a
229+
// forbidden operation must check IsInUserFunctionCallback() and throw.
230+
inline auto EnterUserFunctionCallback() {
231+
user_function_callback_depth_++;
232+
return OnScopeLeave([this]() { user_function_callback_depth_--; });
233+
}
234+
bool IsInUserFunctionCallback() const {
235+
return user_function_callback_depth_ > 0;
236+
}
237+
224238
// In some situations, such as when using custom functions, it is possible
225239
// that SQLite reports an error while JavaScript already has a pending
226240
// exception. In this case, the SQLite error should be ignored. These methods
@@ -241,6 +255,7 @@ class DatabaseSync : public BaseObject {
241255
bool enable_load_extension_;
242256
sqlite3* connection_;
243257
bool ignore_next_sqlite_error_;
258+
int user_function_callback_depth_ = 0;
244259

245260
std::set<BackupJob*> backups_;
246261
std::set<sqlite3_session*> sessions_;
@@ -284,19 +299,6 @@ class StatementSync : public BaseObject {
284299
void Finalize();
285300
bool IsFinalized();
286301

287-
// RAII guard: defer Finalize() if called while sqlite3_step is on the
288-
// stack for this statement. Wrap every sqlite3_step caller.
289-
inline auto MarkStepping() {
290-
stepping_ = true;
291-
return OnScopeLeave([this]() {
292-
stepping_ = false;
293-
if (finalize_pending_) {
294-
finalize_pending_ = false;
295-
Finalize();
296-
}
297-
});
298-
}
299-
300302
SET_MEMORY_INFO_NAME(StatementSync)
301303
SET_SELF_SIZE(StatementSync)
302304

@@ -308,8 +310,6 @@ class StatementSync : public BaseObject {
308310
bool use_big_ints_;
309311
bool allow_bare_named_params_;
310312
bool allow_unknown_named_params_;
311-
bool stepping_ = false;
312-
bool finalize_pending_ = false;
313313
uint64_t reset_generation_ = 0;
314314
std::optional<std::map<std::string, std::string>> bare_named_params_;
315315
inline int ResetStatement();

0 commit comments

Comments
 (0)