Uh oh!
There was an error while loading. Please reload this page.
sqlite: fix crash on db.close() from inside a user function - #63183
sqlite: fix crash on db.close() from inside a user function#63183mceachen wants to merge 3 commits into
Conversation
nodejs-github-bot
commented
May 8, 2026
Review requested:
|
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #63183 +/- ##
==========================================
- Coverage 90.03% 90.03% -0.01%
==========================================
Files 713 713 Lines 224950 224986 +36 Branches 42532 42550 +18 ==========================================
+ Hits 202542 202572 +30 - Misses 14175 14183 +8 + Partials 8233 8231 -2
π New features to boost your workflow:
|
Renegade334
commented
May 8, 2026
This definitely shouldn't segfault, but it's worth saying that "don't close the db handle during a user function callback" is expressly part of the sqlite3_create_function API contract, as is finalizing/resetting the statement, which I think we are also able to do: letstmt;db.function('x',()=>stmt.get());stmt=db.prepare('SELECT x()');stmt.get();Rather than performing forbidden operations and then mitigating against the consequences, it would probably be better to prevent those operations from occurring in the first place. Could we instead set some sort of state while a user callback function is being called, that causes attempts to close/finalize to be rejected with an exception? |
Calling db.close() from inside a user-defined function callback while sqlite3_step is on the call stack caused two distinct crashes: 1. DatabaseSync::Close ran sqlite3_finalize on the statement whose sqlite3_step frame was still active, freeing the VM that step was executing. The outer step then operated on freed memory. 2. Even if (1) is avoided, StatementExecutionHelper::Run dereferenced db->Connection() via sqlite3_last_insert_rowid / sqlite3_changes64 after step returned. The reentrant close zeroed connection_, so the deref crashed. Add a MarkStepping() RAII guard wrapped around every sqlite3_step caller. If Finalize() is called while stepping_, defer it; the guard's destructor runs the deferred finalize after step returns. Add a connection-null check in StatementExecutionHelper::Run before the connection-dependent reads, throwing ERR_INVALID_STATE. Fixes: nodejs#63180 Signed-off-by: Matthew McEachen <matthew@photostructure.com>
8adcb3b to
c6ce251ComparePer SQLite's sqlite3_create_function() contract, closing the database or finalizing/resetting a statement is forbidden while a user-supplied callback is on the stack. The previous fix detected the close case and deferred the finalize after sqlite3_step returned. Per reviewer feedback, prevent the forbidden operations up front instead: track a user-callback depth on DatabaseSync (via an RAII guard wrapped around every JS call from xFunc, the aggregate xStep/xValue/xFinal/xInverse and start callbacks, and the authorizer callback), and have db.close(), the statement execution methods, the iterator's next/return, and the SQL tag store's run/get/all/iterate/clear throw ERR_INVALID_STATE when called from inside such a callback. Removes the now-unreachable deferred-finalize machinery (stepping_, finalize_pending_, MarkStepping) and the connection-null check in StatementExecutionHelper::Run. Adds tests for scalar, aggregate, and authorizer callbacks, and for reentrant iter.next(). Signed-off-by: Matthew McEachen <matthew@photostructure.com>
c6ce251 to
20d7ffaComparemceachen
commented
May 9, 2026
Thanks @Renegade334 -- agreed, switched to prevention. DatabaseSync now tracks a user-callback depth via an RAII guard wrapped around every SQLite-invoked JS call (scalar xFunc, aggregate xStep/xValue/xFinal/xInverse/start, authorizer). While depth > 0, db.close(), the statement and tag-store execution methods, the iterator's next/return, tagStore.clear(), and db.deserialize() throw ERR_INVALID_STATE. Forbidden state is now unreachable, so the deferred-finalize machinery and the connection == nullptr check are gone. New tests: aggregate step/result/start, authorizer, and reentrant iter.next() (your stmt.get()-from-callback example). I just rebased to main (hence the force-push). Happy to squash before landing. |
Ugh, Derp, all nested statement execution from SQL functions is now rejected. I've added a regression test and will push a new diff soon. Edit: the third commit is ready for review. |
The previous commit's db-wide IsInUserFunctionCallback guard was too
broad: it rejected legitimate cross-statement use from inside a user
function (the common "lookup" pattern), e.g.
const lookup = db.prepare('SELECT v FROM lookup WHERE id = ?');
db.function('lookup_v', (id) => lookup.get(id).v);
SQLite only forbids reentry into the *currently running* statement
(recursive sqlite3_step, sqlite3_reset, or sqlite3_finalize), not
operations on other statements on the same connection.
Track per-statement stepping_ on StatementSync, set via a MarkStepping
RAII guard wrapped around each sqlite3_step caller. JS methods that
would step, reset, or finalize a statement (StatementSync run/get/all/
iterate, iterator next/return, SQLTagStore run/get/all/iterate) check
that flag on the specific statement instead of the db-wide depth.
Keep the db-wide IsInUserFunctionCallback check only where the
operation is broadly invasive: db.close, db.deserialize, and SQL tag
store .clear, all of which finalize tracked statements (potentially
the running one).
Drop the authorizer scope: SQLite's authorizer rules are stricter
than user-defined functions (prepare/exec are forbidden too) and
warrant a separate change rather than partial coverage here.
Add tests for the legitimate cross-statement and cross-tag-store
patterns (now passing), and for db[Symbol.dispose]() being a
documented no-op when invoked from a callback.
Signed-off-by: Matthew McEachen <matthew@photostructure.com>df560ee to
c02e2c0Comparemceachen
commented
May 21, 2026
@Renegade334 is there anything I can do or change to help this PR? |
louwers
left a comment
There was a problem hiding this comment.
There's also a filter callback for applying a changeset.
| THROW_AND_RETURN_ON_BAD_STATE( | ||
| env, | ||
| db->IsInUserFunctionCallback(), | ||
| "database cannot be closed inside a user-defined function callback"); |
There was a problem hiding this comment.
This guard doesn't cover the authorizer callback, and that leaves the original use-after-free reachable.
The authorizer runs during statement preparation, and preparation can happen inside sqlite3_step: on SQLITE_SCHEMA the public sqlite3_step wrapper calls sqlite3Reprepare in a loop (deps/sqlite/sqlite3.c:94612-94614), which reaches sqlite3AuthCheck. So:
db.setAuthorizer(()=>{db.close();return0;});db.function('f',()=>{db.exec('CREATE TABLE z (a)');return1;});db.prepare('SELECT f(v) FROM t').all();// multi-rowRow 1's f() does DDL, which expires all prepared statements. On row 2 sqlite3_step returns SQLITE_SCHEMA, reprepares in place, and invokes the authorizer β where IsInUserFunctionCallback() is false, so db.close() goes through. FinalizeStatements() then finalizes the very VM whose sqlite3_step frame is live (crash 1 from the description), and zeroes connection_. Through StatementSync::Run the follow-on sqlite3_last_insert_rowid(db->Connection()) derefs null (crash 2) β commit 2 dropped the connection-null check commit 1 added.
The commit message defers the authorizer to a separate change, but this isn't a coverage nicety: it's the same crash, still reachable from pure JS, with the interim mitigations removed. Note main already wraps AuthorizerCallback (src/node_sqlite.cc:2552) β see my other comment.
| // detected separately via the per-statement | ||
| // StatementSync::IsStepping() flag, which leaves cross-statement use | ||
| // (the "lookup" pattern) unaffected. | ||
| inline auto EnterUserFunctionCallback() { |
There was a problem hiding this comment.
This branch is based on bbf51ad24cb (2026-05-08) and adds a second mechanism alongside one main already has, with narrower coverage.
main carries IsInCallback() / callback_depth_ / class CallbackDepthGuard (src/node_sqlite.h:232-234,249,409-416) applied at five sites β including AuthorizerCallback (node_sqlite.cc:2552) and the sqlite3changeset_apply call (:2409) β plus post-callback IsOpen() checks in xFunc/xStepBase/xValueBase/GetAggregate.
Concretely, the renamed message here fails an existing test: test/parallel/test-sqlite-udf-close.js:33 on main asserts the exact string 'database cannot be closed while in a callback' for all of all/get/run/iterate.
Worth rebasing and building on the existing guard rather than in parallel with it. Two things not to lose in the process: main's authorizer and changeset coverage, and its post-callback IsOpen() checks. That should also make the unrelated sqlite3_reset β ResetStatement() changes in SQLTagStore disappear, since those already match main.
mceachen
commented
Aug 7, 2026
Confirmed that #64743 addressed this. |
Calling db.close() from inside a user-defined function callback while sqlite3_step is on the call stack caused two distinct crashes:
DatabaseSync::Close ran sqlite3_finalize on the statement whose sqlite3_step frame was still active, freeing the VM that step was executing. The outer step then operated on freed memory.
Even if (1) is avoided, StatementExecutionHelper::Run dereferenced db->Connection() via sqlite3_last_insert_rowid / sqlite3_changes64 after step returned. The reentrant close zeroed connection_, so the deref crashed.
Add a MarkStepping() RAII guard wrapped around every sqlite3_step caller. If Finalize() is called while stepping_, defer it; the guard's destructor runs the deferred finalize after step returns. Add a connection-null check in StatementExecutionHelper::Run before the connection-dependent reads, throwing ERR_INVALID_STATE.
Fixes: #63180
(full disclosure: produced with assistance from claude and codex)