Skip to content

Commit 986eb0e

Browse files
TrevorBurnhamTrevorBurnham
committed
sqlite: reject reentry into a running statement
SQLite forbids stepping, resetting, or finalizing a statement while that statement's own user-defined function callback is on the stack. The callback depth added in 5cef767 is tracked per database, so it cannot tell reentry into the running statement apart from the common pattern of querying a different statement from a callback. Mark the statement being executed and reject step, reset, and finalize on that statement with ERR_INVALID_STATE. Previously a reentrant iterator.next() silently consumed rows from the iteration in progress, and a recursive get() failed with a V8 stack overflow instead of reporting the constraint. close() and [Symbol.dispose]() are covered too, since finalizing mid-step frees the running virtual machine. Statements other than the running one are unaffected. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: claude:opus-5
1 parent e2d7b34 commit 986eb0e

3 files changed

Lines changed: 256 additions & 0 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2661,12 +2661,17 @@ void StatementSync::Close(const FunctionCallbackInfo<Value>& args) {
26612661
Environment* env = Environment::GetCurrent(args);
26622662
THROW_AND_RETURN_ON_BAD_STATE(
26632663
env, stmt->IsFinalized(), "statement has been finalized");
2664+
THROW_AND_RETURN_ON_BAD_STATE(
2665+
env, stmt->IsStepping(), "statement is currently being executed");
26642666
stmt->Close();
26652667
}
26662668

26672669
void StatementSync::Dispose(const FunctionCallbackInfo<Value>& args) {
26682670
StatementSync* stmt;
26692671
ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This());
2672+
Environment* env = Environment::GetCurrent(args);
2673+
THROW_AND_RETURN_ON_BAD_STATE(
2674+
env, stmt->IsStepping(), "statement is currently being executed");
26702675
stmt->Close();
26712676
}
26722677

@@ -3111,7 +3116,10 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
31113116
Environment* env = Environment::GetCurrent(args);
31123117
THROW_AND_RETURN_ON_BAD_STATE(
31133118
env, stmt->IsFinalized(), "statement has been finalized");
3119+
THROW_AND_RETURN_ON_BAD_STATE(
3120+
env, stmt->IsStepping(), "statement is currently being executed");
31143121
Isolate* isolate = env->isolate();
3122+
auto stepping = stmt->MarkStepping();
31153123
int r = stmt->ResetStatement();
31163124
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
31173125

@@ -3138,6 +3146,8 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
31383146
Environment* env = Environment::GetCurrent(args);
31393147
THROW_AND_RETURN_ON_BAD_STATE(
31403148
env, stmt->IsFinalized(), "statement has been finalized");
3149+
THROW_AND_RETURN_ON_BAD_STATE(
3150+
env, stmt->IsStepping(), "statement is currently being executed");
31413151
int r = stmt->ResetStatement();
31423152
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31433153

@@ -3161,6 +3171,9 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
31613171
Environment* env = Environment::GetCurrent(args);
31623172
THROW_AND_RETURN_ON_BAD_STATE(
31633173
env, stmt->IsFinalized(), "statement has been finalized");
3174+
THROW_AND_RETURN_ON_BAD_STATE(
3175+
env, stmt->IsStepping(), "statement is currently being executed");
3176+
auto stepping = stmt->MarkStepping();
31643177
int r = stmt->ResetStatement();
31653178
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31663179

@@ -3185,6 +3198,9 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
31853198
Environment* env = Environment::GetCurrent(args);
31863199
THROW_AND_RETURN_ON_BAD_STATE(
31873200
env, stmt->IsFinalized(), "statement has been finalized");
3201+
THROW_AND_RETURN_ON_BAD_STATE(
3202+
env, stmt->IsStepping(), "statement is currently being executed");
3203+
auto stepping = stmt->MarkStepping();
31883204
int r = stmt->ResetStatement();
31893205
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
31903206

@@ -3474,6 +3490,9 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
34743490
return;
34753491
}
34763492

