Skip to content

sqlite: reject statement-less SQL in SQLTagStore - #65157

Open
TrevorBurnham wants to merge 2 commits into
nodejs:mainfrom
TrevorBurnham:sqlite/fix-tagstore-null-stmt
Open

sqlite: reject statement-less SQL in SQLTagStore#65157
TrevorBurnham wants to merge 2 commits into
nodejs:mainfrom
TrevorBurnham:sqlite/fix-tagstore-null-stmt

Conversation

@TrevorBurnham

@TrevorBurnham TrevorBurnham commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

sqlite3_prepare_v2() returns SQLITE_OK without producing a statement when its input holds no SQL, such as a comment or an empty string. Neither caller checked for this.

SQLTagStore cached a StatementSync wrapping the null sqlite3_stmt. Executing it reached sqlite3_clear_bindings(), which only guards against a null statement under SQLITE_ENABLE_API_ARMOR, and segfaulted:

const db = new DatabaseSync(':memory:');
const store = db.createTagStore();
store.run`-- comment`;  // segfault

All four query methods (run, get, all, iterate) were affected.

db.prepare() returned a StatementSync whose statement_ was null. Every method on it threw statement has been finalized, which was misleading because nothing had been finalized. It was also still inserted into statements_, and because IsFinalized() is true for a null statement, its destructor skipped UntrackStatement() and left a dangling pointer that a later close() would finalize.

Both now throw ERR_INVALID_ARG_VALUE at preparation. SQL that merely contains comments is unaffected.

Fixes: #65149

@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 9, 2026
Comment thread src/node_sqlite.cc Outdated
TrevorBurnham added a commit to TrevorBurnham/node that referenced this pull request Aug 9, 2026
Apply the same check to DatabaseSync::Prepare() so that statement-less
SQL is rejected at preparation instead of on first use.

Previously db.prepare('-- comment') returned a StatementSync whose
statement_ was null. Every method on it threw "statement has been
finalized", which was misleading because nothing had been finalized, and
the object was still inserted into statements_. Since IsFinalized() is
true for a null statement, its destructor skipped UntrackStatement() and
left a dangling pointer in the set that a later close() would finalize.

Refs: nodejs#65157 (comment)

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
TrevorBurnham added a commit to TrevorBurnham/node that referenced this pull request Aug 9, 2026
Apply the same check to DatabaseSync::Prepare() so that statement-less
SQL is rejected at preparation instead of on first use. This matches
SQLite's own oo1 JavaScript API, which throws when the SQL contains no
statements rather than exposing the C API's null statement pointer.

Previously db.prepare('-- comment') returned a StatementSync whose
statement_ was null. Every method on it threw "statement has been
finalized", which was misleading because nothing had been finalized, and
the object was still inserted into statements_. Since IsFinalized() is
true for a null statement, its destructor skipped UntrackStatement() and
left a dangling pointer in the set that a later close() would finalize.

Refs: nodejs#65157 (comment)
Refs: https://sqlite.org/wasm/doc/trunk/api-oo1.md

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@TrevorBurnham
TrevorBurnham force-pushed the sqlite/fix-tagstore-null-stmt branch from af2725f to 3d2d495 Compare August 9, 2026 15:55

@Renegade334 Renegade334 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

Comment thread doc/api/sqlite.md Outdated
TrevorBurnham added a commit to TrevorBurnham/node that referenced this pull request Aug 9, 2026
Apply the same check to DatabaseSync::Prepare() so that statement-less
SQL is rejected at preparation instead of on first use. This matches
SQLite's own oo1 JavaScript API, which throws when the SQL contains no
statements rather than exposing the C API's null statement pointer.

Previously db.prepare('-- comment') returned a StatementSync whose
statement_ was null. Every method on it threw "statement has been
finalized", which was misleading because nothing had been finalized, and
the object was still inserted into statements_. Since IsFinalized() is
true for a null statement, its destructor skipped UntrackStatement() and
left a dangling pointer in the set that a later close() would finalize.

Refs: nodejs#65157 (comment)
Refs: https://sqlite.org/wasm/doc/trunk/api-oo1.md

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@TrevorBurnham
TrevorBurnham force-pushed the sqlite/fix-tagstore-null-stmt branch from 3d2d495 to 45be4bd Compare August 9, 2026 19:18
@TrevorBurnham
TrevorBurnham marked this pull request as ready for review August 9, 2026 23:47
@trivikr trivikr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 10, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2026
@nodejs-github-bot

This comment was marked as resolved.

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65157      +/-   ##
==========================================
- Coverage   90.33%   90.33%   -0.01%     
==========================================
  Files         760      760              
  Lines      248523   248529       +6     
  Branches    46906    46895      -11     
==========================================
- Hits       224511   224509       -2     
+ Misses      15451    15450       -1     
- Partials     8561     8570       +9     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.49% <100.00%> (+0.25%) ⬆️

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

sqlite3_prepare_v2() returns SQLITE_OK without producing a statement
when its input holds no SQL, such as a comment. PrepareStatement() only
checked the return code, so it cached a StatementSync wrapping a null
sqlite3_stmt. Executing it reached sqlite3_clear_bindings(), which only
guards against a null statement under SQLITE_ENABLE_API_ARMOR, and
segfaulted.

Reject such input instead of caching it. The StatementSync methods
already avoid the crash because their IsFinalized() guard treats a null
statement as finalized.

Fixes: nodejs#65149

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
Apply the same check to DatabaseSync::Prepare() so that statement-less
SQL is rejected at preparation instead of on first use. This matches
SQLite's own oo1 JavaScript API, which throws when the SQL contains no
statements rather than exposing the C API's null statement pointer.

Previously db.prepare('-- comment') returned a StatementSync whose
statement_ was null. Every method on it threw "statement has been
finalized", which was misleading because nothing had been finalized, and
the object was still inserted into statements_. Since IsFinalized() is
true for a null statement, its destructor skipped UntrackStatement() and
left a dangling pointer in the set that a later close() would finalize.

Refs: nodejs#65157 (comment)
Refs: https://sqlite.org/wasm/doc/trunk/api-oo1.md

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@TrevorBurnham
TrevorBurnham force-pushed the sqlite/fix-tagstore-null-stmt branch from 45be4bd to d7ad352 Compare August 10, 2026 01:49
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 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

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. 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: segfault when a SQLTagStore query contains only a comment

4 participants