From 6c55ff6008884e84149f9d7bf01338aef006bb35 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 12 Jun 2024 19:59:53 -0700 Subject: [PATCH] Fixes #11747 (#11829) --- src/bun.js/bindings/sqlite/JSSQLStatement.cpp | 44 ++++++++------ test/js/bun/sqlite/sqlite.test.js | 58 +++++++++++++++++++ 2 files changed, 85 insertions(+), 17 deletions(-) diff --git a/src/bun.js/bindings/sqlite/JSSQLStatement.cpp b/src/bun.js/bindings/sqlite/JSSQLStatement.cpp index ae33fc767581b1..d33732e5d8d15e 100644 --- a/src/bun.js/bindings/sqlite/JSSQLStatement.cpp +++ b/src/bun.js/bindings/sqlite/JSSQLStatement.cpp @@ -1599,23 +1599,30 @@ static inline JSC::JSValue constructResultObject(JSC::JSGlobalObject* lexicalGlo return JSValue(result); } -static inline JSC::JSArray* constructResultRow(JSC::JSGlobalObject* lexicalGlobalObject, JSSQLStatement* castedThis, ObjectInitializationScope& scope, JSC::GCDeferralContext* deferralContext) +static inline JSC::JSArray* constructResultRow(JSC::VM& vm, JSC::JSGlobalObject* lexicalGlobalObject, JSSQLStatement* castedThis, size_t columnCount) { - int count = castedThis->columnNames->size(); - auto& vm = lexicalGlobalObject->vm(); auto throwScope = DECLARE_THROW_SCOPE(vm); - - JSC::JSArray* result = JSArray::create(vm, lexicalGlobalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), count); auto* stmt = castedThis->stmt; - for (int i = 0, j = 0; j < count; i++, j++) { - if (!castedThis->validColumns.get(i)) { - j -= 1; - continue; - } + MarkedArgumentBuffer arguments; + arguments.ensureCapacity(columnCount); + for (size_t i = 0; i < columnCount; i++) { JSValue value = toJS(vm, lexicalGlobalObject, stmt, i); RETURN_IF_EXCEPTION(throwScope, nullptr); - result->putDirectIndex(lexicalGlobalObject, j, value); + arguments.append(value); + } + + JSC::ObjectInitializationScope initializationScope(vm); + Structure* arrayStructure = lexicalGlobalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous); + JSC::JSArray* result; + + if (LIKELY(result = JSC::JSArray::tryCreateUninitializedRestricted(initializationScope, arrayStructure, columnCount))) { + for (size_t i = 0; i < columnCount; i++) { + result->initializeIndex(initializationScope, i, arguments.at(i)); + } + } else { + RETURN_IF_EXCEPTION(throwScope, nullptr); + result = JSC::constructArray(lexicalGlobalObject, arrayStructure, arguments); } return result; @@ -1833,17 +1840,20 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteStatementFunctionRows, (JSC::JSGlo result = jsNumber(sqlite3_column_count(stmt)); } else { - JSC::ObjectInitializationScope initializationScope(vm); - JSC::GCDeferralContext deferralContext(vm); JSC::JSArray* resultArray = JSC::constructEmptyArray(lexicalGlobalObject, nullptr, 0); { - - while (status == SQLITE_ROW) { - JSC::JSValue row = constructResultRow(lexicalGlobalObject, castedThis, initializationScope, &deferralContext); + size_t columnCount = sqlite3_column_count(stmt); + + do { + JSC::JSArray* row = constructResultRow(vm, lexicalGlobalObject, castedThis, columnCount); + if (UNLIKELY(!row || scope.exception())) { + sqlite3_reset(stmt); + RELEASE_AND_RETURN(scope, {}); + } resultArray->push(lexicalGlobalObject, row); status = sqlite3_step(stmt); - } + } while (status == SQLITE_ROW); } result = resultArray; diff --git a/test/js/bun/sqlite/sqlite.test.js b/test/js/bun/sqlite/sqlite.test.js index 60a9d01b1cdeed..dac26cb6ab5dce 100644 --- a/test/js/bun/sqlite/sqlite.test.js +++ b/test/js/bun/sqlite/sqlite.test.js @@ -10,6 +10,64 @@ const tmpbase = tmpdir() + path.sep; var encode = text => new TextEncoder().encode(text); +// Use different numbers of columns to ensure we crash if using initializeIndex() on a large array can cause bugs. +// https://github.com/oven-sh/bun/issues/11747 +it.each([1, 16, 256, 512, 768])("should work with duplicate columns in values() of length %d", columnCount => { + const db = new Database(":memory:"); + + db.prepare( + `create table \`users\` ( id integer primary key autoincrement, name text, reportTo integer, ${Array.from( + { + length: columnCount, + }, + (_, i) => `column${i} text DEFAULT "make GC happen!!" NOT NULL${i === columnCount - 1 ? "" : ","}`, + ).join("")} );`, + ).run(); + const names = [ + ["dan", null], + ["alef", 1], + ["bob", 2], + ["carl", 3], + ["dave", 4], + ["eve", 5], + ["fred", 6], + ["george", 7], + ["harry", 8], + ["isaac", 9], + ["jacob", 10], + ["kevin", 11], + ["larry", 12], + ["mike", 13], + ["nathan", 14], + ["oscar", 15], + ["peter", 16], + ["qwerty", 17], + ["robert", 18], + ["samuel", 19], + ["tom", 20], + ["william", 21], + ["xavier", 22], + ["yanny", 23], + ["zachary", 24], + ]; + for (const [name, reportTo] of names) { + db.prepare("insert into `users` (name, reportTo) values (?, ?);").run(name, reportTo); + } + const results = db + .prepare("select * from 'users' left join 'users' reportee on `users`.id = reportee.reportTo; ") + .values(); + expect(results).toHaveLength(names.length); + expect(results[0]).toHaveLength((columnCount + 3) * 2); + let prevResult; + for (let result of results) { + expect(result).toHaveLength((columnCount + 3) * 2); + if (prevResult) { + expect(prevResult.slice(columnCount + 3, (columnCount + 3) * 2)).toEqual(result.slice(0, columnCount + 3)); + } + prevResult = result; + } +}); + it("Database.open", () => { // in a folder which doesn't exist try {