From 4c2c3a4a5edaa2dc1fed473d8c81e6322e5b40d2 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Sat, 8 Aug 2026 20:54:22 -0400 Subject: [PATCH 1/2] sqlite: reject statement-less SQL in SQLTagStore sqlite3_prepare_v2() returns SQLITE_OK without producing a statement when its input holds no SQL, such as a comment. PrepareStatement() only checked the return code, so it cached a StatementSync wrapping a null sqlite3_stmt. Executing it reached sqlite3_clear_bindings(), which only guards against a null statement under SQLITE_ENABLE_API_ARMOR, and segfaulted. Reject such input instead of caching it. The StatementSync methods already avoid the crash because their IsFinalized() guard treats a null statement as finalized. Fixes: https://github.com/nodejs/node/issues/65149 Signed-off-by: Trevor Burnham --- src/node_sqlite.cc | 8 ++++++++ test/parallel/test-sqlite-template-tag.js | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 80da5bc0d9bf..76a711942e18 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -3658,6 +3658,14 @@ BaseObjectPtr SQLTagStore::PrepareStatement( return BaseObjectPtr(); } + // sqlite3_prepare_v2() reports success without producing a statement when + // the input holds no SQL, such as a comment. Such a statement cannot be + // bound or executed, so reject it instead of caching it. + if (s == nullptr) { + THROW_ERR_INVALID_ARG_VALUE(env, "The SQL query contains no statements."); + return BaseObjectPtr(); + } + BaseObjectPtr stmt_obj = StatementSync::Create( env, BaseObjectPtr(session->database_), s); diff --git a/test/parallel/test-sqlite-template-tag.js b/test/parallel/test-sqlite-template-tag.js index 445231bef0bd..20376e199d1b 100644 --- a/test/parallel/test-sqlite-template-tag.js +++ b/test/parallel/test-sqlite-template-tag.js @@ -317,6 +317,29 @@ test('sql error messages are descriptive', () => { }); }); +test('rejects SQL that contains no statements', () => { + const expectedError = { + code: 'ERR_INVALID_ARG_VALUE', + message: /contains no statements/, + }; + + for (const method of ['run', 'get', 'all', 'iterate']) { + assert.throws(() => { + // eslint-disable-next-line no-unused-expressions + sql[method]`-- comment`; + }, expectedError); + + assert.throws(() => { + // eslint-disable-next-line no-unused-expressions + sql[method]``; + }, expectedError); + } + + // A rejected statement must not be cached, so a later valid query with the + // same tag store still works. + assert.strictEqual(sql.run`INSERT INTO foo (text) VALUES (${'bob'})`.changes, 1); +}); + test('a tag store keeps the database alive by itself', () => { const sql = new DatabaseSync(':memory:').createTagStore(); From d7ad352a1856a3a58adc4e9ee96dff5ed94018e8 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Sun, 9 Aug 2026 11:48:27 -0400 Subject: [PATCH 2/2] sqlite: reject statement-less SQL in prepare() Apply the same check to DatabaseSync::Prepare() so that statement-less SQL is rejected at preparation instead of on first use. This matches SQLite's own oo1 JavaScript API, which throws when the SQL contains no statements rather than exposing the C API's null statement pointer. Previously db.prepare('-- comment') returned a StatementSync whose statement_ was null. Every method on it threw "statement has been finalized", which was misleading because nothing had been finalized, and the object was still inserted into statements_. Since IsFinalized() is true for a null statement, its destructor skipped UntrackStatement() and left a dangling pointer in the set that a later close() would finalize. Refs: https://github.com/nodejs/node/pull/65157#discussion_r3742903347 Refs: https://sqlite.org/wasm/doc/trunk/api-oo1.md Signed-off-by: Trevor Burnham --- doc/api/sqlite.md | 4 ++++ src/node_sqlite.cc | 15 ++++++++++--- test/parallel/test-sqlite-database-sync.js | 26 ++++++++++++++++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index ae194ff2acaf..49c1260708d3 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -670,6 +670,10 @@ console.log(query.get()); * `sql` {string} A SQL string to compile to a prepared statement. diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 76a711942e18..57cf621f13ee 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1581,6 +1581,16 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo& args) { int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, nullptr); CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void()); + + // sqlite3_prepare_v2() reports success without producing a statement when + // the input holds no SQL, such as a comment. Such a statement can never be + // stepped, and tracking it would leave a dangling pointer in statements_ + // because its destructor treats a null statement as already finalized. + if (s == nullptr) { + THROW_ERR_INVALID_ARG_VALUE(env, "The SQL query contains no statements."); + return; + } + BaseObjectPtr stmt = StatementSync::Create(env, BaseObjectPtr(db), s); db->statements_.insert(stmt.get()); @@ -3658,9 +3668,8 @@ BaseObjectPtr SQLTagStore::PrepareStatement( return BaseObjectPtr(); } - // sqlite3_prepare_v2() reports success without producing a statement when - // the input holds no SQL, such as a comment. Such a statement cannot be - // bound or executed, so reject it instead of caching it. + // As in DatabaseSync::Prepare(), reject input that holds no SQL rather + // than caching a statement that can never be bound or stepped. if (s == nullptr) { THROW_ERR_INVALID_ARG_VALUE(env, "The SQL query contains no statements."); return BaseObjectPtr(); diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index af7677a3cfc0..08a636c9cbdc 100644 --- a/test/parallel/test-sqlite-database-sync.js +++ b/test/parallel/test-sqlite-database-sync.js @@ -397,6 +397,32 @@ suite('DatabaseSync.prototype.prepare()', () => { message: /The "sql" argument must be a string/, }); }); + + test('throws if sql contains no statements', (t) => { + using db = new DatabaseSync(nextDb()); + + for (const sql of ['', ' ', ';', '-- comment', '/* comment */']) { + t.assert.throws(() => { + db.prepare(sql); + }, { + code: 'ERR_INVALID_ARG_VALUE', + message: /contains no statements/, + }); + } + }); + + test('prepares statements that contain comments', (t) => { + using db = new DatabaseSync(nextDb()); + const queries = [ + '-- lead\nSELECT 1 AS v', + 'SELECT 1 AS v -- trail', + 'SELECT /* mid */ 1 AS v', + ]; + + for (const sql of queries) { + t.assert.strictEqual(db.prepare(sql).get().v, 1); + } + }); }); suite('DatabaseSync.prototype.exec()', () => {