diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 57a0007baab..c8d19216ffa 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -668,6 +668,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 e3604d97bc5..e80fc7db0c5 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1575,6 +1575,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()); @@ -3648,6 +3658,13 @@ BaseObjectPtr SQLTagStore::PrepareStatement( return BaseObjectPtr(); } + // 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(); + } + BaseObjectPtr stmt_obj = StatementSync::Create( env, BaseObjectPtr(session->database_), s); diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index af7677a3cfc..08a636c9cbd 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()', () => { diff --git a/test/parallel/test-sqlite-template-tag.js b/test/parallel/test-sqlite-template-tag.js index 445231bef0b..20376e199d1 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();