3493+
THROW_AND_RETURN_ON_BAD_STATE(
3494+
env, stmt->IsStepping(), "statement is currently being executed");
3495+
auto stepping = stmt->MarkStepping();
34773496
if (!ResetAndBindStatement(env, stmt.get(), args)) {
34783497
return;
34793498
}
@@ -3500,6 +3519,8 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
35003519
return;
35013520
}
35023521

3522+
THROW_AND_RETURN_ON_BAD_STATE(
3523+
env, stmt->IsStepping(), "statement is currently being executed");
35033524
if (!ResetAndBindStatement(env, stmt.get(), args)) {
35043525
return;
35053526
}
@@ -3528,6 +3549,9 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
35283549
return;
35293550
}
35303551

3552+
THROW_AND_RETURN_ON_BAD_STATE(
3553+
env, stmt->IsStepping(), "statement is currently being executed");
3554+
auto stepping = stmt->MarkStepping();
35313555
if (!ResetAndBindStatement(env, stmt.get(), args)) {
35323556
return;
35333557
}
@@ -3557,6 +3581,9 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35573581
return;
35583582
}
35593583

3584+
THROW_AND_RETURN_ON_BAD_STATE(
3585+
env, stmt->IsStepping(), "statement is currently being executed");
3586+
auto stepping = stmt->MarkStepping();
35603587
if (!ResetAndBindStatement(env, stmt.get(), args)) {
35613588
return;
35623589
}
@@ -3769,6 +3796,8 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37693796
Environment* env = Environment::GetCurrent(args);
37703797
THROW_AND_RETURN_ON_BAD_STATE(
37713798
env, iter->stmt_->IsFinalized(), "statement has been finalized");
3799+
THROW_AND_RETURN_ON_BAD_STATE(
3800+
env, iter->stmt_->IsStepping(), "statement is currently being executed");
37723801
Isolate* isolate = env->isolate();
37733802

37743803
auto iter_template = getLazyIterTemplate(env);
@@ -3791,6 +3820,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37913820
iter->statement_reset_generation_ != iter->stmt_->reset_generation_,
37923821
"iterator was invalidated");
37933822

