Array unshift/shift should not race against the AI in the compiler thread.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Dec 2018 06:56:51 +0000 (06:56 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Dec 2018 06:56:51 +0000 (06:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192795
<rdar://problem/46724263>

Reviewed by Saam Barati.

JSTests:

* stress/array-unshift-should-not-race-against-compiler-thread.js: Added.

Source/JavaScriptCore:

The Array unshift and shift operations for ArrayStorage type arrays are protected
using the cellLock.  The AbstractInterpreter's foldGetByValOnConstantProperty()
function does grab the cellLock before reading a value from the array's ArrayStorage,
but does not get the array butterfly under the protection of the cellLock.

This is insufficient and racy.  For ArrayStorage type arrays, the fetching of the
butterfly also needs to be protected by the cellLock.  The unshift / shift
operations can move values around in the butterfly.  Hence, the fact that AI has
fetched a butterfly pointer (while ensuring no structure change) is insufficient
to guarantee that the values in the butterfly haven't shifted.

Having AI hold the cellLock the whole time (from before fetching the butterfly
till after reading the value from it) eliminates this race.  Note: we only need
to do this for ArrayStorage type arrays.

Note also that though AI is holding the cellLock in this case, we still need to
ensure that the array structure hasn't changed around the fetching of the butterfly.
This is because operations other than unshift and shift are guarded by this
protocol, and not the cellLock.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* runtime/JSArray.cpp:
(JSC::JSArray::unshiftCountSlowCase):

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

JSTests/ChangeLog
JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/runtime/JSArray.cpp

index b9cf54e..f3dc588 100644 (file)
@@ -1,3 +1,13 @@
+2018-12-17  Mark Lam  <mark.lam@apple.com>
+
+        Array unshift/shift should not race against the AI in the compiler thread.
+        https://bugs.webkit.org/show_bug.cgi?id=192795
+        <rdar://problem/46724263>
+
+        Reviewed by Saam Barati.
+
+        * stress/array-unshift-should-not-race-against-compiler-thread.js: Added.
+
 2018-12-16  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
 
         [JSC] Optimize Object.keys by caching own keys results in StructureRareData
diff --git a/JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js b/JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js
new file mode 100644 (file)
index 0000000..a0d82e1
--- /dev/null
@@ -0,0 +1,7 @@
+let x = [];
+for (let i = 0; i < 30; ++i) {
+    for (let j = 0; j < 20000; ++j) {
+        x[0]
+        x.unshift(undefined);
+    }
+}
index 103617b..3865cdb 100644 (file)
@@ -1,3 +1,36 @@
+2018-12-17  Mark Lam  <mark.lam@apple.com>
+
+        Array unshift/shift should not race against the AI in the compiler thread.
+        https://bugs.webkit.org/show_bug.cgi?id=192795
+        <rdar://problem/46724263>
+
+        Reviewed by Saam Barati.
+
+        The Array unshift and shift operations for ArrayStorage type arrays are protected
+        using the cellLock.  The AbstractInterpreter's foldGetByValOnConstantProperty()
+        function does grab the cellLock before reading a value from the array's ArrayStorage,
+        but does not get the array butterfly under the protection of the cellLock.
+
+        This is insufficient and racy.  For ArrayStorage type arrays, the fetching of the
+        butterfly also needs to be protected by the cellLock.  The unshift / shift
+        operations can move values around in the butterfly.  Hence, the fact that AI has
+        fetched a butterfly pointer (while ensuring no structure change) is insufficient
+        to guarantee that the values in the butterfly haven't shifted.
+
+        Having AI hold the cellLock the whole time (from before fetching the butterfly
+        till after reading the value from it) eliminates this race.  Note: we only need
+        to do this for ArrayStorage type arrays.
+
+        Note also that though AI is holding the cellLock in this case, we still need to
+        ensure that the array structure hasn't changed around the fetching of the butterfly.
+        This is because operations other than unshift and shift are guarded by this
+        protocol, and not the cellLock.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::unshiftCountSlowCase):
+
 2018-12-16  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
 
         [JSC] Optimize Object.keys by caching own keys results in StructureRareData
index f21af7c..7cf1e2b 100644 (file)
@@ -1901,17 +1901,19 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                 if (isNuked(structureIDEarly))
                     return false;
 
-                WTF::loadLoadFence();
-                Butterfly* butterfly = array->butterfly();
+                if (node->arrayMode().arrayClass() == Array::OriginalCopyOnWriteArray) {
 
-                WTF::loadLoadFence();
-                StructureID structureIDLate = array->structureID();
+                    WTF::loadLoadFence();
+                    Butterfly* butterfly = array->butterfly();
 
-                if (structureIDEarly != structureIDLate)
-                    return false;
+                    WTF::loadLoadFence();
+                    StructureID structureIDLate = array->structureID();
+
+                    if (structureIDEarly != structureIDLate)
+                        return false;
+
+                    Structure* structure = m_vm.getStructure(structureIDLate);
 
-                Structure* structure = m_vm.getStructure(structureIDLate);
-                if (node->arrayMode().arrayClass() == Array::OriginalCopyOnWriteArray) {
                     if (!isCopyOnWrite(structure->indexingMode()))
                         return false;
 
@@ -1944,17 +1946,27 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                 }
 
                 if (node->arrayMode().type() == Array::ArrayStorage || node->arrayMode().type() == Array::SlowPutArrayStorage) {
-                    if (!hasAnyArrayStorage(structure->indexingMode()))
-                        return false;
-
-                    if (structure->typeInfo().interceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero())
-                        return false;
-
                     JSValue value;
                     {
                         // ArrayStorage's Butterfly can be half-broken state.
                         auto locker = holdLock(array->cellLock());
 
+                        WTF::loadLoadFence();
+                        Butterfly* butterfly = array->butterfly();
+
+                        WTF::loadLoadFence();
+                        StructureID structureIDLate = array->structureID();
+
+                        if (structureIDEarly != structureIDLate)
+                            return false;
+
+                        Structure* structure = m_vm.getStructure(structureIDLate);
+                        if (!hasAnyArrayStorage(structure->indexingMode()))
+                            return false;
+
+                        if (structure->typeInfo().interceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero())
+                            return false;
+
                         ArrayStorage* storage = butterfly->arrayStorage();
                         if (index >= storage->length())
                             return false;
index f32af38..ded02ae 100644 (file)
@@ -334,6 +334,8 @@ void JSArray::getOwnNonIndexPropertyNames(JSObject* object, ExecState* exec, Pro
 // This method makes room in the vector, but leaves the new space for count slots uncleared.
 bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool addToFront, unsigned count)
 {
+    ASSERT(cellLock().isLocked());
+
     ArrayStorage* storage = ensureArrayStorage(vm);
     Butterfly* butterfly = storage->butterfly();
     Structure* structure = this->structure(vm);