sqlite: reject deserialize() while in a callback - #64796
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
c11abfd to
4af37c6
Compare
4af37c6 to
c64b289
Compare
46139cd to
e6c5c8c
Compare
TrevorBurnham
left a comment
There was a problem hiding this comment.
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.
e6c5c8c to
19c3939
Compare
ae8a08a to
0081aa8
Compare
TrevorBurnham
left a comment
There was a problem hiding this comment.
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.
|
Looks ready to 🚢 to me. One non-blocking note: |
|
The I'll mark it ready for review post rebase after this PR is merged, as they share a reference link. |
This comment was marked as outdated.
This comment was marked as outdated.
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>
c32ea43 to
b2b7405
Compare
|
Landed in b2b7405 |
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