Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mceachen wants to merge 3 commits into from
+444 −14
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14cd548
sqlite: fix crash on db.close() from inside a user function
mceachen May 8, 2026
20d7ffa
sqlite: reject forbidden ops inside user-defined function callbacks
mceachen May 9, 2026
c02e2c0
sqlite: narrow user-callback guard to per-statement reentry
mceachen May 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 67 additions & 14 deletions src/node_sqlite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,13 +386,16 @@ class CustomAggregate {
}

Local<Value> ret;
if (!(self->*mptr)
.Get(isolate)
->Call(env->context(), recv, argc + 1, js_argv.data())
.ToLocal(&ret)) {
self->db_->SetIgnoreNextSQLiteError(true);
sqlite3_result_error(ctx, "", 0);
return;
{
auto guard = self->db_->EnterUserFunctionCallback();
if (!(self->*mptr)
.Get(isolate)
->Call(env->context(), recv, argc + 1, js_argv.data())
.ToLocal(&ret)) {
self->db_->SetIgnoreNextSQLiteError(true);
sqlite3_result_error(ctx, "", 0);
return;
}
}

agg->value.Reset(isolate, ret);
Expand Down Expand Up @@ -422,6 +425,7 @@ class CustomAggregate {
Local<Function>::New(env->isolate(), self->result_fn_);
Local<Value> js_arg[] = {Local<Value>::New(isolate, agg->value)};

auto guard = self->db_->EnterUserFunctionCallback();
if (!fn->Call(env->context(), Null(isolate), 1, js_arg)
.ToLocal(&result)) {
self->db_->SetIgnoreNextSQLiteError(true);
Expand Down Expand Up @@ -455,6 +459,7 @@ class CustomAggregate {
Local<Value> start_v = Local<Value>::New(isolate, start_);
if (start_v->IsFunction()) {
auto fn = start_v.As<Function>();
auto guard = db_->EnterUserFunctionCallback();
MaybeLocal<Value> retval =
fn->Call(env_->context(), Null(isolate), 0, nullptr);
if (!retval.ToLocal(&start_v)) {
Expand Down Expand Up @@ -698,6 +703,7 @@ void UserDefinedFunction::xFunc(sqlite3_context* ctx,
js_argv.emplace_back(local);
}

auto guard = self->db_->EnterUserFunctionCallback();
MaybeLocal<Value> retval =
fn->Call(env->context(), recv, argc, js_argv.data());
Local<Value> result;
Expand Down Expand Up @@ -1420,6 +1426,10 @@ void DatabaseSync::Close(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_ON_BAD_STATE(
env,
db->IsInUserFunctionCallback(),
"database cannot be closed inside a user-defined function callback");

@TrevorBurnham TrevorBurnham Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(); return 0; });
db.function('f', () => { db.exec('CREATE TABLE z (a)'); return 1; });
db.prepare('SELECT f(v) FROM t').all();  // multi-row

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

db->FinalizeStatements();
db->DeleteSessions();
int r = sqlite3_close_v2(db->connection_);
Expand Down Expand Up @@ -1808,6 +1818,11 @@ void DatabaseSync::Deserialize(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_ON_BAD_STATE(
env,
db->IsInUserFunctionCallback(),
"database operation is not allowed inside a user-defined function "
"callback");

if (!args[0]->IsUint8Array()) {
THROW_ERR_INVALID_ARG_TYPE(env->isolate(),
Expand Down Expand Up @@ -2573,6 +2588,9 @@ StatementSync::~StatementSync() {
}

void StatementSync::Finalize() {
if (statement_ == nullptr) {
return;
}
sqlite3_finalize(statement_);
statement_ = nullptr;
InvalidateColumnNameCache();
Expand Down Expand Up @@ -3018,6 +3036,8 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");
Isolate* isolate = env->isolate();
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
Expand All @@ -3026,9 +3046,9 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
return;
}

auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });

Local<Value> result;
auto step = stmt->MarkStepping();
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
if (StatementExecutionHelper::All(env,
stmt->db_.get(),
stmt->statement_,
Expand All @@ -3045,6 +3065,8 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());

Expand All @@ -3068,6 +3090,8 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());

Expand All @@ -3076,6 +3100,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
}

Local<Value> result;
auto step = stmt->MarkStepping();
if (StatementExecutionHelper::Get(env,
stmt->db_.get(),
stmt->statement_,
Expand All @@ -3092,6 +3117,8 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());

Expand All @@ -3100,6 +3127,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
}

Local<Object> result;
auto step = stmt->MarkStepping();
if (StatementExecutionHelper::Run(
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
.ToLocal(&result)) {
Expand Down Expand Up @@ -3352,8 +3380,11 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
return;
}

THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");

uint32_t n_params = args.Length() - 1;
int r = sqlite3_reset(stmt->statement_);
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
Expand All @@ -3364,6 +3395,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
}

Local<Object> result;
auto step = stmt->MarkStepping();
if (StatementExecutionHelper::Run(
env, stmt->db_.get(), stmt->statement_, stmt->use_big_ints_)
.ToLocal(&result)) {
Expand All @@ -3385,8 +3417,11 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
return;
}

THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");

uint32_t n_params = args.Length() - 1;
int r = sqlite3_reset(stmt->statement_);
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
Expand Down Expand Up @@ -3420,10 +3455,13 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
return;
}

THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");

uint32_t n_params = args.Length() - 1;
Isolate* isolate = env->isolate();

int r = sqlite3_reset(stmt->statement_);
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());

int param_count = sqlite3_bind_parameter_count(stmt->statement_);
Expand All @@ -3435,6 +3473,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
}

Local<Value> result;
auto step = stmt->MarkStepping();
if (StatementExecutionHelper::Get(env,
stmt->db_.get(),
stmt->statement_,
Expand All @@ -3459,10 +3498,13 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
return;
}

THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsStepping(), "statement is currently being executed");

uint32_t n_params = args.Length() - 1;
Isolate* isolate = env->isolate();

int r = sqlite3_reset(stmt->statement_);
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());

int param_count = sqlite3_bind_parameter_count(stmt->statement_);
Expand All @@ -3473,8 +3515,9 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
}
}

auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
Local<Value> result;
auto step = stmt->MarkStepping();
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
if (StatementExecutionHelper::All(env,
stmt->db_.get(),
stmt->statement_,
Expand All @@ -3488,6 +3531,11 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
void SQLTagStore::Clear(const FunctionCallbackInfo<Value>& args) {
SQLTagStore* store;
ASSIGN_OR_RETURN_UNWRAP(&store, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env,
store->database_->IsInUserFunctionCallback(),
"tag store cannot be cleared inside a user-defined function callback");
store->sql_tags_.Clear();
}

Expand Down Expand Up @@ -3679,6 +3727,8 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, iter->stmt_->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_ON_BAD_STATE(
env, iter->stmt_->IsStepping(), "statement is currently being executed");
Isolate* isolate = env->isolate();

auto iter_template = getLazyIterTemplate(env);
Expand All @@ -3701,6 +3751,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
iter->statement_reset_generation_ != iter->stmt_->reset_generation_,
"iterator was invalidated");

auto step = iter->stmt_->MarkStepping();
int r = sqlite3_step(iter->stmt_->statement_);
if (r != SQLITE_ROW) {
CHECK_ERROR_OR_THROW(
Expand Down Expand Up @@ -3755,6 +3806,8 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, iter->stmt_->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_ON_BAD_STATE(
env, iter->stmt_->IsStepping(), "statement is currently being executed");
Isolate* isolate = env->isolate();

sqlite3_reset(iter->stmt_->statement_);
Expand Down
32 changes: 32 additions & 0 deletions src/node_sqlite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,24 @@ class DatabaseSync : public BaseObject {
}
sqlite3* Connection();

// SQLite forbids closing the database while a user-defined scalar or
// aggregate function callback is on the stack. Wrap every such
// callback with the RAII guard returned by EnterUserFunctionCallback().
// db.close()/deserialize() and SQL tag store .clear() check
// IsInUserFunctionCallback() and refuse to run, since they would
// finalize statements (potentially the running one). Reentry into the
// *running* statement (recursive step, reset, or finalize) is
// detected separately via the per-statement
// StatementSync::IsStepping() flag, which leaves cross-statement use
// (the "lookup" pattern) unaffected.
inline auto EnterUserFunctionCallback() {

@TrevorBurnham TrevorBurnham Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

user_function_callback_depth_++;
return OnScopeLeave([this]() { user_function_callback_depth_--; });
}
bool IsInUserFunctionCallback() const {
return user_function_callback_depth_ > 0;
}

// In some situations, such as when using custom functions, it is possible
// that SQLite reports an error while JavaScript already has a pending
// exception. In this case, the SQLite error should be ignored. These methods
Expand All @@ -241,6 +259,7 @@ class DatabaseSync : public BaseObject {
bool enable_load_extension_;
sqlite3* connection_;
bool ignore_next_sqlite_error_;
int user_function_callback_depth_ = 0;

std::set<BackupJob*> backups_;
std::set<sqlite3_session*> sessions_;
Expand Down Expand Up @@ -283,6 +302,18 @@ class StatementSync : public BaseObject {
bool GetCachedColumnNames(v8::LocalVector<v8::Name>* keys);
void Finalize();
bool IsFinalized();
bool IsStepping() const { return stepping_; }

// RAII guard: marks this statement as being stepped while alive.
// JS-callable methods that would step, reset, or finalize this
// statement check IsStepping() and throw — that's the
// sqlite3_step / sqlite3_reset / sqlite3_finalize reentry SQLite
// forbids while the statement's user-defined function callback is
// on the stack.
inline auto MarkStepping() {
stepping_ = true;
return OnScopeLeave([this]() { stepping_ = false; });
}

SET_MEMORY_INFO_NAME(StatementSync)
SET_SELF_SIZE(StatementSync)
Expand All @@ -295,6 +326,7 @@ class StatementSync : public BaseObject {
bool use_big_ints_;
bool allow_bare_named_params_;
bool allow_unknown_named_params_;
bool stepping_ = false;
uint64_t reset_generation_ = 0;
std::optional<std::map<std::string, std::string>> bare_named_params_;
inline int ResetStatement();
Expand Down
Loading
Loading