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

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.

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 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.32%. Comparing base (e2d7b34) to head (ee4e9b5).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 90.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65106   +/-   ##
=======================================
  Coverage   90.31%   90.32%           
=======================================
  Files         759      759           
  Lines      248290   248314   +24     
  Branches    46859    46878   +19     
=======================================
+ Hits       224241   224284   +43     
+ Misses      15472    15466    -6     
+ Partials     8577     8564   -13     
Files with missing lines Coverage Δ
src/node_sqlite.h 83.56% <100.00%> (+0.95%) ⬆️
src/node_sqlite.cc 81.01% <90.00%> (-0.05%) ⬇️

... and 29 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.

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

2 participants