Skip to content

Commit c02e2c0

Browse files
mceachenmceachen
committed
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 <matthew@photostructure.com>
1 parent 20d7ffa commit c02e2c0

3 files changed

Lines changed: 173 additions & 74 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 27 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -2520,12 +2520,8 @@ int DatabaseSync::AuthorizerCallback(void* user_data,
25202520
NullableSQLiteStringToValue(isolate, param4).ToLocalChecked(),
25212521
});
25222522

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-
}
2523+
MaybeLocal<Value> retval = callback->Call(
2524+
context, Undefined(isolate), js_argv.size(), js_argv.data());
25292525

25302526
Local<Value> result;
25312527

@@ -3041,10 +3037,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
30413037
THROW_AND_RETURN_ON_BAD_STATE(
30423038
env, stmt->IsFinalized(), "statement has been finalized");
30433039
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");
3040+
env, stmt->IsStepping(), "statement is currently being executed");
30483041
Isolate* isolate = env->isolate();
30493042
int r = stmt->ResetStatement();
30503043
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
@@ -3054,6 +3047,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
30543047
}
30553048

30563049
Local<Value> result;
3050+
auto step = stmt->MarkStepping();
30573051
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
30583052
if (StatementExecutionHelper::All(env,
30593053
stmt->db_.get(),
@@ -3072,10 +3066,7 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
30723066
THROW_AND_RETURN_ON_BAD_STATE(
30733067
env, stmt->IsFinalized(), "statement has been finalized");
30743068
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");
3069+
env, stmt->IsStepping(), "statement is currently being executed");
30793070
int r = stmt->ResetStatement();
30803071
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
30813072

@@ -3100,10 +3091,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
31003091
THROW_AND_RETURN_ON_BAD_STATE(
31013092
env, stmt->IsFinalized(), "statement has been finalized");
31023093
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");
3094+
env, stmt->IsStepping(), "statement is currently being executed");
31073095
int r = stmt->ResetStatement();
31083096
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31093097

@@ -3112,6 +3100,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
31123100
}
31133101

31143102
Local<Value> result;
3103+
auto step = stmt->MarkStepping();
31153104
if (StatementExecutionHelper::Get(env,
31163105
stmt->db_.get(),
31173106
stmt->statement_,
@@ -3129,10 +3118,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
31293118
THROW_AND_RETURN_ON_BAD_STATE(
31303119
env, stmt->IsFinalized(), "statement has been finalized");
31313120
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");
3121+
env, stmt->IsStepping(), "statement is currently being executed");
31363122
int r = stmt->ResetStatement();
31373123
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31383124

@@ -3141,6 +3127,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
31413127
}
31423128

