Skip to content

Commit df560ee

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 df560ee

3 files changed

Lines changed: 116 additions & 68 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 33 additions & 44 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

@@ -3042,9 +3038,8 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
30423038
env, stmt->IsFinalized(), "statement has been finalized");
30433039
THROW_AND_RETURN_ON_BAD_STATE(
30443040
env,
3045-
stmt->db_->IsInUserFunctionCallback(),
3046-
"database operation is not allowed inside a user-defined function "
3047-
"callback");
3041+
stmt->IsStepping(),
3042+
"statement is currently being executed");
30483043
Isolate* isolate = env->isolate();
30493044
int r = stmt->ResetStatement();
30503045
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
@@ -3054,6 +3049,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
30543049
}
30553050

30563051
Local<Value> result;
3052+
auto step = stmt->MarkStepping();
30573053
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
30583054
if (StatementExecutionHelper::All(env,
30593055
stmt->db_.get(),
@@ -3073,9 +3069,8 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
30733069
env, stmt->IsFinalized(), "statement has been finalized");
30743070
THROW_AND_RETURN_ON_BAD_STATE(
30753071
env,
3076-
stmt->db_->IsInUserFunctionCallback(),
3077-
"database operation is not allowed inside a user-defined function "
3078-
"callback");
3072+
stmt->IsStepping(),
3073+
"statement is currently being executed");
30793074
int r = stmt->ResetStatement();
30803075
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
30813076

@@ -3101,9 +3096,8 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
31013096
env, stmt->IsFinalized(), "statement has been finalized");
31023097
THROW_AND_RETURN_ON_BAD_STATE(
31033098
env,
3104-
stmt->db_->IsInUserFunctionCallback(),
3105-
"database operation is not allowed inside a user-defined function "
3106-
"callback");
3099+
stmt->IsStepping(),
3100+
"statement is currently being executed");
31073101
int r = stmt->ResetStatement();
31083102
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31093103

@@ -3112,6 +3106,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
31123106
}
31133107

31143108
Local<Value> result;
3109+
auto step = stmt->MarkStepping();
31153110
if (StatementExecutionHelper::Get(env,
31163111
stmt->db_.get(),
31173112
stmt->statement_,
@@ -3130,9 +3125,8 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
31303125
env, stmt->IsFinalized(), "statement has been finalized");
31313126
THROW_AND_RETURN_ON_BAD_STATE(
31323127
env,
3133-
stmt->db_->IsInUserFunctionCallback(),
3134-
"database operation is not allowed inside a user-defined function "
3135-
"callback");
3128+
stmt->IsStepping(),
3129+
"statement is currently being executed");
31363130
int r = stmt->ResetStatement();
31373131
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31383132

@@ -3141,6 +3135,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
31413135
}
31423136

31433137
Local<Object> result;
3138+
auto step = stmt->MarkStepping();
31443139
if (StatementExecutionHelper::Run(
31453140
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
31463141
.ToLocal(&result)) {
@@ -3386,18 +3381,16 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
33863381

33873382
THROW_AND_RETURN_ON_BAD_STATE(
33883383
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");
33943384

33953385
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
33963386

33973387
if (!stmt) {
33983388
return;
33993389
}
34003390

3391+
THROW_AND_RETURN_ON_BAD_STATE(
3392+
env, stmt->IsStepping(), "statement is currently being executed");
3393+
34013394
uint32_t n_params = args.Length() - 1;
34023395
int r = stmt->ResetStatement();
34033396
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
@@ -3410,6 +3403,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
34103403
}
34113404

34123405
Local<Object> result;
3406+
auto step = stmt->MarkStepping();
34133407
if (StatementExecutionHelper::Run(
34143408
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
34153409
.ToLocal(&result)) {
@@ -3424,18 +3418,16 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
34243418

34253419
THROW_AND_RETURN_ON_BAD_STATE(
34263420
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");
34323421

34333422
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
34343423

34353424
if (!stmt) {
34363425
return;
34373426
}
34383427

3428+
THROW_AND_RETURN_ON_BAD_STATE(
3429+
env, stmt->IsStepping(), "statement is currently being executed");
3430+
34393431
uint32_t n_params = args.Length() - 1;
34403432
int r = stmt->ResetStatement();
34413433
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
@@ -3464,18 +3456,16 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34643456

34653457
THROW_AND_RETURN_ON_BAD_STATE(
34663458
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");
34723459

34733460
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
34743461

34753462
if (!stmt) {
34763463
return;
34773464
}
34783465

3466+
THROW_AND_RETURN_ON_BAD_STATE(
3467+
env, stmt->IsStepping(), "statement is currently being executed");
3468+
34793469
uint32_t n_params = args.Length() - 1;
34803470
Isolate* isolate = env->isolate();
34813471

@@ -3491,6 +3481,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34913481
}
34923482

34933483
Local<Value> result;
3484+
auto step = stmt->MarkStepping();
34943485
if (StatementExecutionHelper::Get(env,
34953486
stmt->db_.get(),
34963487
stmt->statement_,
@@ -3508,18 +3499,16 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35083499

35093500
THROW_AND_RETURN_ON_BAD_STATE(
35103501
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");
35163502

35173503
BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);
35183504

35193505
if (!stmt) {
35203506
return;
35213507
}
35223508

3509+
THROW_AND_RETURN_ON_BAD_STATE(
3510+
env, stmt->IsStepping(), "statement is currently being executed");
3511+
35233512
uint32_t n_params = args.Length() - 1;
35243513
Isolate* isolate = env->isolate();
35253514

@@ -3535,6 +3524,7 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35353524
}
35363525

35373526
Local<Value> result;
3527+
auto step = stmt->MarkStepping();
35383528
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
35393529
if (StatementExecutionHelper::All(env,
35403530
stmt->db_.get(),
@@ -3747,9 +3737,8 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37473737
env, iter->stmt_->IsFinalized(), "statement has been finalized");
37483738
THROW_AND_RETURN_ON_BAD_STATE(
37493739
env,
3750-
iter->stmt_->db_->IsInUserFunctionCallback(),
3751-
"database operation is not allowed inside a user-defined function "
3752-
"callback");
3740+
iter->stmt_->IsStepping(),
3741+
"statement is currently being executed");
37533742
Isolate* isolate = env->isolate();
37543743

37553744
auto iter_template = getLazyIterTemplate(env);
@@ -3772,6 +3761,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37723761
iter->statement_reset_generation_ != iter->stmt_->reset_generation_,
37733762
"iterator was invalidated");
37743763

3764+
auto step = iter->stmt_->MarkStepping();
37753765
int r = sqlite3_step(iter->stmt_->statement_);
37763766
if (r != SQLITE_ROW) {
37773767
CHECK_ERROR_OR_THROW(
@@ -3828,9 +3818,8 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
38283818
env, iter->stmt_->IsFinalized(), "statement has been finalized");
38293819
THROW_AND_RETURN_ON_BAD_STATE(
38303820
env,
3831-
iter->stmt_->db_->IsInUserFunctionCallback(),
3832-
"database operation is not allowed inside a user-defined function "
3833-
"callback");
3821+
iter->stmt_->IsStepping(),
3822+
"statement is currently being executed");
38343823
Isolate* isolate = env->isolate();
38353824

38363825
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();

‎test/parallel/test-sqlite-custom-functions.js‎

Lines changed: 60 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
'use strict';
2-
const { skipIfSQLiteMissing, mustCall, mustCallAtLeast } = require('../common');
2+
const { skipIfSQLiteMissing, mustCall } = require('../common');
33
skipIfSQLiteMissing();
44
const assert = require('node:assert');
55
const { DatabaseSync } = require('node:sqlite');
@@ -417,9 +417,9 @@ suite('DatabaseSync.prototype.function()', () => {
417417
code: 'ERR_INVALID_STATE',
418418
message: /database cannot be closed inside a user-defined function callback/,
419419
};
420-
const reentrantOpError = {
420+
const reentrantStmtError = {
421421
code: 'ERR_INVALID_STATE',
422-
message: /database operation is not allowed inside a user-defined function callback/,
422+
message: /statement is currently being executed/,
423423
};
424424

425425
function newDbWithRows() {
@@ -551,7 +551,7 @@ suite('DatabaseSync.prototype.function()', () => {
551551
let stmt;
552552
db.function('x', () => stmt.get());
553553
stmt = db.prepare('SELECT x()');
554-
assert.throws(() => stmt.get(), reentrantOpError);
554+
assert.throws(() => stmt.get(), reentrantStmtError);
555555
assert.strictEqual(db.isOpen, true);
556556
});
557557

@@ -561,7 +561,7 @@ suite('DatabaseSync.prototype.function()', () => {
561561
db.prepare('INSERT INTO t VALUES (1, 10)').run();
562562
let iter;
563563
db.function('reenter', mustCall(() => {
564-
assert.throws(() => iter.next(), reentrantOpError);
564+
assert.throws(() => iter.next(), reentrantStmtError);
565565
return 0;
566566
}));
567567
iter = db.prepare('SELECT reenter() FROM t').iterate();
@@ -626,19 +626,6 @@ suite('DatabaseSync.prototype.function()', () => {
626626
assert.strictEqual(db.isOpen, true);
627627
});
628628

629-
test('authorizer callback rejects db.close()', () => {
630-
const db = new DatabaseSync(':memory:');
631-
db.setAuthorizer(mustCallAtLeast(() => {
632-
assert.throws(() => db.close(), reentrantCloseError);
633-
return 0;
634-
}, 1));
635-
assert.deepStrictEqual(
636-
db.prepare('SELECT 1 AS x').get(),
637-
{ __proto__: null, x: 1 },
638-
);
639-
assert.strictEqual(db.isOpen, true);
640-
});
641-
642629
test('SQL tag store clear from a callback is rejected', () => {
643630
const db = new DatabaseSync(':memory:');
644631
const sql = db.createTagStore(4);
@@ -650,5 +637,60 @@ suite('DatabaseSync.prototype.function()', () => {
650637
});
651638
assert.strictEqual(sql.size, 1);
652639
});
640+
641+
// Regression: a user function may run other prepared statements on
642+
// the same connection (the "lookup" pattern). Only the *currently
643+
// running* statement is unsafe to step/reset/finalize.
644+
test('callback can run a different prepared statement on the same db', () => {
645+
const db = new DatabaseSync(':memory:');
646+
db.exec('CREATE TABLE lookup (id INTEGER PRIMARY KEY, v INTEGER)');
647+
db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY)');
648+
db.prepare('INSERT INTO lookup VALUES (1, 100), (2, 200)').run();
649+
db.prepare('INSERT INTO t VALUES (1), (2)').run();
650+
const lookup = db.prepare('SELECT v FROM lookup WHERE id = ?');
651+
db.function('lookup_v', mustCall((id) => lookup.get(id).v, 2));
652+
assert.deepStrictEqual(
653+
db.prepare('SELECT lookup_v(id) AS v FROM t ORDER BY id').all(),
654+
[
655+
{ __proto__: null, v: 100 },
656+
{ __proto__: null, v: 200 },
657+
],
658+
);
659+
});
660+
661+
test('callback can use SQL tag store with different SQL', () => {
662+
const db = new DatabaseSync(':memory:');
663+
db.exec('CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER)');
664+
db.prepare('INSERT INTO t VALUES (1, 10), (2, 20)').run();
665+
const sql = db.createTagStore(4);
666+
db.function('double_v', mustCall((id) => {
667+
return sql.get`SELECT v * 2 AS d FROM t WHERE id = ${id}`.d;
668+
}, 2));
669+
assert.deepStrictEqual(
670+
db.prepare('SELECT double_v(id) AS d FROM t ORDER BY id').all(),
671+
[
672+
{ __proto__: null, d: 20 },
673+
{ __proto__: null, d: 40 },
674+
],
675+
);
676+
});
677+
678+
test('db[Symbol.dispose]() is a no-op when invoked from a callback', () => {
679+
const db = new DatabaseSync(':memory:');
680+
db.function('do_dispose', mustCall(() => {
681+
db[Symbol.dispose]();
682+
return 1;
683+
}));
684+
// The dispose attempt is silently skipped; the outer query still
685+
// completes and the database stays open.
686+
assert.deepStrictEqual(
687+
db.prepare('SELECT do_dispose() AS x').get(),
688+
{ __proto__: null, x: 1 },
689+
);
690+
assert.strictEqual(db.isOpen, true);
691+
// Outside of any callback, dispose works normally.
692+
db[Symbol.dispose]();
693+
assert.strictEqual(db.isOpen, false);
694+
});
653695
});
654696
});

0 commit comments

Comments
 (0)