FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: reject statement-less SQL in prepare() · nodejs/node@5e387ce · GitHub

/ node Public

Commit 5e387ce

Browse files
TrevorBurnham
authored and
aduh95
committed
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: #65157 (comment) Refs: https://sqlite.org/wasm/doc/trunk/api-oo1.md Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> PR-URL: #65157 Fixes: #65149 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
1 parent 83169e5 commit 5e387ce

3 files changed

Lines changed: 42 additions & 3 deletions

File tree

‎doc/api/sqlite.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -662,6 +662,10 @@ console.log(query.get());
662662

663663
<!-- YAML
664664
added: v22.5.0
665+
changes:
666+
- version: REPLACEME
667+
pr-url: https://github.com/nodejs/node/pull/65157
668+
description: Throw `ERR_INVALID_ARG_VALUE` if `sql` contains no statements.
665669
-->
666670

667671
* `sql` {string} A SQL string to compile to a prepared statement.

‎src/node_sqlite.cc‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1581,6 +1581,16 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo<Value>& args) {
15811581
int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, nullptr);
15821582

15831583
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
1584+
1585+
// sqlite3_prepare_v2() reports success without producing a statement when
1586+
// the input holds no SQL, such as a comment. Such a statement can never be
1587+
// stepped, and tracking it would leave a dangling pointer in statements_
1588+
// because its destructor treats a null statement as already finalized.
1589+
if (s == nullptr) {
1590+
THROW_ERR_INVALID_ARG_VALUE(env, "The SQL query contains no statements.");
1591+
return;
1592+
}
1593+
15841594
BaseObjectPtr<StatementSync> stmt =
15851595
StatementSync::Create(env, BaseObjectPtr<DatabaseSync>(db), s);
15861596
db->statements_.insert(stmt.get());
@@ -3659,9 +3669,8 @@ BaseObjectPtr<StatementSync> SQLTagStore::PrepareStatement(
36593669
return BaseObjectPtr<StatementSync>();
36603670
}
36613671

3662-
// sqlite3_prepare_v2() reports success without producing a statement when
3663-
// the input holds no SQL, such as a comment. Such a statement cannot be
3664-
// bound or executed, so reject it instead of caching it.
3672+
// As in DatabaseSync::Prepare(), reject input that holds no SQL rather
3673+
// than caching a statement that can never be bound or stepped.
36653674
if (s == nullptr) {
36663675
THROW_ERR_INVALID_ARG_VALUE(env, "The SQL query contains no statements.");
36673676
return BaseObjectPtr<StatementSync>();

‎test/parallel/test-sqlite-database-sync.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,32 @@ suite('DatabaseSync.prototype.prepare()', () => {
397397
message: /The "sql" argument must be a string/,
398398
});
399399
});
400+
401+
test('throws if sql contains no statements', (t) => {
402+
using db = new DatabaseSync(nextDb());
403+
404+
for (const sql of ['', ' ', ';', '-- comment', '/* comment */']) {
405+
t.assert.throws(() => {
406+
db.prepare(sql);
407+
}, {
408+
code: 'ERR_INVALID_ARG_VALUE',
409+
message: /contains no statements/,
410+
});
411+
}
412+
});
413+
414+
test('prepares statements that contain comments', (t) => {
415+
using db = new DatabaseSync(nextDb());
416+
const queries = [
417+
'-- lead\nSELECT 1 AS v',
418+
'SELECT 1 AS v -- trail',
419+
'SELECT /* mid */ 1 AS v',
420+
];
421+
422+
for (const sql of queries) {
423+
t.assert.strictEqual(db.prepare(sql).get().v, 1);
424+
}
425+
});
400426
});
401427

402428
suite('DatabaseSync.prototype.exec()', () => {

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL