| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 98.57143% with 1 line in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #65156 +/- ##
=======================================
Coverage 90.32% 90.32%
=======================================
Files 760 751 -9
Lines 249130 249185 +55
Branches 47041 47098 +57
=======================================
+ Hits 225030 225088 +58
+ Misses 15490 15485 -5
- Partials 8610 8612 +2
... and 46 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
SQLite requires that an authorizer callback not modify the connection that invoked it, and counts sqlite3_prepare_v2() and sqlite3_step() as modifications. node:sqlite let the callback call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync. Track authorizer depth on DatabaseSync with an RAII guard around the callback and throw ERR_INVALID_STATE from the affected entry points while it is on the stack. Covering every authorizer invocation, including the re-prepare that SQLite can run during sqlite3_step(), exposed a second and distinct hazard: reentering a statement that is currently being stepped is a use-after-free rather than a contract violation, since finalizing it frees the virtual machine under sqlite3_step() and re-running it resets that machine mid-execution. Any callback SQLite invokes during execution can reach it, so a user-defined function is enough. Track the statements currently being stepped and reject reentry into only those, which leaves a user-defined function free to prepare, run, and finalize its own helper statements. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Fixes: nodejs#63207 Assisted-by: claude:opus-5
Cover the guard paths the existing tests never reached: the authorizer guards on enableDefensive() and loadExtension(), the stepping guard on statement[Symbol.dispose](), and the stepping guards on the tag store's get(), all(), and iterate(). The tag store test looped over the four outer driver methods but always reentered through run(), so three of its four guards never fired. loadExtension() checks that extension loading is enabled before the authorizer guard, so its test opens the database with allowExtension. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
Sorry, something went wrong.
SQLite requires that an authorizer callback not modify the connection that invoked it, and counts sqlite3_prepare_v2() and sqlite3_step() as modifications. node:sqlite let the callback call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync. Track authorizer depth on DatabaseSync with an RAII guard around the callback and throw ERR_INVALID_STATE from the affected entry points while it is on the stack. Covering every authorizer invocation, including the re-prepare that SQLite can run during sqlite3_step(), exposed a second and distinct hazard: reentering a statement that is currently being stepped is a use-after-free rather than a contract violation, since finalizing it frees the virtual machine under sqlite3_step() and re-running it resets that machine mid-execution. Any callback SQLite invokes during execution can reach it, so a user-defined function is enough. Track the statements currently being stepped and reject reentry into only those, which leaves a user-defined function free to prepare, run, and finalize its own helper statements. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Fixes: #63207 Assisted-by: claude:opus-5 PR-URL: #65156 Reviewed-By: Xuguang Mei <meixuguang@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
SQLite requires that an authorizer callback not modify the connection that invoked it, and counts sqlite3_prepare_v2() and sqlite3_step() as modifications. node:sqlite let the callback call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync. Track authorizer depth on DatabaseSync with an RAII guard around the callback and throw ERR_INVALID_STATE from the affected entry points while it is on the stack. Covering every authorizer invocation, including the re-prepare that SQLite can run during sqlite3_step(), exposed a second and distinct hazard: reentering a statement that is currently being stepped is a use-after-free rather than a contract violation, since finalizing it frees the virtual machine under sqlite3_step() and re-running it resets that machine mid-execution. Any callback SQLite invokes during execution can reach it, so a user-defined function is enough. Track the statements currently being stepped and reject reentry into only those, which leaves a user-defined function free to prepare, run, and finalize its own helper statements. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Fixes: #63207 Assisted-by: claude:opus-5 PR-URL: #65156 Reviewed-By: Xuguang Mei <meixuguang@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
| Back | FazBrowse Home | New Git URL |
Fixes: #63207
Per sqlite3_set_authorizer(), an authorizer callback must not modify the connection that invoked it, and sqlite3_prepare_v2() and sqlite3_step() both count. node:sqlite allowed the callback to call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync.
Authorizer reentrancy. Track authorizer depth on DatabaseSync with an RAII guard around the callback and throw ERR_INVALID_STATE from the affected entry points while it is on the stack. Depth is per-connection, so a different DatabaseSync stays usable.
Guarded: prepare, exec, serialize, setAuthorizer, createSession, applyChangeset, createTagStore, function, aggregate, enableLoadExtension, enableDefensive, loadExtension, the limits setter; stmt.run/get/all/iterate; iter.next/return; sqlTagStore.run/get/all/iterate; session.changeset/patchset. db.close() and db.deserialize() keep their existing callback-depth messages.
The guard covers every authorizer invocation, not just those from an explicit prepare(): SQLite may re-prepare during sqlite3_step() after a schema change, and serialize() and the session changeset methods prepare internally. Reentry through changeset() never terminated — it recursed until the process died, uncatchable from JavaScript.
Statement reentry. Covering the re-prepare path surfaced a memory-safety bug rather than a contract violation: a statement that is currently being stepped cannot be reentered. Finalizing it frees the virtual machine sqlite3_step() is running, and re-running it resets that virtual machine mid-execution. Neither is authorizer-specific — a user-defined function reaches them:
Verified against unpatched bfa3e982ec3: stmt.run(), get(), all(), iterate(), stmt.close(), iter.return(), and re-entering the same cached tagged literal on a tag store each segfault. A single reentrant call on a small result set often returns cleanly, so the crash needs a row payload large enough to force a page fault, or nesting.
iter.next() is the exception: it advances the shared virtual machine rather than resetting or freeing it, so it corrupts iteration instead of crashing (300 nested calls over 800 rows survive). It is guarded alongside the rest because reentering a statement mid-step is not a state a caller can use correctly.
Gating on "any callback is running" would forbid a UDF from preparing, running, and finalizing its own helper statement, which is safe. Instead, track the statements currently being stepped and reject reentry into only those, with statement is already being executed. Tracking is a stack, so a UDF may reenter an inner statement it stepped but not the outer one, and it spans the paired sqlite3_reset() calls, which can run JavaScript through an aggregate's xFinal. statement[Symbol.dispose]() returns early when already finalized, so disposing a statement that is already closed stays a no-op and preserves a pending exception rather than wrapping it in a SuppressedError.
Deliberately unguarded. Three APIs reachable from a callback are left available: