Skip to content

sqlite: reject deserialize() while in a callback - #64796

Merged
trivikr merged 1 commit into
nodejs:mainfrom
trivikr:sqlite-reject-reentrant-close
Aug 9, 2026
Merged

sqlite: reject deserialize() while in a callback#64796
trivikr merged 1 commit into
nodejs:mainfrom
trivikr:sqlite-reject-reentrant-close

Conversation

@trivikr

@trivikr trivikr commented Jul 28, 2026

Copy link
Copy Markdown
Member

Refs: #64795

deserialize() could be called from a user-defined function invoked during statement execution, tearing down the database connection while sqlite3_step() was still using it. Reuse the existing callback depth check to throw ERR_INVALID_STATE instead, matching the guard already in place for close().


Assisted-by: codex:gpt-5.6-sol

@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 Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64796      +/-   ##
==========================================
- Coverage   90.33%   90.33%   -0.01%     
==========================================
  Files         760      760              
  Lines      248522   248523       +1     
  Branches    46904    46906       +2     
==========================================
- Hits       224513   224511       -2     
- Misses      15444    15451       +7     
+ Partials     8565     8561       -4     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.23% <100.00%> (+<0.01%) ⬆️

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

@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 28, 2026
@trivikr
trivikr force-pushed the sqlite-reject-reentrant-close branch from c11abfd to 4af37c6 Compare July 31, 2026 04:28
@trivikr
trivikr requested a review from geeksilva97 August 2, 2026 02:56
@trivikr
trivikr force-pushed the sqlite-reject-reentrant-close branch from 4af37c6 to c64b289 Compare August 3, 2026 19:58
@trivikr
trivikr force-pushed the sqlite-reject-reentrant-close branch 3 times, most recently from 46139cd to e6c5c8c Compare August 5, 2026 02:22

@TrevorBurnham TrevorBurnham left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The guard itself looks right to me: IsInCallback() is the correct predicate, since the crash needs a live sqlite3_step() frame, which is only reachable from a callback, and every step-reentrant entry point (xFunc, xStepBase/xValueBase, the authorizer, sqlite3changeset_apply) already takes CallbackDepthGuard. Placing it before FinalizeStatements() also means a rejected call leaves no partial state. Three non-blocking notes below, plus one on the docs.

doc/api/sqlite.md: the new throwing condition isn't documented. The database.deserialize() section only mentions that existing statements are finalized first; it'd help to state that the method throws ERR_INVALID_STATE when called from a user-defined function, aggregate, authorizer, or changeset filter/conflict callback. The same is missing for database.close() from #64743 — might be worth adding both here.

Comment thread src/node_sqlite.cc
Comment thread test/parallel/test-sqlite-serialize.js Outdated
@trivikr
trivikr force-pushed the sqlite-reject-reentrant-close branch from e6c5c8c to 19c3939 Compare August 6, 2026 18:10
@trivikr trivikr changed the title sqlite: prevent reentrant statement finalization sqlite: reject deserialize() while in a callback Aug 6, 2026
@trivikr
trivikr force-pushed the sqlite-reject-reentrant-close branch 2 times, most recently from ae8a08a to 0081aa8 Compare August 7, 2026 03:34

@TrevorBurnham TrevorBurnham left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick turnaround on the commit message and the test rename — both look good, and keeping the test with its deserialize siblings is a fair call.

The C++ guard is unchanged and still correct. I built this branch and confirmed the #64795 repro now throws instead of crashing, that coverage extends past the tested UDF case to aggregate and authorizer callbacks, and that there are no false positives across connections (deserializing an unrelated database from another database's callback still works). The remaining comments are all on the doc commit, plus one on the Fixes: line.

One more docs point that isn't in the diff: database.close() (line 291) has carried this same callback restriction since 5cef767, but its docs still say only "An exception is thrown if the database is not open." Documenting the restriction for deserialize() while leaving close() undocumented is an odd asymmetry — probably worth a one-line addition there while you're in this section.

Comment thread doc/api/sqlite.md Outdated
Comment thread doc/api/sqlite.md Outdated
Comment thread src/node_sqlite.cc
@TrevorBurnham

Copy link
Copy Markdown
Contributor

Looks ready to 🚢 to me.

One non-blocking note: database.close() is guarded by the same IsInCallback() check as deserialize(), and throws ERR_INVALID_STATE under the same conditions, but its docs don't reflect that. I'd suggest copying the sentence you added to the docs for deserialize() over to the docs for database.close().

@trivikr

trivikr commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

The database.close() doc update us posted at #65090

I'll mark it ready for review post rebase after this PR is merged, as they share a reference link.

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

deserialize() could be called from a user-defined function invoked
during statement execution, tearing down the database connection while
sqlite3_step() was still using it. Reuse the existing callback depth
check to throw ERR_INVALID_STATE instead, matching the guard already
in place for close().

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: codex:gpt-5.6-sol
PR-URL: nodejs#64796
Refs: nodejs#64795
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@trivikr
trivikr force-pushed the sqlite-reject-reentrant-close branch from c32ea43 to b2b7405 Compare August 9, 2026 23:27
@trivikr
trivikr merged commit b2b7405 into nodejs:main Aug 9, 2026
19 checks passed
@trivikr

trivikr commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Landed in b2b7405

@trivikr
trivikr deleted the sqlite-reject-reentrant-close branch August 9, 2026 23:28
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.

4 participants