From 2fae197fb7fcbccdd1b2bd9e0097e2e41f088d53 Mon Sep 17 00:00:00 2001 From: semimikoh Date: Fri, 15 May 2026 14:21:38 +0900 Subject: [PATCH 1/2] 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 1272deb42f1..9d977cbe63b 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)) { \ @@ -2989,9 +3000,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()); @@ -3065,10 +3084,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(); @@ -3076,7 +3101,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); @@ -3085,9 +3111,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); @@ -3100,9 +3126,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) { @@ -3119,8 +3148,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(), @@ -3128,6 +3159,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); } } @@ -3566,7 +3599,10 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { } } - 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(), @@ -3574,6 +3610,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); } } @@ -3800,7 +3838,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; @@ -3853,7 +3894,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 52592e6f59d8b8b89ff0cc16618f6b4d98760c92 Mon Sep 17 00:00:00 2001 From: semimikoh Date: Fri, 7 Aug 2026 10:49:28 +0900 Subject: [PATCH 2/2] 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 9d977cbe63b..8656f4b6532 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -3006,6 +3006,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(); @@ -3894,8 +3897,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()', () => {