sqlite: reject reentry into a running statement - #65106
Open
TrevorBurnham wants to merge 1 commit into
Open
Conversation
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
Collaborator
|
Review requested:
|
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: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 thatsqlite3_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 existingtest-sqlite-udf-close.js.Assisted-by: claude:opus-5