Fix Object.defineProperties evaluation order

quickjs.c

@@ -41246,38 +41246,64 @@ static __exception int JS_ObjectDefineProperties(JSContext *ctx,
     JSValue props, desc;
     JSObject *p;
     JSPropertyEnum *atoms;
-    uint32_t len, i;
-    int ret = -1;
+    struct {
+        JSAtom atom; /* owned by atoms */
+        JSPropertyDescriptor desc;
+    } *tab;
+    uint32_t len, i, count;
+    int ret = -1, res, desc_flags;
 
     if (!JS_IsObject(obj)) {
         JS_ThrowTypeError(ctx, "Object.defineProperties called on non-object");
         return -1;
     }
-    desc = JS_UNDEFINED;
     props = JS_ToObject(ctx, properties);
     if (JS_IsException(props))
         return -1;
     p = JS_VALUE_GET_OBJ(props);
-    if (JS_GetOwnPropertyNamesInternal(ctx, &atoms, &len, p, JS_GPN_ENUM_ONLY | JS_GPN_STRING_MASK | JS_GPN_SYMBOL_MASK) < 0)
+    tab = NULL;
+    count = 0;
+    if (JS_GetOwnPropertyNamesInternal(ctx, &atoms, &len, p, JS_GPN_STRING_MASK | JS_GPN_SYMBOL_MASK) < 0)
         goto exception;
-    // XXX: ECMA specifies that all descriptions should be validated before
-    //      modifying the object. This would require allocating an array
-    //      JSPropertyDescriptor and use 2 separate loops.
+    if (len > 0) {
+        tab = js_malloc(ctx, sizeof(tab[0]) * len);
+        if (!tab)
+            goto exception;
+    }
+    /* ECMA-262 ObjectDefineProperties: look up each key and read its
+       descriptor before the next key, and define the properties only
+       once all the descriptors are valid. */
     for(i = 0; i < len; i++) {
-        JS_FreeValue(ctx, desc);
+        res = JS_GetOwnPropertyFlagsInternal(ctx, &desc_flags, p, atoms[i].atom);
+        if (res < 0)
+            goto exception;
+        if (!res || !(desc_flags & JS_PROP_ENUMERABLE))
+            continue;
         desc = JS_GetProperty(ctx, props, atoms[i].atom);
         if (JS_IsException(desc))
             goto exception;
-        if (JS_DefinePropertyDesc(ctx, obj, atoms[i].atom, desc,
-                                  JS_PROP_THROW | JS_PROP_DEFINE_PROPERTY) < 0)
+        res = js_obj_to_desc(ctx, &tab[count].desc, desc);
+        JS_FreeValue(ctx, desc);
+        if (res < 0)
+            goto exception;
+        tab[count].atom = atoms[i].atom;
+        count++;
+    }
+    for(i = 0; i < count; i++) {
+        if (JS_DefineProperty(ctx, obj, tab[i].atom, tab[i].desc.value,
+                              tab[i].desc.getter, tab[i].desc.setter,
+                              tab[i].desc.flags | JS_PROP_THROW |
+                              JS_PROP_DEFINE_PROPERTY) < 0)
             goto exception;
     }
     ret = 0;
 
 exception:
+    for(i = 0; i < count; i++)
+        js_free_desc(ctx, &tab[i].desc);
+    js_free(ctx, tab);
     js_free_prop_enum(ctx, atoms, len);
     JS_FreeValue(ctx, props);
-    JS_FreeValue(ctx, desc);
     return ret;
 }
 

tests/bug1794.js

@@ -0,0 +1,53 @@
+import { assert, assertThrows } from "./assert.js";
+
+// Each key is checked for enumerability and its descriptor read before
+// the next key is looked at.
+let log = [];
+const props = new Proxy({ a: { value: 1 }, b: { value: 2 } }, {
+    ownKeys(t) { log.push("ownKeys"); return Reflect.ownKeys(t); },
+    getOwnPropertyDescriptor(t, k) { log.push("gopd " + k); return Reflect.getOwnPropertyDescriptor(t, k); },
+    get(t, k) { log.push("get " + k); return Reflect.get(t, k); },
+});
+let o = Object.defineProperties({}, props);
+assert(log.join(), "ownKeys,gopd a,get a,gopd b,get b");
+assert(o.a, 1);
+assert(o.b, 2);
+
+log = [];
+o = Object.create(null, props);
+assert(log.join(), "ownKeys,gopd a,get a,gopd b,get b");
+assert(o.a, 1);
+assert(o.b, 2);
+
+// All descriptors are validated before any property is defined.
+o = {};
+assertThrows(TypeError, () => Object.defineProperties(o, { a: { value: 1 }, b: { get: 1 } }));
+assert("a" in o, false);
+assertThrows(TypeError, () => Object.defineProperties(o, { a: { value: 1 }, b: 2 }));
+assert("a" in o, false);
+
+// A getter that makes a later key non-enumerable or deletes it is seen.
+const src = {
+    get a() {
+        Object.defineProperty(src, "b", { enumerable: false });
+        delete src.c;
+        return { value: 1 };
+    },
+    b: { value: 2 },
+    c: { value: 3 },
+    d: { value: 4 },
+};
+o = Object.defineProperties({}, src);
+assert(Object.keys(o).length, 0);
+assert(o.a, 1);
+assert("b" in o, false);
+assert("c" in o, false);
+assert(o.d, 4);
+
+// Non-enumerable and symbol keys.
+const sym = Symbol("s");
+const src2 = { [sym]: { value: "s" } };
+Object.defineProperty(src2, "hidden", { value: { value: "h" }, enumerable: false });
+o = Object.defineProperties({}, src2);
+assert(o[sym], "s");
+assert("hidden" in o, false);