Attachment 357527 Details for Bug 192795 – proposed patch.

[patch] proposed patch.

bug-192795.patch (text/plain), 6.95 KB, created by Mark Lam on 2018-12-17 20:48:07 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2018-12-17 20:48:07 PST

Size: 6.95 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 239316)
>+++ JSTests/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        * stress/array-unshift-should-not-race-against-compiler-thread.js: Added.
>+
> 2018-12-17  Mark Lam  <mark.lam@apple.com>
> 
>         SamplingProfiler's isValidFramePointer() should reject address at stack origin.
>Index: JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js
>===================================================================
>--- JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js	(nonexistent)
>+++ JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js	(working copy)
>@@ -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: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 239312)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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-17  Mark Lam  <mark.lam@apple.com>
> 
>         SamplingProfiler's isValidFramePointer() should reject address at stack origin.
>Index: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h	(revision 239312)
>+++ Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h	(working copy)
>@@ -1900,17 +1900,19 @@ bool AbstractInterpreter<AbstractStateTy
>                 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;
> 
>@@ -1943,17 +1945,27 @@ bool AbstractInterpreter<AbstractStateTy
>                 }
> 
>                 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: Source/JavaScriptCore/runtime/JSArray.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/JSArray.cpp	(revision 239312)
>+++ Source/JavaScriptCore/runtime/JSArray.cpp	(working copy)
>@@ -334,6 +334,8 @@ void JSArray::getOwnNonIndexPropertyName
> // 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);

Actions: View | Formatted Diff | Diff

Attachments on bug 192795: 357527