Skip to content

Commit 54867cd

Browse files
trivikrrichardlau
trivikr
authored and
richardlau
committed
sqlite: throw on oversized strings instead of aborting
Several conversions from SQLite text to V8 strings passed a length of -1 to String::NewFromUtf8(). With that length, V8 skips its length check and aborts the process when the UTF-8 text exceeds String::kMaxLength. SQLite error messages embed the offending identifier or token, so CreateSQLiteErrorImpl() could hit this. NullableSQLiteStringToValue(), used for setAuthorizer() callback arguments and statement.columns() metadata, had the same problem, and the authorizer also called ToLocalChecked() on each argument. Convert error messages with Utf8StringMaybeOneByte(), and check the length up front in NullableSQLiteStringToValue(), so oversized strings throw ERR_STRING_TOO_LONG as column values already do. In the authorizer, deny the action and let the pending error reach the caller. Signed-off-by: Trivikram Kamat <[email protected]> Assisted-by: claude:opus-5.5 PR-URL: #66521 Fixes: #66520 Fixes: #66522 Reviewed-By: Guilherme Araújo <[email protected]> Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 1e8247a commit 54867cd

2 files changed

Lines changed: 50 additions & 12 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,9 @@ MaybeLocal<Object> CreateSQLiteErrorImpl(Isolate* isolate,
284284
Local<Context> context = isolate->GetCurrentContext();
285285
Local<String> js_msg;
286286
Local<Object> e;
287-
if (!String::NewFromUtf8(isolate, message).ToLocal(&js_msg) ||
287+
// SQLite error messages embed the offending identifier or token, so they
288+
// can exceed String::kMaxLength.
289+
if (!Utf8StringMaybeOneByte(isolate, message).ToLocal(&js_msg) ||
288290
!Exception::Error(js_msg)->ToObject(context).ToLocal(&e) ||
289291
e->Set(context, env->code_string(), env->err_sqlite_error_string())
290292
.IsNothing()) {
@@ -399,7 +401,15 @@ inline MaybeLocal<Value> NullableSQLiteStringToValue(Isolate* isolate,
399401
return Null(isolate);
400402
}
401403

402-
return String::NewFromUtf8(isolate, str, NewStringType::kInternalized)
404+
// With the default length of -1, V8 aborts on strings over kMaxLength.
405+
const size_t len = strlen(str);
406+
if (len > static_cast<size_t>(String::kMaxLength)) [[unlikely]] {
407+
isolate->ThrowException(node::ERR_STRING_TOO_LONG(isolate));
408+
return MaybeLocal<Value>();
409+
}
410+
411+
return String::NewFromUtf8(
412+
isolate, str, NewStringType::kInternalized, static_cast<int>(len))
403413
.As<Value>();
404414
}
405415

@@ -3631,15 +3641,15 @@ int Database::AuthorizerCallback(void* user_data,
36313641

36323642
Local<Function> callback = cb.As<Function>();
36333643

3634-
LocalVector<Value> js_argv(
3635-
isolate,
3636-
{
3637-
Integer::New(isolate, action_code),
3638-
NullableSQLiteStringToValue(isolate, param1).ToLocalChecked(),
3639-
NullableSQLiteStringToValue(isolate, param2).ToLocalChecked(),
3640-
NullableSQLiteStringToValue(isolate, param3).ToLocalChecked(),
3641-
NullableSQLiteStringToValue(isolate, param4).ToLocalChecked(),
3642-
});
3644+
LocalVector<Value> js_argv(isolate, {Integer::New(isolate, action_code)});
3645+
for (const char* param : {param1, param2, param3, param4}) {
3646+
Local<Value> arg;
3647+
if (!NullableSQLiteStringToValue(isolate, param).ToLocal(&arg)) {
3648+
db->SetIgnoreNextSQLiteError(true);
3649+
return SQLITE_DENY;
3650+
}
3651+
js_argv.push_back(arg);
3652+
}
36433653

36443654
MaybeLocal<Value> retval = callback->Call(
36453655
context, Undefined(isolate), js_argv.size(), js_argv.data());

‎test/parallel/test-sqlite-statement.js‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
'use strict';
33
const { enoughTestMem, skipIfSQLiteMissing } = require('../common');
44
skipIfSQLiteMissing();
5-
const { Database, Statement } = require('node:sqlite');
5+
const {
6+
Database,
7+
Statement,
8+
constants: { SQLITE_OK },
9+
} = require('node:sqlite');
610
const { constants } = require('node:buffer');
711
const { suite, test } = require('node:test');
812

@@ -1448,6 +1452,9 @@ suite('values larger than the maximum string length', { skip: !enoughTestMem },
14481452
// hex() doubles its input, so this is the smallest blob whose text form
14491453
// exceeds what V8 can hold in a string.
14501454
const blobSize = (constants.MAX_STRING_LENGTH >>> 1) + 1;
1455+
// '\u20ac' is 1 UTF-16 code unit but 3 UTF-8 bytes, so this identifier is a
1456+
// valid JS string whose UTF-8 form exceeds the limit.
1457+
const longIdentifier = '\u20ac'.repeat(Math.ceil(constants.MAX_STRING_LENGTH / 3) + 10);
14511458
const tooLong = { code: 'ERR_STRING_TOO_LONG', name: 'Error' };
14521459

14531460
test('get() throws instead of returning undefined', (t) => {
@@ -1458,6 +1465,27 @@ suite('values larger than the maximum string length', { skip: !enoughTestMem },
14581465
}, tooLong);
14591466
});
14601467

1468+
test('prepare() throws when the SQLite error message is too long', (t) => {
1469+
using db = new Database(':memory:');
1470+
// SQLite repeats the identifier in the error.
1471+
t.assert.throws(() => {
1472+
db.prepare(`SELECT 1 FROM "${longIdentifier}"`);
1473+
}, tooLong);
1474+
});
1475+
1476+
test('prepare() throws for an oversized authorizer argument', (t) => {
1477+
using db = new Database(':memory:');
1478+
db.setAuthorizer(() => SQLITE_OK);
1479+
t.assert.throws(() => db.prepare(`CREATE TABLE "${longIdentifier}" (x)`), tooLong);
1480+
});
1481+
1482+
test('columns() throws for an oversized declared type', (t) => {
1483+
using db = new Database(':memory:');
1484+
db.exec(`CREATE TABLE t (x "${longIdentifier}")`);
1485+
using stmt = db.prepare('SELECT x FROM t');
1486+
t.assert.throws(() => stmt.columns(), tooLong);
1487+
});
1488+
14611489
test('exec() surfaces the error from a user-defined function', (t) => {
14621490
using db = new Database(':memory:');
14631491
db.exec('CREATE TABLE data(val TEXT)');

0 commit comments

Comments
 (0)