Skip to content

sqlite: reject reentry while binding parameters - #65294

Open
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite/binding-window-reentry
Open

sqlite: reject reentry while binding parameters#65294
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite/binding-window-reentry

Conversation

@TrevorBurnham

@TrevorBurnham TrevorBurnham commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Follow-up for #65156. That PR added tracking for the statements currently being stepped and rejects reentry into them. The guard is established inside the execution helpers, though, which the entry points reach only after resetting the statement and binding its parameters.

Binding reads properties off the supplied object, so a named-parameter getter runs JavaScript in that window. On current main:

const { DatabaseSync } = require('node:sqlite');
const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE data (value INTEGER); INSERT INTO data VALUES (1),(2),(3);');
const st = db.prepare('SELECT value FROM data WHERE value >= $min');

let inner;
const params = { get $min() { inner ??= st.iterate({ $min: 1 }); return 1; } };

const outer = st.iterate(params);
console.log(outer.next(), inner.next(), outer.next(), inner.next());
{ done: false, value: { value: 1 } }   // outer
{ done: false, value: { value: 2 } }   // inner
{ done: false, value: { value: 3 } }   // outer
{ done: true,  value: null }           // inner

Two live iterators over one virtual machine. The reset-generation check does not catch it because the inner iterate() bumps the generation before the outer iterator is constructed, so both record the same value and neither is invalidated.

This PR hoists the guard to the four StatementSync entry points, before the reset, so it spans binding as well as stepping. The guards inside the helpers are left alone; stepping_statements_ is a stack, so the nested acquisition is balanced. It also adds test cases salvaged from #65106, which was superseded by #65156.

c4b755e tracks the statements currently being stepped and rejects
reentry into them, but the guard is established inside the execution
helpers, after the entry point has already reset the statement and
bound its parameters. Binding reads properties off the supplied
object, so a named-parameter getter runs JavaScript in that window.
Reentering the same statement there resets it a second time and, for
iterate(), hands out a second iterator; both iterators record the
same reset generation, so neither is invalidated and they interleave
rows from one virtual machine.

Establish the guard at the four StatementSync entry points instead,
before the reset, so it spans binding as well as stepping. The
existing guards inside the helpers are left in place; the stepping
set is a stack, so the nested acquisition is balanced.

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
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 14, 2026
@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.30%. Comparing base (c8996ea) to head (58b955d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65294      +/-   ##
==========================================
- Coverage   90.33%   90.30%   -0.03%     
==========================================
  Files         751      751              
  Lines      250048   250239     +191     
  Branches    47254    47300      +46     
==========================================
+ Hits       225877   225990     +113     
- Misses      15553    15620      +67     
- Partials     8618     8629      +11     
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.20% <100.00%> (-0.05%) ⬇️

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

2 participants