From 1d401b8dc610c794fbb6ac1088ebdfd11fa3be52 Mon Sep 17 00:00:00 2001 From: "mark.lam@apple.com" Date: Tue, 11 Oct 2016 23:25:38 +0000 Subject: [PATCH] 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): git-svn-id: https://svn.webkit.org/repository/webkit/trunk@207178 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- JSTests/ChangeLog | 9 +++ JSTests/stress/array-concat-on-frozen-object.js | 72 ++++++++++++++++++++++ Source/JavaScriptCore/ChangeLog | 40 ++++++++++++ Source/JavaScriptCore/runtime/ArrayPrototype.cpp | 4 +- .../JavaScriptCore/runtime/SparseArrayValueMap.cpp | 17 ++--- 5 files changed, 133 insertions(+), 9 deletions(-) create mode 100644 JSTests/stress/array-concat-on-frozen-object.js diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog index 567503d..3dea92c 100644 --- a/JSTests/ChangeLog +++ b/JSTests/ChangeLog @@ -1,3 +1,12 @@ +2016-10-11 Mark Lam + + 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 ValueAdd should be constant folded if the operands are constant String,Primitive or Primitive,String diff --git a/JSTests/stress/array-concat-on-frozen-object.js b/JSTests/stress/array-concat-on-frozen-object.js new file mode 100644 index 0000000..6b785c0 --- /dev/null +++ b/JSTests/stress/array-concat-on-frozen-object.js @@ -0,0 +1,72 @@ +//@ runFTLNoCJIT + +let totalFailed = 0; + +function shouldEqual(testId, actual, expected) { + if (actual != expected) { + throw testId + ": ERROR: expect " + expected + ", actual " + actual; + } +} + +function makeArray() { + return ['unmodifiable']; +} + +function makeArrayLikeObject() { + var obj = {}; + obj[0] = 'unmodifiable'; + obj.length = 1; + return obj; +} + +function emptyArraySourceMaker() { + return []; +} + +function singleElementArraySourceMaker() { + return ['modified_1']; +} + +// Make test functions with unique codeblocks. +function makeConcatTest(testId) { + return new Function("arr", "return arr.concat(['" + testId + "'])"); +} +function makeConcatOnHoleyArrayTest(testId) { + return new Function("arr", "var other = ['" + testId + "']; other[1000] = '" + testId + "'; return arr.concat(other);"); +} + +let numIterations = 10000; + +function runTest(testId, testMaker, targetMaker, sourceMaker, expectedValue, expectedException) { + var test = testMaker(testId); + noInline(test); + + for (var i = 0; i < numIterations; i++) { + var exception = undefined; + + var obj = targetMaker(); + obj = Object.freeze(obj); + + var arr = sourceMaker(); + arr.constructor = { [Symbol.species]: function() { return obj; } }; + + try { + test(arr); + } catch (e) { + exception = "" + e; + exception = exception.substr(0, 10); // Search for "TypeError:". + } + shouldEqual(testId, exception, expectedException); + shouldEqual(testId, obj[0], expectedValue); + } +} + +runTest(10010, makeConcatTest, makeArray, emptyArraySourceMaker, "unmodifiable", "TypeError:"); +runTest(10011, makeConcatTest, makeArray, singleElementArraySourceMaker, "unmodifiable", "TypeError:"); +runTest(10020, makeConcatTest, makeArrayLikeObject, emptyArraySourceMaker, "unmodifiable", "TypeError:"); +runTest(10021, makeConcatTest, makeArrayLikeObject, singleElementArraySourceMaker, "unmodifiable", "TypeError:"); + +runTest(10110, makeConcatOnHoleyArrayTest, makeArray, emptyArraySourceMaker, "unmodifiable", "TypeError:"); +runTest(10111, makeConcatOnHoleyArrayTest, makeArray, singleElementArraySourceMaker, "unmodifiable", "TypeError:"); +runTest(10120, makeConcatOnHoleyArrayTest, makeArrayLikeObject, emptyArraySourceMaker, "unmodifiable", "TypeError:"); +runTest(10121, makeConcatOnHoleyArrayTest, makeArrayLikeObject, singleElementArraySourceMaker, "unmodifiable", "TypeError:"); diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index eca9b65..0e74e1f 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,43 @@ +2016-10-11 Mark Lam + + 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 [DOMJIT] DOMJIT::Patchpoint should have a way to receive constant folded arguments diff --git a/Source/JavaScriptCore/runtime/ArrayPrototype.cpp b/Source/JavaScriptCore/runtime/ArrayPrototype.cpp index bd22b6e..ce6b94e 100644 --- a/Source/JavaScriptCore/runtime/ArrayPrototype.cpp +++ b/Source/JavaScriptCore/runtime/ArrayPrototype.cpp @@ -1078,7 +1078,7 @@ static bool moveElements(ExecState* exec, VM& vm, JSArray* target, unsigned targ for (unsigned i = 0; i < sourceLength; ++i) { 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); } } @@ -1087,7 +1087,7 @@ static bool moveElements(ExecState* exec, VM& vm, JSArray* target, unsigned targ JSValue value = getProperty(exec, source, i); 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); } } diff --git a/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp b/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp index 1fb9a06..990c6c2 100644 --- a/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp +++ b/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp @@ -117,14 +117,17 @@ bool SparseArrayValueMap::putDirect(ExecState* exec, JSObject* array, unsigned i AddResult result = add(array, i); 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); return true; -- 1.8.3.1