Changeset 235356 in webkit

Timestamp:

Aug 27, 2018 1:31:43 AM (6 years ago)

Author:

yusukesuzuki@slowstart.org

Message:

[JSC] Array.prototype.reverse modifies JSImmutableButterfly
​https://bugs.webkit.org/show_bug.cgi?id=188794

Reviewed by Saam Barati.

JSTests:

• stress/reverse-with-immutable-butterfly.js: Added.

(shouldBe):
(reverseInt):
(reverseDouble):
(reverseContiguous):

Source/JavaScriptCore:

While Array.prototype.reverse modifies the butterfly of the given Array,
it does not account JSImmutableButterfly case. So it accidentally modifies
the content of JSImmutableButterfly.
This patch converts CoW arrays to writable arrays before reversing.

• runtime/ArrayPrototype.cpp:

(JSC::arrayProtoFuncReverse):

• runtime/JSObject.h:

(JSC::JSObject::ensureWritable):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/reverse-with-immutable-butterfly.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ArrayPrototype.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/JSArray.cpp (modified) (5 diffs)
• Source/JavaScriptCore/runtime/JSArrayInlines.h (modified) (2 diffs)
• Source/JavaScriptCore/runtime/JSObject.cpp (modified) (5 diffs)
• Source/JavaScriptCore/runtime/JSObject.h (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2018-08-24  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
+
+        [JSC] Array.prototype.reverse modifies JSImmutableButterfly
+        https://bugs.webkit.org/show_bug.cgi?id=188794
+
+        Reviewed by Saam Barati.
+
+        * stress/reverse-with-immutable-butterfly.js: Added.
+        (shouldBe):
+        (reverseInt):
+        (reverseDouble):
+        (reverseContiguous):
+
 2018-08-22  Saam barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2018-08-24  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
+
+        [JSC] Array.prototype.reverse modifies JSImmutableButterfly
+        https://bugs.webkit.org/show_bug.cgi?id=188794
+
+        Reviewed by Saam Barati.
+
+        While Array.prototype.reverse modifies the butterfly of the given Array,
+        it does not account JSImmutableButterfly case. So it accidentally modifies
+        the content of JSImmutableButterfly.
+        This patch converts CoW arrays to writable arrays before reversing.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoFuncReverse):
+        * runtime/JSObject.h:
+        (JSC::JSObject::ensureWritable):
+
 2018-08-24  Michael Saboff  <msaboff@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp

@@ -856 +856 @@
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
+    thisObject->ensureWritable(vm);
+
     switch (thisObject->indexingType()) {
     case ALL_CONTIGUOUS_INDEXING_TYPES:

trunk/Source/JavaScriptCore/runtime/JSArray.cpp

@@ -288 +288 @@
     }
 
-    if (isCopyOnWrite(thisObject->indexingMode()))
-        thisObject->convertFromCopyOnWrite(vm);
+    thisObject->ensureWritable(vm);
 
     if (propertyName == vm.propertyNames->length) {
@@ -663 +662 @@
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     Butterfly* butterfly = this->butterfly();
@@ -771 +769 @@
 {
     VM& vm = exec.vm();
+
+    ensureWritable(vm);
+
     auto arrayType = indexingMode();
     switch (arrayType) {
-    case CopyOnWriteArrayWithInt32:
-    case CopyOnWriteArrayWithDouble:
-    case CopyOnWriteArrayWithContiguous:
-        convertFromCopyOnWrite(vm);
-        arrayType = indexingMode();
-        FALLTHROUGH;
     case ArrayWithDouble:
     case ArrayWithInt32:
@@ -923 +918 @@
     RELEASE_ASSERT(count > 0);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     Butterfly* butterfly = this->butterfly();
@@ -1082 +1076 @@
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     Butterfly* butterfly = this->butterfly();

trunk/Source/JavaScriptCore/runtime/JSArrayInlines.h

@@ -89 +89 @@
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-reloop:
+    ensureWritable(vm);
+
     Butterfly* butterfly = this->butterfly();
 
@@ -229 +230 @@
     }
 
-    default: {
-        RELEASE_ASSERT(isCopyOnWrite(indexingMode()));
-        convertFromCopyOnWrite(vm);
-        goto reloop;
-    }
+    default:
+        RELEASE_ASSERT_NOT_REACHED();
     }
 }

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -839 +839 @@
     }
 
-    if (isCopyOnWrite(thisObject->indexingMode()))
-        thisObject->convertFromCopyOnWrite(vm);
+    thisObject->ensureWritable(vm);
 
     switch (thisObject->indexingType()) {
@@ -1637 +1636 @@
         return nullptr;
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
-    
+    ensureWritable(vm);
+
     switch (indexingType()) {
     case ALL_BLANK_INDEXING_TYPES:
@@ -1674 +1672 @@
 ArrayStorage* JSObject::ensureArrayStorageExistsAndEnterDictionaryIndexingMode(VM& vm)
 {
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     switch (indexingType()) {
@@ -1708 +1705 @@
 void JSObject::switchToSlowPutArrayStorage(VM& vm)
 {
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     switch (indexingType()) {
@@ -2545 +2541 @@
     ASSERT(index <= MAX_ARRAY_INDEX);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     if (!inSparseIndexingMode()) {

trunk/Source/JavaScriptCore/runtime/JSObject.h

@@ -866 +866 @@
         return ensureArrayStorageSlow(vm);
     }
+
+    void ensureWritable(VM& vm)
+    {
+        if (isCopyOnWrite(indexingMode()))
+            convertFromCopyOnWrite(vm);
+    }
         
     static size_t offsetOfInlineStorage();