31433129
Local<Object> result;
3130+
auto step = stmt->MarkStepping();
31443131
if (StatementExecutionHelper::Run(
31453132
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
31463133
.ToLocal(&result)) {
@@ -3386,18 +3373,16 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
33863373

33873374
THROW_AND_RETURN_ON_BAD_STATE(
33883375
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");
33943376

33953377
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
33963378

33973379
if (!stmt) {
33983380
return;
33993381
}
34003382

3383+
THROW_AND_RETURN_ON_BAD_STATE(
3384+
env, stmt->IsStepping(), "statement is currently being executed");
3385+
34013386
uint32_t n_params = args.Length() - 1;
34023387
int r = stmt->ResetStatement();
34033388
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
@@ -3410,6 +3395,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
34103395
}
34113396

34123397
Local<Object> result;
3398+
auto step = stmt->MarkStepping();
34133399
if (StatementExecutionHelper::Run(
34143400
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
34153401
.ToLocal(&result)) {
@@ -3424,18 +3410,16 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
34243410

34253411
THROW_AND_RETURN_ON_BAD_STATE(
34263412
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");
34323413

34333414
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
34343415

34353416
if (!stmt) {
34363417
return;
34373418
}
34383419

3420+
THROW_AND_RETURN_ON_BAD_STATE(
3421+
env, stmt->IsStepping(), "statement is currently being executed");
3422+
34393423
uint32_t n_params = args.Length() - 1;
34403424
int r = stmt->ResetStatement();
34413425
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
@@ -3464,18 +3448,16 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34643448

34653449
THROW_AND_RETURN_ON_BAD_STATE(
34663450
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");
34723451

34733452
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
34743453

34753454
if (!stmt) {
34763455
return;
34773456
}
34783457

3458+
THROW_AND_RETURN_ON_BAD_STATE(
3459+
env, stmt->IsStepping(), "statement is currently being executed");
3460+
34793461
uint32_t n_params = args.Length() - 1;
34803462
Isolate* isolate = env->isolate();
34813463

@@ -3491,6 +3473,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34913473
}
34923474

34933475
Local<Value> result;
3476+
auto step = stmt->MarkStepping();
34943477
if (StatementExecutionHelper::Get(env,
34953478
stmt->db_.get(),
34963479
stmt->statement_,
@@ -3508,18 +3491,16 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35083491

35093492
THROW_AND_RETURN_ON_BAD_STATE(
35103493
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");
35163494

35173495
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
35183496

35193497
if (!stmt) {
35203498
return;
35213499
}
35223500

3501+
THROW_AND_RETURN_ON_BAD_STATE(
3502+
env, stmt->IsStepping(), "statement is currently being executed");
3503+
35233504
uint32_t n_params = args.Length() - 1;
35243505
Isolate* isolate = env->isolate();
35253506

@@ -3535,6 +3516,7 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35353516
}
35363517

35373518
Local<Value> result;
3519+
auto step = stmt->MarkStepping();
35383520
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
35393521
if (StatementExecutionHelper::All(env,
35403522
stmt->db_.get(),
@@ -3746,10 +3728,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37463728
THROW_AND_RETURN_ON_BAD_STATE(
37473729
env, iter->stmt_->IsFinalized(), "statement has been finalized");
37483730
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");
3731+
env, iter->stmt_->IsStepping(), "statement is currently being executed");
37533732
Isolate* isolate = env->isolate();
37543733

37553734
auto iter_template = getLazyIterTemplate(env);
@@ -3772,6 +3751,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37723751
iter->statement_reset_generation_ != iter->stmt_->reset_generation_,
37733752
"iterator was invalidated");
37743753

3754+
auto step = iter->stmt_->MarkStepping();
37753755
int r = sqlite3_step(iter->stmt_->statement_);
37763756
if (r != SQLITE_ROW) {
37773757
CHECK_ERROR_OR_THROW(
@@ -3827,10 +3807,7 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
38273807
THROW_AND_RETURN_ON_BAD_STATE(
38283808
env, iter->stmt_->IsFinalized(), "statement has been finalized");
38293809
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");
3810+
env, iter->stmt_->IsStepping(), "statement is currently being executed");
38343811
Isolate* isolate = env->isolate();
38353812

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

‎src/node_sqlite.h‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -221,12 +221,16 @@ 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.
224+
// SQLite forbids closing the database while a user-defined scalar or
225+
// aggregate function callback is on the stack. Wrap every such
226+
// callback with the RAII guard returned by EnterUserFunctionCallback().
227+
// db.close()/deserialize() and SQL tag store .clear() check
228+
// IsInUserFunctionCallback() and refuse to run, since they would
229+
// finalize statements (potentially the running one). Reentry into the
230+
// *running* statement (recursive step, reset, or finalize) is
231+
// detected separately via the per-statement
232+
// StatementSync::IsStepping() flag, which leaves cross-statement use
233+
// (the "lookup" pattern) unaffected.
230234
inline auto EnterUserFunctionCallback() {
231235
user_function_callback_depth_++;
232236
return OnScopeLeave([this]() { user_function_callback_depth_--; });
@@ -298,6 +302,18 @@ class StatementSync : public BaseObject {
298302
bool GetCachedColumnNames(v8::LocalVector<v8::Name>* keys);
299303
void Finalize();
300304
bool IsFinalized();
305+
bool IsStepping() const { return stepping_; }
306+
307+
// RAII guard: marks this statement as being stepped while alive.
308+
// JS-callable methods that would step, reset, or finalize this
309+
// statement check IsStepping() and throw — that's the
310+
// sqlite3_step / sqlite3_reset / sqlite3_finalize reentry SQLite
311+
// forbids while the statement's user-defined function callback is
312+
// on the stack.
313+
inline auto MarkStepping() {
314+
stepping_ = true;
315+
return OnScopeLeave([this]() { stepping_ = false; });
316+
}
301317

302318
SET_MEMORY_INFO_NAME(StatementSync)
303319
SET_SELF_SIZE(StatementSync)
@@ -310,6 +326,7 @@ class StatementSync : public BaseObject {
310326
bool use_big_ints_;
311327
bool allow_bare_named_params_;
312328
bool allow_unknown_named_params_;
329+
bool stepping_ = false;
313330
uint64_t reset_generation_ = 0;
314331
std::optional<std::map<std::string, std::string>> bare_named_params_;
315332
inline int ResetStatement();

0 commit comments

Comments
 (0)