From d3e10a8ff9f31a901879e06b42586c558fbda4eb Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Thu, 27 Aug 2026 11:55:29 -0400 Subject: [PATCH 1/2] sqlite: re-validate database state after reading options prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup() validated the connection, then read their options bag with Object::Get(). A property getter runs arbitrary JavaScript at that point, so a getter calling close() invalidates what was just checked. Five of the six then passed a null sqlite3* to SQLite and crashed; prepare() reported a spurious "out of memory". Re-check IsOpen() after option parsing, immediately before the SQLite call, keeping the early check so invalid calls still fail before any user code runs. IsOpen() is the only condition a getter can change: authorizer and callback depths are RAII-managed. createSession() already parsed options first, so it only gains the early check. deserialize() also latched the buffer length before reading options.dbName. A getter that shrank the backing store left the length too large; CopyContents() then handed the uninitialized remainder to SQLite, from where serialize() returned it to JavaScript. Check the CopyContents() result instead of discarding it. function() and aggregate() cast the callback's length property with As() and no IsInt32() guard. length is configurable, so any type reached the cast and produced a silently wrong arity. Fixes: https://github.com/nodejs/node/issues/65586 Signed-off-by: Trevor Burnham --- doc/api/sqlite.md | 6 +- src/node_sqlite.cc | 81 ++++- .../test-sqlite-options-getter-reentry.js | 302 ++++++++++++++++++ 3 files changed, 378 insertions(+), 11 deletions(-) create mode 100644 test/parallel/test-sqlite-options-getter-reentry.js diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 653051e8d670..c5b82552527a 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -243,7 +243,8 @@ Registers a new aggregate function with the SQLite database. This method is a wr JavaScript numbers. **Default:** `false`. * `varargs` {boolean} If `true`, `options.step` and `options.inverse` may be invoked with any number of arguments (between zero and [`SQLITE_MAX_FUNCTION_ARG`][]). If `false`, - `inverse` and `step` must be invoked with exactly `length` arguments. + `inverse` and `step` must be invoked with exactly `length` arguments, and + their `length` properties must be integers. **Default:** `false`. * `start` {number | string | null | Array | Object | Function} The identity value for the aggregation function. This value is used when the aggregation @@ -430,7 +431,8 @@ added: JavaScript numbers. **Default:** `false`. * `varargs` {boolean} If `true`, `function` may be invoked with any number of arguments (between zero and [`SQLITE_MAX_FUNCTION_ARG`][]). If `false`, - `function` must be invoked with exactly `function.length` arguments. + `function` must be invoked with exactly `function.length` arguments, which + must be an integer. **Default:** `false`. * `fn` {Function} The JavaScript function to call when the SQLite function is invoked. The return value of this function should be a valid SQLite data type: diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 05c4904fdd9e..87bf08c2a230 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1718,6 +1718,10 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo& args) { } } + // Reading the options bag above can run user JavaScript through a property + // getter, which may have closed the database since it was checked. + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + Utf8Value sql(env->isolate(), args[0].As()); sqlite3_stmt* s = nullptr; @@ -1905,9 +1909,22 @@ void DatabaseSync::CustomFunction(const FunctionCallbackInfo& args) { if (!fn->Get(env->context(), env->length_string()).ToLocal(&js_len)) { return; } + + if (!js_len->IsInt32()) { + THROW_ERR_INVALID_ARG_TYPE( + env->isolate(), + "The \"function.length\" property must be an integer."); + return; + } + argc = js_len.As()->Value(); } + // Reading the options bag and "function.length" above can run user + // JavaScript through a property getter, which may have closed the database + // since it was checked. + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + UserDefinedFunction* user_data = new UserDefinedFunction( env, fn, BaseObjectWeakPtr(db), use_bigint_args); int text_rep = SQLITE_UTF8; @@ -2070,6 +2087,10 @@ void DatabaseSync::Deserialize(const FunctionCallbackInfo& args) { } } + // Reading the options bag above can run user JavaScript through a property + // getter, which may have closed the database since it was checked. + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + // sqlite3_malloc64 is required because SQLITE_DESERIALIZE_FREEONCLOSE // transfers ownership to SQLite, which calls sqlite3_free() on close. // See: https://www.sqlite.org/c3ref/deserialize.html @@ -2080,7 +2101,16 @@ void DatabaseSync::Deserialize(const FunctionCallbackInfo& args) { return; } - input->CopyContents(buf, byte_length); + // The same user JavaScript may also have shrunk or detached the backing + // store, in which case byte_length is stale and CopyContents() leaves the + // remainder of buf uninitialized. Handing that to SQLite would disclose it + // through serialize(). + if (input->CopyContents(buf, byte_length) != byte_length) { + sqlite3_free(buf); + THROW_ERR_INVALID_STATE( + env, "The \"buffer\" argument was resized while reading \"options\""); + return; + } db->FinalizeStatements(); @@ -2218,17 +2248,37 @@ void DatabaseSync::AggregateFunction(const FunctionCallbackInfo& args) { return; } + if (!js_len->IsInt32()) { + THROW_ERR_INVALID_ARG_TYPE( + env->isolate(), + "The \"options.step.length\" property must be an integer."); + return; + } + // Subtract 1 because the first argument is the aggregate value. argc = js_len.As()->Value() - 1; - if (!inverseFunc.IsEmpty() && - !inverseFunc->Get(env->context(), env->length_string()) - .ToLocal(&js_len)) { - return; + if (!inverseFunc.IsEmpty()) { + if (!inverseFunc->Get(env->context(), env->length_string()) + .ToLocal(&js_len)) { + return; + } + + if (!js_len->IsInt32()) { + THROW_ERR_INVALID_ARG_TYPE( + env->isolate(), + "The \"options.inverse.length\" property must be an integer."); + return; + } } argc = std::max({argc, js_len.As()->Value() - 1, 0}); } + // Reading the options bag and the step/inverse "length" properties above can + // run user JavaScript through a property getter, which may have closed the + // database since it was checked. + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + int text_rep = SQLITE_UTF8; if (direct_only) { text_rep |= SQLITE_DIRECTONLY; @@ -2257,10 +2307,15 @@ void DatabaseSync::AggregateFunction(const FunctionCallbackInfo& args) { } void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { + DatabaseSync* db; + ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); + Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); + std::string table; std::string db_name = "main"; - Environment* env = Environment::GetCurrent(args); if (args.Length() > 0) { if (!args[0]->IsObject()) { THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -2311,10 +2366,9 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { } } - DatabaseSync* db; - ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); + // Reading the options bag above can run user JavaScript through a property + // getter, which may have closed the database since it was checked. THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); - THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); sqlite3_session* pSession; int r = @@ -2441,6 +2495,11 @@ void Backup(const FunctionCallbackInfo& args) { } } + // Reading the destination path and the options bag above can run user + // JavaScript through a property getter, which may have closed the database + // since it was checked. + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + Local resolver; if (!Promise::Resolver::New(env->context()).ToLocal(&resolver)) { return; @@ -2584,6 +2643,10 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { } } + // Reading the options bag above can run user JavaScript through a property + // getter, which may have closed the database since it was checked. + THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + // Keep the database alive during sqlite3changeset_apply(), which may // call conflict or filter callbacks that trigger JavaScript execution. // If the JavaScript callback drops all references to the database, diff --git a/test/parallel/test-sqlite-options-getter-reentry.js b/test/parallel/test-sqlite-options-getter-reentry.js new file mode 100644 index 000000000000..4d4e593caf01 --- /dev/null +++ b/test/parallel/test-sqlite-options-getter-reentry.js @@ -0,0 +1,302 @@ +'use strict'; +require('../common'); +const tmpdir = require('../common/tmpdir'); +const { join } = require('node:path'); +const { backup, DatabaseSync } = require('node:sqlite'); +const { suite, test } = require('node:test'); + +tmpdir.refresh(); + +const invalidState = { + code: 'ERR_INVALID_STATE', + message: /database is not open/, +}; + +// A property getter on the options bag runs arbitrary JavaScript in the middle +// of the call, so state validated before the options were read can be stale by +// the time it is used. +suite('closing the database from an options getter', () => { + test('prepare() throws instead of using a closed connection', (t) => { + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + db.prepare('SELECT 1', { + get returnArrays() { + db.close(); + return false; + }, + }); + }, invalidState); + }); + + test('function() throws instead of using a closed connection', (t) => { + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + db.function('fn', { + get useBigIntArguments() { + db.close(); + return false; + }, + }, () => 1); + }, invalidState); + }); + + test('function() throws when the length getter closes the database', (t) => { + const db = new DatabaseSync(':memory:'); + const fn = () => 1; + Object.defineProperty(fn, 'length', { + configurable: true, + get() { + db.close(); + return 0; + }, + }); + t.assert.throws(() => { + db.function('fn', fn); + }, invalidState); + }); + + test('aggregate() throws instead of using a closed connection', (t) => { + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + db.aggregate('agg', { + get start() { + db.close(); + return 0; + }, + step: (acc, value) => acc, + }); + }, invalidState); + }); + + test('aggregate() throws when the length getter closes the database', (t) => { + const db = new DatabaseSync(':memory:'); + const step = (acc, value) => acc; + Object.defineProperty(step, 'length', { + configurable: true, + get() { + db.close(); + return 2; + }, + }); + t.assert.throws(() => { + db.aggregate('agg', { start: 0, step }); + }, invalidState); + }); + + test('deserialize() throws instead of using a closed connection', (t) => { + const source = new DatabaseSync(':memory:'); + source.exec('CREATE TABLE data(value TEXT)'); + const image = source.serialize(); + + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + db.deserialize(image, { + get dbName() { + db.close(); + return 'main'; + }, + }); + }, invalidState); + }); + + test('createSession() throws instead of using a closed connection', (t) => { + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + db.createSession({ + get db() { + db.close(); + return 'main'; + }, + }); + }, invalidState); + }); + + test('applyChangeset() throws instead of using a closed connection', (t) => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)'); + const session = db.createSession(); + db.exec('INSERT INTO data (key) VALUES (1)'); + const changeset = session.changeset(); + + t.assert.throws(() => { + db.applyChangeset(changeset, { + get onConflict() { + db.close(); + return undefined; + }, + }); + }, invalidState); + }); + + test('backup() throws instead of using a closed connection', (t) => { + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + backup(db, join(tmpdir.path, 'getter-backup.db'), { + get rate() { + db.close(); + return 1; + }, + }); + }, invalidState); + }); +}); + +// The state check runs before the options bag is read, so a call that is +// already doomed must not execute any of the caller's getters. +test('options getters do not run on an already-closed database', (t) => { + const source = new DatabaseSync(':memory:'); + source.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)'); + const image = source.serialize(); + const session = source.createSession(); + source.exec('INSERT INTO data (key) VALUES (1)'); + const changeset = session.changeset(); + + const cases = { + prepare: (db, options) => db.prepare('SELECT 1', options), + function: (db, options) => db.function('fn', options, () => 1), + aggregate: (db, options) => db.aggregate('agg', options), + deserialize: (db, options) => db.deserialize(image, options), + createSession: (db, options) => db.createSession(options), + applyChangeset: (db, options) => db.applyChangeset(changeset, options), + backup: (db, options) => + backup(db, join(tmpdir.path, 'closed-backup.db'), options), + }; + + // The property each method reads first, and a valid value for it, so that a + // getter which does run leaves the "did not run" assertion as the failure + // rather than a type error from the returned value. + const probes = { + prepare: ['returnArrays', false], + function: ['useBigIntArguments', false], + aggregate: ['start', 0], + deserialize: ['dbName', 'main'], + createSession: ['db', 'main'], + applyChangeset: ['onConflict', undefined], + backup: ['rate', 1], + }; + + for (const [name, invoke] of Object.entries(cases)) { + const db = new DatabaseSync(':memory:'); + db.close(); + + const [key, value] = probes[name]; + let ran = false; + const options = { + get [key]() { + ran = true; + return value; + }, + }; + + t.assert.throws(() => invoke(db, options), invalidState, name); + t.assert.strictEqual(ran, false, `${name} ran an options getter`); + } +}); + +suite('resizing a deserialize() buffer from an options getter', () => { + test('throws rather than handing uninitialized memory to SQLite', (t) => { + const source = new DatabaseSync(':memory:'); + source.exec('CREATE TABLE data(value TEXT)'); + source.prepare('INSERT INTO data (value) VALUES (?)').run('hello'); + const image = source.serialize(); + + const buffer = new ArrayBuffer(image.byteLength, { + maxByteLength: image.byteLength, + }); + new Uint8Array(buffer).set(image); + + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + db.deserialize(new Uint8Array(buffer), { + get dbName() { + buffer.resize(1024); + return 'main'; + }, + }); + }, { + code: 'ERR_INVALID_STATE', + message: /"buffer" argument was resized/, + }); + }); + + test('throws when the buffer is detached', (t) => { + const source = new DatabaseSync(':memory:'); + source.exec('CREATE TABLE data(value TEXT)'); + const image = source.serialize(); + + const buffer = new ArrayBuffer(image.byteLength); + new Uint8Array(buffer).set(image); + + const db = new DatabaseSync(':memory:'); + t.assert.throws(() => { + db.deserialize(new Uint8Array(buffer), { + get dbName() { + structuredClone(buffer, { transfer: [buffer] }); + return 'main'; + }, + }); + }, { + code: 'ERR_INVALID_STATE', + message: /"buffer" argument was resized/, + }); + }); +}); + +// fn.length is configurable, so it must be validated rather than cast blindly. +suite('non-integer callback length', () => { + const badLengths = ['abc', {}, [], null, undefined, NaN, 1.5, Symbol.iterator, + 10n, true, 2 ** 40]; + + test('function() rejects a non-integer length', (t) => { + const db = new DatabaseSync(':memory:'); + for (const value of badLengths) { + const fn = () => 1; + Object.defineProperty(fn, 'length', { configurable: true, value }); + t.assert.throws(() => { + db.function('fn', fn); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "function\.length" property must be an integer/, + }, `length=${String(value)}`); + } + }); + + test('aggregate() rejects a non-integer step length', (t) => { + const db = new DatabaseSync(':memory:'); + for (const value of badLengths) { + const step = (acc, next) => acc; + Object.defineProperty(step, 'length', { configurable: true, value }); + t.assert.throws(() => { + db.aggregate('agg', { start: 0, step, result: (acc) => acc }); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "options\.step\.length" property must be an integer/, + }, `length=${String(value)}`); + } + }); + + test('aggregate() rejects a non-integer inverse length', (t) => { + const db = new DatabaseSync(':memory:'); + for (const value of badLengths) { + const inverse = (acc, next) => acc; + Object.defineProperty(inverse, 'length', { configurable: true, value }); + t.assert.throws(() => { + db.aggregate('agg', { + start: 0, + step: (acc, next) => acc, + inverse, + result: (acc) => acc, + }); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "options\.inverse\.length" property must be an integer/, + }, `length=${String(value)}`); + } + }); + + test('a normal function length is still accepted', (t) => { + const db = new DatabaseSync(':memory:'); + db.function('plus', (a, b) => a + b); + t.assert.strictEqual(db.prepare('SELECT plus(1, 2) AS v').get().v, 3); + }); +}); From 8f7d36df6f5aecf343dd8da41f0a532281b338ef Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Thu, 27 Aug 2026 14:11:51 -0400 Subject: [PATCH 2/2] fixup! sqlite: re-validate database state after reading options Add the skipIfSQLiteMissing() guard. Builds configured with shared libraries omit node:sqlite, so requiring it unconditionally failed the x86_64-darwin and aarch64-darwin shared-library CI jobs. Signed-off-by: Trevor Burnham --- test/parallel/test-sqlite-options-getter-reentry.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-sqlite-options-getter-reentry.js b/test/parallel/test-sqlite-options-getter-reentry.js index 4d4e593caf01..495b85db92fb 100644 --- a/test/parallel/test-sqlite-options-getter-reentry.js +++ b/test/parallel/test-sqlite-options-getter-reentry.js @@ -1,5 +1,6 @@ 'use strict'; -require('../common'); +const { skipIfSQLiteMissing } = require('../common'); +skipIfSQLiteMissing(); const tmpdir = require('../common/tmpdir'); const { join } = require('node:path'); const { backup, DatabaseSync } = require('node:sqlite');