From 6c683d007734cfedff91ed5da1513733f765fd38 Mon Sep 17 00:00:00 2001 From: semimikoh Date: Fri, 15 May 2026 14:21:38 +0900 Subject: [PATCH 1/4] sqlite: check sqlite3_step() and sqlite3_reset() results Signed-off-by: semimikoh --- src/node_sqlite.cc | 72 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 57 insertions(+), 15 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 027372e42da..0a84efe70cf 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -88,6 +88,17 @@ inline MaybeLocal Utf8StringMaybeOneByte(Isolate* isolate, } \ } while (0) +#define RESET_OR_THROW(isolate, db, stmt, ret) \ + CHECK_ERROR_OR_THROW((isolate), (db), sqlite3_reset((stmt)), SQLITE_OK, (ret)) + +// Surface deferred SQLite errors that sqlite3_reset() returns from the prior +// sqlite3_step(). Disables the safety-net reset guard via |needs_reset|. +#define RESET_AND_CHECK(isolate, db, stmt, needs_reset, ret) \ + do { \ + (needs_reset) = false; \ + RESET_OR_THROW((isolate), (db), (stmt), (ret)); \ + } while (0) + #define THROW_AND_RETURN_ON_BAD_STATE(env, condition, msg) \ do { \ if ((condition)) { \ @@ -3005,9 +3016,17 @@ MaybeLocal StatementExecutionHelper::Run(Environment* env, bool use_big_ints) { Isolate* isolate = env->isolate(); EscapableHandleScope scope(isolate); - sqlite3_step(stmt); - int r = sqlite3_reset(stmt); - CHECK_ERROR_OR_THROW(isolate, db, r, SQLITE_OK, MaybeLocal()); + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt); + }); + + int step_r = sqlite3_step(stmt); + if (step_r != SQLITE_DONE && step_r != SQLITE_ROW) { + THROW_ERR_SQLITE_ERROR(isolate, db); + return MaybeLocal(); + } + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); sqlite3_int64 last_insert_rowid = sqlite3_last_insert_rowid(db->Connection()); sqlite3_int64 changes = sqlite3_changes64(db->Connection()); @@ -3081,10 +3100,16 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, bool use_big_ints) { Isolate* isolate = env->isolate(); EscapableHandleScope scope(isolate); - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt); }); + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt); + }); int r = sqlite3_step(stmt); - if (r == SQLITE_DONE) return scope.Escape(Undefined(isolate)); + if (r == SQLITE_DONE) { + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); + return scope.Escape(Undefined(isolate)); + } if (r != SQLITE_ROW) { THROW_ERR_SQLITE_ERROR(isolate, db); return MaybeLocal(); @@ -3092,7 +3117,8 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, int num_cols = sqlite3_column_count(stmt); if (num_cols == 0) { - return Undefined(isolate); + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); + return scope.Escape(Undefined(isolate)); } LocalVector row_values(isolate); @@ -3101,9 +3127,9 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, return MaybeLocal(); } + Local result; if (return_arrays) { - return scope.Escape( - Array::New(isolate, row_values.data(), row_values.size())); + result = Array::New(isolate, row_values.data(), row_values.size()); } else { LocalVector keys(isolate); keys.reserve(num_cols); @@ -3116,9 +3142,12 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, } DCHECK_EQ(keys.size(), row_values.size()); - return scope.Escape(Object::New( - isolate, Null(isolate), keys.data(), row_values.data(), num_cols)); + result = Object::New( + isolate, Null(isolate), keys.data(), row_values.data(), num_cols); } + + RESET_AND_CHECK(isolate, db, stmt, needs_reset, MaybeLocal()); + return scope.Escape(result); } void StatementSync::All(const FunctionCallbackInfo& args) { @@ -3135,8 +3164,10 @@ void StatementSync::All(const FunctionCallbackInfo& args) { return; } - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); - + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt->statement_); + }); Local result; if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3144,6 +3175,8 @@ void StatementSync::All(const FunctionCallbackInfo& args) { stmt->return_arrays_, stmt->use_big_ints_) .ToLocal(&result)) { + RESET_AND_CHECK( + isolate, stmt->db_.get(), stmt->statement_, needs_reset, void()); args.GetReturnValue().Set(result); } } @@ -3577,7 +3610,10 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { return; } - auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); }); + bool needs_reset = true; + auto reset = OnScopeLeave([&]() { + if (needs_reset) sqlite3_reset(stmt->statement_); + }); Local result; if (StatementExecutionHelper::All(env, stmt->db_.get(), @@ -3585,6 +3621,8 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { stmt->return_arrays_, stmt->use_big_ints_) .ToLocal(&result)) { + RESET_AND_CHECK( + isolate, stmt->db_.get(), stmt->statement_, needs_reset, void()); args.GetReturnValue().Set(result); } } @@ -3811,7 +3849,10 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { if (r != SQLITE_ROW) { CHECK_ERROR_OR_THROW( env->isolate(), iter->stmt_->db_.get(), r, SQLITE_DONE, void()); - sqlite3_reset(iter->stmt_->statement_); + RESET_OR_THROW(env->isolate(), + iter->stmt_->db_.get(), + iter->stmt_->statement_, + void()); iter->done_ = true; MaybeLocal values[] = {Boolean::New(isolate, true), Null(isolate)}; Local result; @@ -3864,7 +3905,8 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo& args) { env, iter->stmt_->IsFinalized(), "statement has been finalized"); Isolate* isolate = env->isolate(); - sqlite3_reset(iter->stmt_->statement_); + RESET_OR_THROW( + isolate, iter->stmt_->db_.get(), iter->stmt_->statement_, void()); iter->done_ = true; auto iter_template = getLazyIterTemplate(env); From f369e87fa8eb2a72f5f00bd7d0ac5cb9819c000b Mon Sep 17 00:00:00 2001 From: semimikoh Date: Fri, 7 Aug 2026 10:49:28 +0900 Subject: [PATCH 2/4] sqlite: address review feedback on reset error handling - StatementSyncIterator::Return() no longer throws on a deferred reset error, matching the OnScopeLeave guards used elsewhere: it is invoked during abrupt iterator completion (e.g. a throw inside a for...of body), and throwing there would discard the caller's already-pending exception. - Add a short comment on the accepted SQLITE_ROW result in Run(). - Add tests covering get()/all() surfacing a deferred SQLite error from reset() after already building a row/array, the iterator not replaying results after natural exhaustion, and a pending exception propagating correctly when the loop body throws mid-iteration. Signed-off-by: semimikoh --- src/node_sqlite.cc | 10 ++- test/parallel/test-sqlite-statement-sync.js | 76 +++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 0a84efe70cf..77ac1c28bf1 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -3022,6 +3022,9 @@ MaybeLocal StatementExecutionHelper::Run(Environment* env, }); int step_r = sqlite3_step(stmt); + // SQLITE_ROW is accepted here (and discarded) so that run() can still be + // used on RETURNING/SELECT statements, matching prior behavior of + // ignoring the step result entirely. if (step_r != SQLITE_DONE && step_r != SQLITE_ROW) { THROW_ERR_SQLITE_ERROR(isolate, db); return MaybeLocal(); @@ -3905,8 +3908,11 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo& args) { env, iter->stmt_->IsFinalized(), "statement has been finalized"); Isolate* isolate = env->isolate(); - RESET_OR_THROW( - isolate, iter->stmt_->db_.get(), iter->stmt_->statement_, void()); + // Unlike Next(), the reset result is intentionally ignored here: Return() + // is invoked by the language during abrupt completion (e.g. a `throw` + // inside a `for...of` body), and throwing on a deferred SQLite error + // would discard the caller's already-pending exception. + sqlite3_reset(iter->stmt_->statement_); iter->done_ = true; auto iter_template = getLazyIterTemplate(env); diff --git a/test/parallel/test-sqlite-statement-sync.js b/test/parallel/test-sqlite-statement-sync.js index cf0e4daa45c..a55e19fd14f 100644 --- a/test/parallel/test-sqlite-statement-sync.js +++ b/test/parallel/test-sqlite-statement-sync.js @@ -79,6 +79,28 @@ suite('StatementSync.prototype.get()', () => { message: /statement has been finalized/, }); }); + + test('surfaces a deferred SQLite error from reset() even though a row was already built', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + PRAGMA foreign_keys = ON; + PRAGMA defer_foreign_keys = ON; + CREATE TABLE parent(id INTEGER PRIMARY KEY); + CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id)); + `); + // The FK check is deferred until the implicit transaction commits, which + // happens inside reset() here because RETURNING leaves the statement's + // VDBE running after the row is produced. + const stmt = db.prepare( + 'INSERT INTO child (parent_id) VALUES (999) RETURNING id' + ); + t.assert.throws(() => { + stmt.get(); + }, { + code: 'ERR_SQLITE_ERROR', + message: /FOREIGN KEY constraint failed/, + }); + }); }); suite('StatementSync.prototype.all()', () => { @@ -144,6 +166,25 @@ suite('StatementSync.prototype.all()', () => { message: /statement has been finalized/, }); }); + + test('surfaces a deferred SQLite error from reset() even though the array was already built', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + PRAGMA foreign_keys = ON; + PRAGMA defer_foreign_keys = ON; + CREATE TABLE parent(id INTEGER PRIMARY KEY); + CREATE TABLE child(id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id)); + `); + const stmt = db.prepare( + 'INSERT INTO child (parent_id) VALUES (999) RETURNING id' + ); + t.assert.throws(() => { + stmt.all(); + }, { + code: 'ERR_SQLITE_ERROR', + message: /FOREIGN KEY constraint failed/, + }); + }); }); suite('StatementSync.prototype.iterate()', () => { @@ -322,6 +363,41 @@ suite('StatementSync.prototype.iterate()', () => { message: /statement has been finalized/, }); }); + + test('does not replay results after the iterator is naturally exhausted', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE test(key TEXT); + INSERT INTO test (key) VALUES ('key1'); + `); + const it = db.prepare('SELECT * FROM test').iterate(); + t.assert.deepStrictEqual(it.next(), { + __proto__: null, done: false, value: { __proto__: null, key: 'key1' }, + }); + t.assert.deepStrictEqual( + it.next(), { __proto__: null, done: true, value: null }); + // Calling next() again on an exhausted iterator must keep reporting + // done, not silently reset the statement and replay from row 1. + t.assert.deepStrictEqual( + it.next(), { __proto__: null, done: true, value: null }); + }); + + test('propagates a pending exception when the loop body throws mid-iteration', (t) => { + using db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE test(key TEXT); + INSERT INTO test (key) VALUES ('key1'); + INSERT INTO test (key) VALUES ('key2'); + `); + const stmt = db.prepare('SELECT * FROM test'); + const userError = new Error('boom'); + t.assert.throws(() => { + // eslint-disable-next-line no-unused-vars + for (const row of stmt.iterate()) { + throw userError; + } + }, (err) => err === userError); + }); }); suite('StatementSync.prototype.run()', () => { From 1dd0a31cd0b059a318be888ba9b74ae0d9b3e269 Mon Sep 17 00:00:00 2001 From: semimikoh Date: Sun, 9 Aug 2026 20:32:44 +0900 Subject: [PATCH 3/4] sqlite: set done_ before the checked reset in Next() RESET_OR_THROW returns early on a deferred error, so the previous ordering skipped iter->done_ = true when the reset in the natural exhaustion path failed. The statement was reset either way, so setting done_ first is safe and avoids leaving a caught error in a resumable state. Signed-off-by: semimikoh --- src/node_sqlite.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 77ac1c28bf1..1cd9c18913c 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -3852,11 +3852,11 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { if (r != SQLITE_ROW) { CHECK_ERROR_OR_THROW( env->isolate(), iter->stmt_->db_.get(), r, SQLITE_DONE, void()); + iter->done_ = true; RESET_OR_THROW(env->isolate(), iter->stmt_->db_.get(), iter->stmt_->statement_, void()); - iter->done_ = true; MaybeLocal values[] = {Boolean::New(isolate, true), Null(isolate)}; Local result; if (NewDictionaryInstanceNullProto(env->context(), iter_template, values) From 90fe5e2b0d66822279a87ca4d2ddb86dcacb23fd Mon Sep 17 00:00:00 2001 From: semimikoh Date: Sun, 9 Aug 2026 22:00:57 +0900 Subject: [PATCH 4/4] sqlite: fix build after rebase onto upstream refactor Rebasing onto main picked up "sqlite: refactor error helpers and user function pointers", which moved SQLTagStore::All()'s reset and bind logic into ResetAndBindStatement() and removed the local Isolate* isolate declaration that this PR's trailing RESET_AND_CHECK call still depends on. Re-add it. Signed-off-by: semimikoh --- src/node_sqlite.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 1cd9c18913c..a41d25b89f0 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -3613,6 +3613,7 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { return; } + Isolate* isolate = env->isolate(); bool needs_reset = true; auto reset = OnScopeLeave([&]() { if (needs_reset) sqlite3_reset(stmt->statement_);