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 intonodejs:mainfrom
Conversation
|
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:
|
|
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: let stmt;
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
c6ce251
Compare
Per 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
20d7ffa
Compare
|
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
c02e2c0
Compare
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)