Bug 1847369 - only allow indexed properties in elements for both Arrays and non-Arrays r=jandem

Differential Revision: https://phabricator.services.mozilla.com/D185614

js/src/builtin/Array.cpp

@@ -72,7 +72,7 @@ using JS::AutoCheckCannotGC;
 using JS::IsArrayAnswer;
 using JS::ToUint32;
 
-static inline bool ObjectMayHaveExtraIndexedOwnProperties(JSObject* obj) {
+bool js::ObjectMayHaveExtraIndexedOwnProperties(JSObject* obj) {
   if (!obj->is<NativeObject>()) {
     return true;
   }
@@ -114,7 +114,7 @@ bool js::PrototypeMayHaveIndexedProperties(NativeObject* obj) {
  * elements. This includes other indexed properties in its shape hierarchy, and
  * indexed properties or elements along its prototype chain.
  */
-static bool ObjectMayHaveExtraIndexedProperties(JSObject* obj) {
+bool js::ObjectMayHaveExtraIndexedProperties(JSObject* obj) {
   MOZ_ASSERT_IF(obj->hasDynamicPrototype(), !obj->is<NativeObject>());
 
   if (ObjectMayHaveExtraIndexedOwnProperties(obj)) {

js/src/builtin/Array.h

@@ -157,6 +157,10 @@ extern JSString* ArrayToSource(JSContext* cx, HandleObject obj);
 extern bool IsCrossRealmArrayConstructor(JSContext* cx, JSObject* obj,
                                          bool* result);
 
+extern bool ObjectMayHaveExtraIndexedOwnProperties(JSObject* obj);
+
+extern bool ObjectMayHaveExtraIndexedProperties(JSObject* obj);
+
 extern bool PrototypeMayHaveIndexedProperties(NativeObject* obj);
 
 // JS::IsArray has multiple overloads, use js::IsArrayFromJit to disambiguate.

js/src/builtin/JSON.cpp

@@ -805,19 +805,16 @@ static bool CanFastStringifyObject(NativeObject* obj) {
   }
 
   if (obj->is<ArrayObject>()) {
-    if (ProtoMayHaveEnumerableProperties(obj)) {
-      // Array objects look up properties with keys [0..length). Non-Arrays only
-      // look at own properties, so they do not need this check.
+    // Arrays will look up all keys [0..length) so disallow anything that could
+    // find those keys anywhere but in the dense elements.
+    if (!IsPackedArray(obj) && ObjectMayHaveExtraIndexedProperties(obj)) {
       return false;
     }
-    if (obj->isIndexed() && !obj->hasEnumerableProperty()) {
-      // Array objects may have sparse indexes, which will normally trigger
-      // a SPARSE_INDEX bailout when those properties are iterated over. But
-      // there is an optimization where non-element properties are skipped if
-      // !obj->hasEnumerableProperty(), so force a bail here.
-      //
-      // Non-Arrays do not output properties named [0..length), so will not run
-      // into this problem.
+  } else {
+    // Non-Arrays will only look at own properties, but still disallow any
+    // indexed properties other than in the dense elements because they would
+    // require sorting.
+    if (ObjectMayHaveExtraIndexedOwnProperties(obj)) {
       return false;
     }
   }
@@ -841,7 +838,6 @@ static bool CanFastStringifyObject(NativeObject* obj) {
   MACRO(DEEP_RECURSION)                       \
   MACRO(NON_DATA_PROPERTY)                    \
   MACRO(TOO_MANY_PROPERTIES)                  \
-  MACRO(SPARSE_INDEX)                         \
   MACRO(BIGINT)                               \
   MACRO(API)                                  \
   MACRO(HAVE_REPLACER)                        \
@@ -1037,8 +1033,8 @@ class OwnNonIndexKeysIterForJSON {
       return;
     }
     if (!nobj->hasEnumerableProperty()) {
-      // Note that any shortcut condition here must consider the possibility of
-      // sparse indexes. See CanFastStringifyObject() for details.
+      // Non-Arrays with no enumerable properties can just be skipped.
+      MOZ_ASSERT(!nobj->is<ArrayObject>());
       done_ = true;
       return;
     }
@@ -1364,7 +1360,11 @@ static bool FastStr(JSContext* cx, Handle<Value> v, StringifyContext* scx,
       }
 
       MOZ_ASSERT(iter.done());
-      top.advanceToProperties();
+      if (top.isArray) {
+        MOZ_ASSERT(!top.nobj->isIndexed());
+      } else {
+        top.advanceToProperties();
+      }
     }
 
     if (top.iter.is<OwnNonIndexKeysIterForJSON>()) {
@@ -1380,18 +1380,12 @@ static bool FastStr(JSContext* cx, Handle<Value> v, StringifyContext* scx,
 
         PropertyInfoWithKey prop = iter.next();
 
-        uint32_t index = -1;
-        if (top.nobj->isIndexed() && IdIsIndex(prop.key(), &index)) {
-          // Do not support sparse elements, because they need to be sorted
-          // numerically and before any non-index property names.
-          *whySlow = BailReason::SPARSE_INDEX;
-          return true;
-        }
-
-        if (top.isArray) {
-          // Arrays ignore non-index properties.
-          continue;
-        }
+        // A non-Array with indexed elements would need to sort the indexes
+        // numerically, which this code does not support. These objects are
+        // skipped when obj->isIndexed(), so no index properties should be found
+        // here.
+        mozilla::DebugOnly<uint32_t> index = -1;
+        MOZ_ASSERT(!IdIsIndex(prop.key(), &index));
 
         Value val = top.nobj->getSlot(prop.slot());
         if (!PreprocessFastValue(cx, &val, scx, whySlow)) {
@@ -1412,15 +1406,9 @@ static bool FastStr(JSContext* cx, Handle<Value> v, StringifyContext* scx,
         }
         wroteMember = true;
 
-        if (prop.key().isString()) {
-          if (!Quote(cx, scx->sb, prop.key().toString())) {
-            return false;
-          }
-        } else {
-          MOZ_ASSERT(int32_t(index) >= 0, "not a string, not an index");
-          if (!EmitQuotedIndexColon(scx->sb, index)) {
-            return false;
-          }
+        MOZ_ASSERT(prop.key().isString());
+        if (!Quote(cx, scx->sb, prop.key().toString())) {
+          return false;
         }
 
         if (!scx->sb.append(':')) {

js/src/tests/non262/JSON/stringify-fastpath.js

@@ -138,6 +138,32 @@ function testFastPath() {
     failures += checkFast(Number.NaN, "PRIMITIVE");
     failures += checkFast(undefined, "PRIMITIVE");
 
+    // Array has enumerated indexed + non-indexed slots.
+    const nonElements = [];
+    Object.defineProperty(nonElements, 0, { value: "hi", enumerated: true });
+    nonElements.named = 7;
+    failures += checkFast(nonElements, "INELIGIBLE_OBJECT");
+
+    // Array's prototype has indexed slot and/or inherited element.
+    const proto = {};
+    Object.defineProperty(proto, "0", { value: 1, enumerable: false });
+    const holy = [, , 3];
+    Object.setPrototypeOf(holy, proto);
+    failures += checkFast(holy, "INELIGIBLE_OBJECT");
+    Object.setPrototypeOf(holy, { 1: true });
+    failures += checkFast(holy, "INELIGIBLE_OBJECT");
+
+    // This is probably redundant with one of the above, but it was
+    // found by a fuzzer at one point.
+    const accessorProto = Object.create(Array.prototype);
+    Object.defineProperty(accessorProto, "0", {
+        get() { return 2; }, set() { }
+    });
+    const child = [];
+    Object.setPrototypeOf(child, accessorProto);
+    child.push(1);
+    failures += checkFast(child, "INELIGIBLE_OBJECT");
+
     failures += checkFast({ get x() { return 1; } }, "NON_DATA_PROPERTY");
 
     const self = {};
@@ -159,10 +185,10 @@ function testFastPath() {
     middle[2] = ouroboros;
     failures += checkFast(ouroboros, "DEEP_RECURSION"); // Cyclic after 10 recursions
 
-    failures += checkFast({ 0: true, 1: true, 10000: true }, "SPARSE_INDEX");
+    failures += checkFast({ 0: true, 1: true, 10000: true }, "INELIGIBLE_OBJECT");
     const arr = [1, 2, 3];
     arr[10000] = 4;
-    failures += checkFast(arr, "SPARSE_INDEX");
+    failures += checkFast(arr, "INELIGIBLE_OBJECT");
 
     failures += checkFast({ x: 12n }, "BIGINT");
 

js/src/vm/Iteration.cpp

@@ -609,7 +609,7 @@ struct SortComparatorIds {
 
 #endif /* DEBUG */
 
-void js::AssertNoEnumerableProperties(NativeObject* obj) {
+static void AssertNoEnumerableProperties(NativeObject* obj) {
 #ifdef DEBUG
   // Verify the object has no enumerable properties if the HasEnumerable
   // ObjectFlag is not set.
@@ -628,6 +628,36 @@ void js::AssertNoEnumerableProperties(NativeObject* obj) {
 #endif  // DEBUG
 }
 
+// Typed arrays and classes with an enumerate hook can have extra properties not
+// included in the shape's property map or the object's dense elements.
+static bool ClassCanHaveExtraEnumeratedProperties(const JSClass* clasp) {
+  return IsTypedArrayClass(clasp) || clasp->getNewEnumerate() ||
+         clasp->getEnumerate();
+}
+
+static bool ProtoMayHaveEnumerableProperties(JSObject* obj) {
+  if (!obj->is<NativeObject>()) {
+    return true;
+  }
+
+  JSObject* proto = obj->as<NativeObject>().staticPrototype();
+  while (proto) {
+    if (!proto->is<NativeObject>()) {
+      return true;
+    }
+    NativeObject* nproto = &proto->as<NativeObject>();
+    if (nproto->hasEnumerableProperty() ||
+        nproto->getDenseInitializedLength() > 0 ||
+        ClassCanHaveExtraEnumeratedProperties(nproto->getClass())) {
+      return true;
+    }
+    AssertNoEnumerableProperties(nproto);
+    proto = nproto->staticPrototype();
+  }
+
+  return false;
+}
+
 bool PropertyEnumerator::snapshot(JSContext* cx) {
   // If we're only interested in enumerable properties and the proto chain has
   // no enumerable properties (the common case), we can optimize this to ignore

js/src/vm/Iteration.h

@@ -800,38 +800,6 @@ IteratorHelperObject* NewIteratorHelper(JSContext* cx);
 bool IterableToArray(JSContext* cx, HandleValue iterable,
                      MutableHandle<ArrayObject*> array);
 
-void AssertNoEnumerableProperties(NativeObject* obj);
-
-// Typed arrays and classes with an enumerate hook can have extra properties not
-// included in the shape's property map or the object's dense elements.
-static inline bool ClassCanHaveExtraEnumeratedProperties(const JSClass* clasp) {
-  return IsTypedArrayClass(clasp) || clasp->getNewEnumerate() ||
-         clasp->getEnumerate();
-}
-
-static inline bool ProtoMayHaveEnumerableProperties(JSObject* obj) {
-  if (!obj->is<NativeObject>()) {
-    return true;
-  }
-
-  JSObject* proto = obj->as<NativeObject>().staticPrototype();
-  while (proto) {
-    if (!proto->is<NativeObject>()) {
-      return true;
-    }
-    NativeObject* nproto = &proto->as<NativeObject>();
-    if (nproto->hasEnumerableProperty() ||
-        nproto->getDenseInitializedLength() > 0 ||
-        ClassCanHaveExtraEnumeratedProperties(nproto->getClass())) {
-      return true;
-    }
-    AssertNoEnumerableProperties(nproto);
-    proto = nproto->staticPrototype();
-  }
-
-  return false;
-}
-
 } /* namespace js */
 
 #endif /* vm_Iteration_h */