Skip to content

sqlite: fix crash on db.close() from inside a user function#63183

Open
mceachen wants to merge 3 commits intonodejs:mainfrom
mceachen:fix-sqlite-reentrant-close-segv
Open

sqlite: fix crash on db.close() from inside a user function#63183
mceachen wants to merge 3 commits intonodejs:mainfrom
mceachen:fix-sqlite-reentrant-close-segv

Conversation

@mceachen
Copy link
Copy Markdown
Contributor

@mceachen mceachen commented May 8, 2026

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: #63180

(full disclosure: produced with assistance from claude and codex)

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels May 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.03%. Comparing base (bbf51ad) to head (c02e2c0).

Files with missing lines Patch % Lines
src/node_sqlite.cc 95.00% 1 Missing and 1 partial ⚠️
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     
Files with missing lines Coverage Δ
src/node_sqlite.h 83.09% <100.00%> (+2.45%) ⬆️
src/node_sqlite.cc 80.62% <95.00%> (+0.08%) ⬆️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Renegade334
Copy link
Copy Markdown
Member

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>
@mceachen mceachen force-pushed the fix-sqlite-reentrant-close-segv branch from 8adcb3b to c6ce251 Compare May 9, 2026 03:24
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>
@mceachen mceachen force-pushed the fix-sqlite-reentrant-close-segv branch from c6ce251 to 20d7ffa Compare May 9, 2026 03:28
@mceachen
Copy link
Copy Markdown
Contributor Author

mceachen 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.

@mceachen
Copy link
Copy Markdown
Contributor Author

mceachen commented May 9, 2026

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:sqlite segfaults when db.close() is called from a user-defined function callback during query execution

3 participants