Fixes #11747 (#11829)

oven-sh · Jun 13, 2024 · 6c55ff6 · 6c55ff6
1 parent ba5dd63
commit 6c55ff6
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 17 deletions.
diff --git 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
@@ -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 {