bun:sqlite: fix stale Structure reads when a getter mutates the params object during bind

src/jsc/bindings/sqlite/JSSQLStatement.cpp

@@ -977,7 +977,6 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin
     };
 
     auto& vm = JSC::getVM(globalObject);
-    auto& structure = *target->structure();
     bindings.ensureNamesLoaded(vm, stmt);
     const auto& bindingNames = bindings.bindingNames;
     size_t size = bindings.count;
@@ -1012,7 +1011,8 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin
 
             const auto identifier = Identifier::fromString(vm, str);
             PropertySlot slot(target, PropertySlot::InternalMethodType::GetOwnProperty);
-            if (!target->getOwnNonIndexPropertySlot(vm, &structure, identifier, slot)) {
+            // Getters for earlier parameters can mutate the object, so the Structure must be re-read per lookup.
+            if (!target->getOwnNonIndexPropertySlot(vm, target->structure(), identifier, slot)) {
                 return {};
             }
 
@@ -1073,38 +1073,38 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin
             count++;
         }
     }
-    // Is it a simple object with no getters or setters?
+    // Named parameters, e.g.
     //
     // { foo: "bar", baz: "qux" }
     //
-    else if (target->canUseFastGetOwnProperty(structure)) {
+    else {
         for (size_t i = 0; i < size; i++) {
             const auto& property = bindingNames[i];
-            JSValue value = property.isEmpty() ? target->getDirectIndex(globalObject, i) : target->fastGetOwnProperty(vm, structure, bindingNames[i]);
-            if (!statementStillAlive())
-                return {};
-            if (!value && !scope.exception()) {
-                if (throwOnMissing) {
-                    throwException(globalObject, scope, createError(globalObject, makeString("Missing parameter \""_s, property.isEmpty() ? String::number(i) : property.string(), "\""_s)));
-                } else {
-                    continue;
+            JSValue value;
+            bool hasProperty = false;
+
+            // Getters for earlier parameters can mutate the object, so the Structure and fast-path check are re-done per parameter.
+            Structure* structure = target->structure();
+            if (property.isEmpty()) {
+                value = target->getDirectIndex(globalObject, i);
+                hasProperty = !!value;
+            } else if (target->canUseFastGetOwnProperty(*structure)) [[likely]] {
+                value = target->fastGetOwnProperty(vm, *structure, property);
+                hasProperty = !!value;
+            } else {
+                PropertySlot slot(target, PropertySlot::InternalMethodType::GetOwnProperty);
+                hasProperty = target->methodTable()->getOwnPropertySlot(target, globalObject, property, slot);
+                if (hasProperty && !scope.exception()) {
+                    if (!slot.isTaintedByOpaqueObject()) [[likely]]
+                        value = slot.getValue(globalObject, property);
+                    else
+                        value = target->get(globalObject, property);
                 }
             }
 
-            RETURN_IF_EXCEPTION(scope, {});
-
-            if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, safeIntegers)) {
+            if (!statementStillAlive())
                 return {};
-            }
 
-            RETURN_IF_EXCEPTION(scope, {});
-            count++;
-        }
-    } else {
-        for (size_t i = 0; i < size; i++) {
-            PropertySlot slot(target, PropertySlot::InternalMethodType::GetOwnProperty);
-            const auto& property = bindingNames[i];
-            bool hasProperty = property.isEmpty() ? target->methodTable()->getOwnPropertySlotByIndex(target, globalObject, i, slot) : target->methodTable()->getOwnPropertySlot(target, globalObject, property, slot);
             if (!hasProperty && !scope.exception()) {
                 if (throwOnMissing) {
                     throwException(globalObject, scope, createError(globalObject, makeString("Missing parameter \""_s, property.isEmpty() ? String::number(i) : property.string(), "\""_s)));
@@ -1115,19 +1115,6 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin
 
             RETURN_IF_EXCEPTION(scope, {});
 
-            JSValue value;
-            if (!slot.isTaintedByOpaqueObject()) [[likely]]
-                value = slot.getValue(globalObject, property);
-            else {
-                value = target->get(globalObject, property);
-                RETURN_IF_EXCEPTION(scope, {});
-            }
-
-            RETURN_IF_EXCEPTION(scope, {});
-
-            if (!statementStillAlive())
-                return {};
-
             if (!rebindValue(globalObject, db, stmt, i + 1, value, scope, safeIntegers)) {
                 return {};
             }

test/js/bun/sqlite/sqlite.test.js

@@ -189,6 +189,114 @@ describe("safeIntegers", () => {
   }
 }
 
+describe("bind parameters object mutated by a getter during bind", () => {
+  // A getter that runs while binding can add/delete properties, changing the
+  // object's layout mid-bind. Lookups for later parameters must see the
+  // current layout; reading through the old one binds whatever property now
+  // occupies the stale slot, or crashes on an empty slot.
+
+  it("strict mode: throws missing parameter instead of binding a foreign value", () => {
+    const db = new Database(":memory:", { strict: true });
+    const q = db.query("select ?1 as a, $b as b, $c as c");
+    const t = {};
+    t[0] = "i";
+    Object.defineProperty(t, "b", {
+      enumerable: true,
+      get() {
+        delete t.c;
+        t.secret = "SECRET-NOT-A-PARAM";
+        return "two";
+      },
+    });
+    t.c = "three";
+    expect(() => q.all(t)).toThrow('Missing parameter "$c"');
+  });
+
+  it("strict mode: binds the value the parameter holds at bind time", () => {
+    const db = new Database(":memory:", { strict: true });
+    const q = db.query("select ?1 as a, $b as b, $c as c");
+    const t = {};
+    t[0] = "i";
+    Object.defineProperty(t, "b", {
+      enumerable: true,
+      get() {
+        delete t.c;
+        t.filler = "FILLER";
+        t.c = "three-new";
+        return "two";
+      },
+    });
+    t.c = "three-old";
+    expect(q.all(t)).toEqual([{ a: "i", b: "two", c: "three-new" }]);
+  });
+
+  it("default mode: treats a parameter deleted by an index getter as missing", () => {
+    const db = new Database(":memory:");
+    const q = db.query("select ? as a, $c as c");
+    const u = { $c: "three" };
+    Object.defineProperty(u, 0, {
+      enumerable: true,
+      get() {
+        delete u.$c;
+        u.secret = "S";
+        return 0;
+      },
+    });
+    expect(q.all(u)).toEqual([{ a: 0, c: null }]);
+  });
+
+  it("default mode: calls a getter installed for a later parameter mid-bind", () => {
+    const db = new Database(":memory:");
+    const q = db.query("select ? as a, $c as c");
+    const u = {};
+    Object.defineProperty(u, 0, {
+      enumerable: true,
+      get() {
+        delete u.$c;
+        Object.defineProperty(u, "$c", { enumerable: true, get: () => "via-getter" });
+        return 0;
+      },
+    });
+    u.$c = "three";
+    expect(q.all(u)).toEqual([{ a: 0, c: "via-getter" }]);
+  });
+
+  it("does not crash when a getter deletes a later parameter", async () => {
+    // Crashed before the fix: the stale layout said $c was present, and the
+    // bind read an empty slot.
+    const code = `
+      import { Database } from "bun:sqlite";
+      const db = new Database(":memory:", { strict: true });
+      const q = db.query("select ?1 as a, $b as b, $c as c");
+      const t = {};
+      t[0] = "i";
+      Object.defineProperty(t, "b", {
+        enumerable: true,
+        get() {
+          delete t.c;
+          return "two";
+        },
+      });
+      t.c = "three";
+      try {
+        q.all(t);
+        console.log("no-error");
+      } catch (e) {
+        console.log("error: " + e.message);
+      }
+    `;
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "-e", code],
+      env: bunEnv,
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect(stderr).toBe("");
+    expect(stdout.trim()).toBe('error: Missing parameter "$c"');
+    expect(exitCode).toBe(0);
+  });
+});
+
 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.