3823+
auto stepping = iter->stmt_->MarkStepping();
37943824
int r = sqlite3_step(iter->stmt_->statement_);
37953825
if (r != SQLITE_ROW) {
37963826
CHECK_ERROR_OR_THROW(
@@ -3846,6 +3876,8 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
38463876
Environment* env = Environment::GetCurrent(args);
38473877
THROW_AND_RETURN_ON_BAD_STATE(
38483878
env, iter->stmt_->IsFinalized(), "statement has been finalized");
3879+
THROW_AND_RETURN_ON_BAD_STATE(
3880+
env, iter->stmt_->IsStepping(), "statement is currently being executed");
38493881
Isolate* isolate = env->isolate();
38503882

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

‎src/node_sqlite.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,19 @@ class StatementSync : public BaseObject {
291291
bool GetCachedColumnNames(v8::LocalVector<v8::Name>* keys);
292292
void Finalize();
293293
bool IsFinalized();
294+
bool IsStepping() const { return stepping_; }
295+
296+
// SQLite forbids stepping, resetting, or finalizing a statement while that
297+
// same statement's user-defined function callback is on the stack. The
298+
// callback depth tracked by DatabaseSync is per-database, so it cannot
299+
// distinguish reentry into the running statement from the common pattern of
300+
// querying a *different* statement from a callback. This flag marks the
301+
// statement that is currently being stepped so that only the former is
302+
// rejected.
303+
inline auto MarkStepping() {
304+
stepping_ = true;
305+
return OnScopeLeave([this]() { stepping_ = false; });
306+
}
294307

295308
SET_MEMORY_INFO_NAME(StatementSync)
296309
SET_SELF_SIZE(StatementSync)
@@ -304,6 +317,7 @@ class StatementSync : public BaseObject {
304317
bool use_big_ints_;
305318
bool allow_bare_named_params_;
306319
bool allow_unknown_named_params_;
320+
bool stepping_ = false;
307321
uint64_t reset_generation_ = 0;
308322
std::optional<std::map<std::string, std::string>> bare_named_params_;
309323
inline int ResetStatement();

‎test/parallel/test-sqlite-udf-statement-reentry.js‎

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
'use strict';
2+
3+
const { skipIfSQLiteMissing, mustCall } = require('../common');
4+
skipIfSQLiteMissing();
5+
const assert = require('node:assert');
6+
const { suite, test } = require('node:test');
7+
const { DatabaseSync } = require('node:sqlite');
8+
9+
const reentryError = {
10+
code: 'ERR_INVALID_STATE',
11+
message: 'statement is currently being executed',
12+
};
13+
14+
function newDbWithRows() {
15+
const db = new DatabaseSync(':memory:');
16+
db.exec(`
17+
CREATE TABLE data (value INTEGER);
18+
INSERT INTO data VALUES (1), (2), (3);
19+
`);
20+
return db;
21+
}
22+
23+
suite('reentry into the running statement is rejected', () => {
24+
for (const method of ['all', 'get', 'run']) {
25+
test(`statement.${method}() from its own UDF`, () => {
26+
const db = newDbWithRows();
27+
let statement;
28+
db.function('reenter', mustCall((value) => {
29+
assert.throws(() => statement[method](), reentryError);
30+
return value;
31+
}));
32+
33+
statement = db.prepare('SELECT reenter(value) AS value FROM data LIMIT 1');
34+
assert.deepStrictEqual(statement.get(), { __proto__: null, value: 1 });
35+
assert.strictEqual(db.isOpen, true);
36+
});
37+
}
38+
39+
test('iterator next() from its own UDF', () => {
40+
const db = newDbWithRows();
41+
let iterator;
42+
db.function('reenter', mustCall((value) => {
43+
assert.throws(() => iterator.next(), reentryError);
44+
return value;
45+
}, 3));
46+
47+
iterator = db.prepare('SELECT reenter(value) AS value FROM data').iterate();
48+
assert.deepStrictEqual([...iterator].map((row) => row.value), [1, 2, 3]);
49+
assert.strictEqual(db.isOpen, true);
50+
});
51+
52+
test('iterator return() from its own UDF', () => {
53+
const db = newDbWithRows();
54+
let iterator;
55+
db.function('reenter', mustCall((value) => {
56+
assert.throws(() => iterator.return(), reentryError);
57+
return value;
58+
}));
59+
60+
iterator = db.prepare('SELECT reenter(value) AS value FROM data').iterate();
61+
assert.strictEqual(iterator.next().done, false);
62+
iterator.return();
63+
assert.strictEqual(db.isOpen, true);
64+
});
65+
66+
test('recursive get() reports the reentry rather than overflowing the stack',
67+
() => {
68+
const db = new DatabaseSync(':memory:');
69+
let statement;
70+
db.function('reenter', mustCall(() => {
71+
assert.throws(() => statement.get(), reentryError);
72+
return 1;
73+
}));
74+
75+
statement = db.prepare('SELECT reenter() AS value');
76+
assert.deepStrictEqual(statement.get(), { __proto__: null, value: 1 });
77+
});
78+
79+
test('statement.close() from its own UDF', () => {
80+
const db = newDbWithRows();
81+
let statement;
82+
db.function('reenter', mustCall((value) => {
83+
assert.throws(() => statement.close(), reentryError);
84+
return value;
85+
}));
86+
87+
statement = db.prepare('SELECT reenter(value) AS value FROM data LIMIT 1');
88+
assert.deepStrictEqual(statement.get(), { __proto__: null, value: 1 });
89+
statement.close();
90+
});
91+
92+
test('statement[Symbol.dispose]() from its own UDF', () => {
93+
const db = newDbWithRows();
94+
let statement;
95+
db.function('reenter', mustCall((value) => {
96+
assert.throws(() => statement[Symbol.dispose](), reentryError);
97+
return value;
98+
}));
99+
100+
statement = db.prepare('SELECT reenter(value) AS value FROM data LIMIT 1');
101+
assert.deepStrictEqual(statement.get(), { __proto__: null, value: 1 });
102+
statement[Symbol.dispose]();
103+
});
104+
105+
test('statement is usable again after the callback returns', () => {
106+
const db = newDbWithRows();
107+
let statement;
108+
db.function('reenter', mustCall((value) => {
109+
assert.throws(() => statement.all(), reentryError);
110+
return value;
111+
}, 2));
112+
113+
statement = db.prepare('SELECT reenter(value) AS value FROM data LIMIT 1');
114+
assert.deepStrictEqual(statement.all(), [{ __proto__: null, value: 1 }]);
115+
assert.deepStrictEqual(statement.all(), [{ __proto__: null, value: 1 }]);
116+
});
117+
});
118+
119+
suite('a different statement remains usable from a callback', () => {
120+
test('the lookup pattern still works', () => {
121+
const db = newDbWithRows();
122+
db.exec('CREATE TABLE names (value INTEGER, name TEXT);' +
123+
"INSERT INTO names VALUES (1, 'one'), (2, 'two'), (3, 'three');");
124+
const lookup = db.prepare('SELECT name FROM names WHERE value = ?');
125+
126+
db.function('name_of', mustCall((value) => lookup.get(value).name, 3));
127+
128+
assert.deepStrictEqual(
129+
db.prepare('SELECT name_of(value) AS name FROM data').all(),
130+
[
131+
{ __proto__: null, name: 'one' },
132+
{ __proto__: null, name: 'two' },
133+
{ __proto__: null, name: 'three' },
134+
],
135+
);
136+
});
137+
138+
test('a nested iterator over a different statement still works', () => {
139+
const db = newDbWithRows();
140+
const inner = db.prepare('SELECT value FROM data');
141+
142+
db.function('sum_all', mustCall(() => {
143+
let total = 0;
144+
for (const row of inner.iterate()) {
145+
total += row.value;
146+
}
147+
return total;
148+
}));
149+
150+
assert.deepStrictEqual(
151+
db.prepare('SELECT sum_all() AS total LIMIT 1').get(),
152+
{ __proto__: null, total: 6 },
153+
);
154+
});
155+
});
156+
157+
suite('SQL tag store reentry is rejected', () => {
158+
for (const method of ['all', 'get', 'run']) {
159+
test(`tag store ${method} re-executing the same tag`, () => {
160+
const db = newDbWithRows();
161+
const sql = db.createTagStore(4);
162+
db.function('reenter', mustCall((value) => {
163+
assert.throws(
164+
() => sql[method]`SELECT reenter(value) AS value FROM data LIMIT 1`,
165+
reentryError,
166+
);
167+
return value;
168+
}));
169+
170+
assert.deepStrictEqual(
171+
sql.get`SELECT reenter(value) AS value FROM data LIMIT 1`,
172+
{ __proto__: null, value: 1 },
173+
);
174+
assert.strictEqual(db.isOpen, true);
175+
});
176+
}
177+
});
178+
179+
suite('aggregate functions', () => {
180+
test('reentry from an aggregate step is rejected', () => {
181+
const db = newDbWithRows();
182+
let statement;
183+
db.aggregate('reenter_agg', {
184+
start: 0,
185+
step: mustCall((total, value) => {
186+
assert.throws(() => statement.get(), reentryError);
187+
return total + value;
188+
}, 3),
189+
});
190+
191+
statement = db.prepare('SELECT reenter_agg(value) AS total FROM data');
192+
assert.deepStrictEqual(statement.get(), { __proto__: null, total: 6 });
193+
});
194+
195+
test('reentry from an aggregate result is rejected', () => {
196+
const db = newDbWithRows();
197+
let statement;
198+
db.aggregate('reenter_result', {
199+
start: 0,
200+
step: (total, value) => total + value,
201+
result: mustCall((total) => {
202+
assert.throws(() => statement.get(), reentryError);
203+
return total;
204+
}),
205+
});
206+
207+
statement = db.prepare('SELECT reenter_result(value) AS total FROM data');
208+
assert.deepStrictEqual(statement.get(), { __proto__: null, total: 6 });
209+
});
210+
});

0 commit comments

Comments
 (0)