Array methods should throw TypeError upon attempting to modify a string
authorross.kirsling@sony.com <ross.kirsling@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Sep 2019 00:26:29 +0000 (00:26 +0000)
committerross.kirsling@sony.com <ross.kirsling@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Sep 2019 00:26:29 +0000 (00:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201910

Reviewed by Keith Miller.

JSTests:

* stress/array-methods-should-not-modify-string.js: Added.

* mozilla/js1_6/Array/regress-304828.js:
Fix test. Original copy was changed similarly seven years ago:
https://searchfox.org/mozilla-central/source/js/src/tests/non262/Array/regress-304828.js

* stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js:
Fix test. `Object.__proto__ = []; Object.shift();` shouldn't be valid JS.

Source/JavaScriptCore:

We currently allow Array prototype methods to modify strings that they are called upon in certain cases.
(In particular, we're inconsistent about permitting writes to the length property.)

According to section 22.1.3 of the ES spec, this should result in a TypeError.
https://tc39.es/ecma262/#sec-properties-of-the-array-prototype-object
(Test262 cases are needed, but the key is that all such methods use Set(..., true) which throws on failure.)

* runtime/ArrayPrototype.cpp:
(JSC::putLength):
(JSC::setLength):
Never update the length property of a non-JSArray without checking whether we're actually allowed to.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250275 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/mozilla/js1_6/Array/regress-304828.js
JSTests/stress/array-methods-should-not-modify-string.js [new file with mode: 0644]
JSTests/stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ArrayPrototype.cpp

index a19b8a5..a2c1d16 100644 (file)
@@ -1,3 +1,19 @@
+2019-09-23  Ross Kirsling  <ross.kirsling@sony.com>
+
+        Array methods should throw TypeError upon attempting to modify a string
+        https://bugs.webkit.org/show_bug.cgi?id=201910
+
+        Reviewed by Keith Miller.
+
+        * stress/array-methods-should-not-modify-string.js: Added.
+
+        * mozilla/js1_6/Array/regress-304828.js:
+        Fix test. Original copy was changed similarly seven years ago:
+        https://searchfox.org/mozilla-central/source/js/src/tests/non262/Array/regress-304828.js
+
+        * stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js:
+        Fix test. `Object.__proto__ = []; Object.shift();` shouldn't be valid JS.
+
 2019-09-23  Mark Lam  <mark.lam@apple.com>
 
         Lazy JSGlobalObject property materialization should not use putDirectWithoutTransition.
index 4cc3a94..e23d09c 100644 (file)
@@ -86,7 +86,7 @@ reportCompare(expect, actual, summary + ': sort');
 
 // push
 value  = 'abc';
-expect = 6;
+expect = 'TypeError: Attempted to assign to readonly property.';
 try
 {
   actual = Array.prototype.push.call(value, 'd', 'e', 'f');
@@ -96,7 +96,6 @@ catch(e)
   actual = e + '';
 }
 reportCompare(expect, actual, summary + ': push');
-reportCompare('abc', value, summary + ': push');
 
 // pop
 value  = 'abc';
diff --git a/JSTests/stress/array-methods-should-not-modify-string.js b/JSTests/stress/array-methods-should-not-modify-string.js
new file mode 100644 (file)
index 0000000..f1f6638
--- /dev/null
@@ -0,0 +1,27 @@
+function shouldThrowTypeError(func) {
+    let error;
+    try {
+        func();
+    } catch (e) {
+        error = e;
+    }
+
+    if (!(error instanceof TypeError))
+        throw new Error('Expected TypeError!');
+}
+
+shouldThrowTypeError(() => { Array.prototype.push.call(''); });
+shouldThrowTypeError(() => { Array.prototype.push.call('', 1); });
+shouldThrowTypeError(() => { Array.prototype.push.call('abc'); });
+shouldThrowTypeError(() => { Array.prototype.push.call('abc', 1); });
+
+shouldThrowTypeError(() => { Array.prototype.pop.call(''); });
+shouldThrowTypeError(() => { Array.prototype.pop.call('abc'); });
+
+shouldThrowTypeError(() => { Array.prototype.shift.call(''); });
+shouldThrowTypeError(() => { Array.prototype.shift.call('', 1); });
+shouldThrowTypeError(() => { Array.prototype.shift.call('abc'); });
+shouldThrowTypeError(() => { Array.prototype.shift.call('abc', 1); });
+
+shouldThrowTypeError(() => { Array.prototype.unshift.call(''); });
+shouldThrowTypeError(() => { Array.prototype.unshift.call('abc'); });
index cfe3f7c..adba442 100644 (file)
@@ -8,7 +8,7 @@ function main() {
             const v15 = v10 + 127;
             const v16 = String();
             const v17 = String.fromCharCode(v10,v10,v15);
-            const v19 = Object.shift();
+            const v19 = v3.shift();
             function v23() {
                 let v28 = arguments;
             }
index f65bd8a..f0223e9 100644 (file)
@@ -1,3 +1,22 @@
+2019-09-23  Ross Kirsling  <ross.kirsling@sony.com>
+
+        Array methods should throw TypeError upon attempting to modify a string
+        https://bugs.webkit.org/show_bug.cgi?id=201910
+
+        Reviewed by Keith Miller.
+
+        We currently allow Array prototype methods to modify strings that they are called upon in certain cases.
+        (In particular, we're inconsistent about permitting writes to the length property.)
+
+        According to section 22.1.3 of the ES spec, this should result in a TypeError.
+        https://tc39.es/ecma262/#sec-properties-of-the-array-prototype-object
+        (Test262 cases are needed, but the key is that all such methods use Set(..., true) which throws on failure.)
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::putLength):
+        (JSC::setLength):
+        Never update the length property of a non-JSArray without checking whether we're actually allowed to.
+
 2019-09-23  Mark Lam  <mark.lam@apple.com>
 
         Lazy JSGlobalObject property materialization should not use putDirectWithoutTransition.
index 590607f..edb7619 100644 (file)
@@ -166,10 +166,14 @@ static ALWAYS_INLINE JSValue getProperty(ExecState* exec, JSObject* object, unsi
     RELEASE_AND_RETURN(scope, slot.getValue(exec, index));
 }
 
-static ALWAYS_INLINE bool putLength(ExecState* exec, VM& vm, JSObject* obj, JSValue value)
+static ALWAYS_INLINE void putLength(ExecState* exec, VM& vm, JSObject* obj, JSValue value)
 {
+    auto scope = DECLARE_THROW_SCOPE(vm);
     PutPropertySlot slot(obj);
-    return obj->methodTable(vm)->put(obj, exec, vm.propertyNames->length, value, slot);
+    bool success = obj->methodTable(vm)->put(obj, exec, vm.propertyNames->length, value, slot);
+    RETURN_IF_EXCEPTION(scope, void());
+    if (UNLIKELY(!success))
+        throwTypeError(exec, scope, ReadonlyPropertyWriteError);
 }
 
 static ALWAYS_INLINE void setLength(ExecState* exec, VM& vm, JSObject* obj, unsigned value)
@@ -180,10 +184,8 @@ static ALWAYS_INLINE void setLength(ExecState* exec, VM& vm, JSObject* obj, unsi
         jsCast<JSArray*>(obj)->setLength(exec, value, throwException);
         RETURN_IF_EXCEPTION(scope, void());
     }
-    bool success = putLength(exec, vm, obj, jsNumber(value));
-    RETURN_IF_EXCEPTION(scope, void());
-    if (UNLIKELY(!success))
-        throwTypeError(exec, scope, ReadonlyPropertyWriteError);
+    scope.release();
+    putLength(exec, vm, obj, jsNumber(value));
 }
 
 namespace ArrayPrototypeInternal {