Skip to content

sqlite: reject reentry into a running statement - #65106

Open
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite/guard-statement-reentry
Open

sqlite: reject reentry into a running statement#65106
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite/guard-statement-reentry

Conversation

@TrevorBurnham

Copy link
Copy Markdown
Contributor

Fixes: #65102

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 (#64743) is tracked per database, so it cannot distinguish reentry into the running statement from the common pattern of querying a different statement from a callback.

This adds a per-statement flag, set for the duration of an execution, and rejects step/reset/finalize on that statement with ERR_INVALID_STATE: statement is currently being executed. Before this change:

// silently consumed rows from the iteration in progress
let iter;
db.function('reenter', () => { iter.next(); return 0; });
iter = db.prepare('SELECT reenter() FROM t').iterate();
iter.next();

// failed with "Maximum call stack size exceeded" rather than reporting the constraint
let stmt;
db.function('x', () => stmt.get());
stmt = db.prepare('SELECT x()');
stmt.get();

Covered entry points: all(), get(), run(), iterate(), iterator.next(), iterator.return(), close(), [Symbol.dispose](), and the four SQL tag store methods. close() and [Symbol.dispose]() are included because finalizing mid-step frees the virtual machine that sqlite3_step() is still executing.

Statements other than the running one are unaffected, so the lookup pattern and nested iteration over a different statement keep working — both are covered by tests.

The flag is set before parameter binding, since a getter on a named-parameters object can also reenter.

Prior art: #63183 took this approach alongside its own database-level guard. That PR was closed once #64743 landed, so this salvages the per-statement half and builds on the guard already in main.

Verified locally on macOS arm64: the new test file plus all 20 parallel/test-sqlite* tests pass, including the existing test-sqlite-udf-close.js.

Assisted-by: claude:opus-5

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Aug 7, 2026
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.29%. Comparing base (e2d7b34) to head (02e92f5).
⚠️ Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 86.36% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65106      +/-   ##
==========================================
- Coverage   90.31%   90.29%   -0.02%     
==========================================
  Files         759      759              
  Lines      248290   248320      +30     
  Branches    46859    46871      +12     
==========================================
- Hits       224241   224228      -13     
- Misses      15472    15522      +50     
+ Partials     8577     8570       -7     
Files with missing lines Coverage Δ
src/node_sqlite.h 83.56% <100.00%> (+0.95%) ⬆️
src/node_sqlite.cc 81.15% <86.36%> (+0.09%) ⬆️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@trivikr

This comment was marked as outdated.

@TrevorBurnham
TrevorBurnham force-pushed the sqlite/guard-statement-reentry branch from ee4e9b5 to 986eb0e Compare August 7, 2026 17:41
@TrevorBurnham

Copy link
Copy Markdown
Contributor Author

@trivikr I've addressed the issues you noted. The format-cpp check is now timing out after 15 minutes ("The operation was canceled"); the same check succeeds locally.

Comment thread src/node_sqlite.cc
Comment thread test/parallel/test-sqlite-udf-statement-reentry.js
@TrevorBurnham
TrevorBurnham force-pushed the sqlite/guard-statement-reentry branch from 986eb0e to fde8f11 Compare August 8, 2026 12:09
Comment thread test/parallel/test-sqlite-udf-statement-reentry.js
Comment thread test/parallel/test-sqlite-udf-statement-reentry.js Outdated
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.

The mark is set before parameters are bound, so a getter or valueOf()
that reenters while its own arguments are being evaluated is rejected
as well. Without this, two iterators could share one virtual machine
and interleave rows from a single result set.

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
Assisted-by: claude:opus-5
@TrevorBurnham
TrevorBurnham force-pushed the sqlite/guard-statement-reentry branch from fde8f11 to 02e92f5 Compare August 8, 2026 14:58
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sqlite: reentrancy into a running statement from a user-defined function is unguarded

3 participants