Changeset 207178 in webkit

Timestamp:

Oct 11, 2016, 4:25:38 PM (8 years ago)

Author:

mark.lam@apple.com

Message:

Array.prototype.concat should not modify frozen objects.
​https://bugs.webkit.org/show_bug.cgi?id=163302

Reviewed by Filip Pizlo.

JSTests:

• stress/array-concat-on-frozen-object.js: Added.

Source/JavaScriptCore:

The ES6 spec for Array.prototype.concat states that it uses the
CreateDataPropertyOrThrow() to add items to the result array. The spec for
CreateDataPropertyOrThrow states:

"This abstract operation creates a property whose attributes are set to the same
defaults used for properties created by the ECMAScript language assignment
operator. Normally, the property will not already exist. If it does exist and is
not configurable or if O is not extensible, DefineOwnProperty will return
false causing this operation to throw a TypeError exception."

Since the properties of frozen objects are not extensible, not configurable, and
not writable, Array.prototype.concat should fail to write to the result array if
it is frozen.

Ref: ​https://tc39.github.io/ecma262/#sec-array.prototype.concat,
​https://tc39.github.io/ecma262/#sec-createdatapropertyorthrow, and
​https://tc39.github.io/ecma262/#sec-createdataproperty.

The fix consists of 2 parts:

1. moveElement() should use the PutDirectIndexShouldThrow mode when invoking putDirectIndex(), and
2. SparseArrayValueMap::putDirect() should check for the case where the property is read only.

(2) ensures that we don't write into a non-writable property.
(1) ensures that we throw a TypeError for attempts to write to a non-writeable
property.

• runtime/ArrayPrototype.cpp:

(JSC::moveElements):

• runtime/SparseArrayValueMap.cpp:

(JSC::SparseArrayValueMap::putDirect):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/array-concat-on-frozen-object.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ArrayPrototype.cpp (modified) (2 diffs)
• Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2016-10-11  Mark Lam  <mark.lam@apple.com>
+
+        Array.prototype.concat should not modify frozen objects.
+        https://bugs.webkit.org/show_bug.cgi?id=163302
+
+        Reviewed by Filip Pizlo.
+
+        * stress/array-concat-on-frozen-object.js: Added.
+
 2016-10-11  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-10-11  Mark Lam  <mark.lam@apple.com>
+
+        Array.prototype.concat should not modify frozen objects.
+        https://bugs.webkit.org/show_bug.cgi?id=163302
+
+        Reviewed by Filip Pizlo.
+
+        The ES6 spec for Array.prototype.concat states that it uses the
+        CreateDataPropertyOrThrow() to add items to the result array.  The spec for
+        CreateDataPropertyOrThrow states:
+
+        "This abstract operation creates a property whose attributes are set to the same
+        defaults used for properties created by the ECMAScript language assignment
+        operator. Normally, the property will not already exist. If it does exist and is
+        not configurable or if O is not extensible, [[DefineOwnProperty]] will return
+        false causing this operation to throw a TypeError exception."
+
+        Since the properties of frozen objects are not extensible, not configurable, and
+        not writable, Array.prototype.concat should fail to write to the result array if
+        it is frozen.
+
+        Ref: https://tc39.github.io/ecma262/#sec-array.prototype.concat,
+        https://tc39.github.io/ecma262/#sec-createdatapropertyorthrow, and
+        https://tc39.github.io/ecma262/#sec-createdataproperty.
+
+        The fix consists of 2 parts:
+        1. moveElement() should use the PutDirectIndexShouldThrow mode when invoking
+           putDirectIndex(), and
+        2. SparseArrayValueMap::putDirect() should check for the case where the property
+           is read only.
+
+        (2) ensures that we don't write into a non-writable property.
+        (1) ensures that we throw a TypeError for attempts to write to a non-writeable
+        property.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::moveElements):
+        * runtime/SparseArrayValueMap.cpp:
+        (JSC::SparseArrayValueMap::putDirect):
+
 2016-10-11  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp

@@ -1079 +1079 @@
             JSValue value = source->tryGetIndexQuickly(i);
             if (value) {
-                target->putDirectIndex(exec, targetOffset + i, value);
+                target->putDirectIndex(exec, targetOffset + i, value, 0, PutDirectIndexShouldThrow);
                 RETURN_IF_EXCEPTION(scope, false);
             }
@@ -1088 +1088 @@
             RETURN_IF_EXCEPTION(scope, false);
             if (value) {
-                target->putDirectIndex(exec, targetOffset + i, value);
+                target->putDirectIndex(exec, targetOffset + i, value, 0, PutDirectIndexShouldThrow);
                 RETURN_IF_EXCEPTION(scope, false);
             }

trunk/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp

@@ -118 +118 @@
     SparseArrayEntry& entry = result.iterator->value;
 
-    // To save a separate find & add, we first always add to the sparse map.
-    // In the uncommon case that this is a new property, and the array is not
-    // extensible, this is not the right thing to have done - so remove again.
-    if (mode != PutDirectIndexLikePutDirect && result.isNewEntry && !array->isStructureExtensible()) {
-        remove(result.iterator);
-        return reject(exec, mode == PutDirectIndexShouldThrow, "Attempting to define property on object that is not extensible.");
+    if (mode != PutDirectIndexLikePutDirect && !array->isStructureExtensible()) {
+        // To save a separate find & add, we first always add to the sparse map.
+        // In the uncommon case that this is a new property, and the array is not
+        // extensible, this is not the right thing to have done - so remove again.
+        if (result.isNewEntry) {
+            remove(result.iterator);
+            return reject(exec, mode == PutDirectIndexShouldThrow, "Attempting to define property on object that is not extensible.");
+        }
+        if (entry.attributes & ReadOnly)
+            return reject(exec, mode == PutDirectIndexShouldThrow, ReadonlyPropertyWriteError);
     }
-
     entry.attributes = attributes;
     entry.set(exec->vm(), this, value);