Changeset 239325 in webkit


Ignore:
Timestamp:
Dec 17, 2018 10:56:51 PM (5 years ago)
Author:
mark.lam@apple.com
Message:

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.

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):

Location:
trunk
Files:
1 added
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChangeLog

    r239324 r239325  
     12018-12-17  Mark Lam  <mark.lam@apple.com>
     2
     3        Array unshift/shift should not race against the AI in the compiler thread.
     4        https://bugs.webkit.org/show_bug.cgi?id=192795
     5        <rdar://problem/46724263>
     6
     7        Reviewed by Saam Barati.
     8
     9        * stress/array-unshift-should-not-race-against-compiler-thread.js: Added.
     10
    1112018-12-16  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
    212
  • trunk/Source/JavaScriptCore/ChangeLog

    r239324 r239325  
     12018-12-17  Mark Lam  <mark.lam@apple.com>
     2
     3        Array unshift/shift should not race against the AI in the compiler thread.
     4        https://bugs.webkit.org/show_bug.cgi?id=192795
     5        <rdar://problem/46724263>
     6
     7        Reviewed by Saam Barati.
     8
     9        The Array unshift and shift operations for ArrayStorage type arrays are protected
     10        using the cellLock.  The AbstractInterpreter's foldGetByValOnConstantProperty()
     11        function does grab the cellLock before reading a value from the array's ArrayStorage,
     12        but does not get the array butterfly under the protection of the cellLock.
     13
     14        This is insufficient and racy.  For ArrayStorage type arrays, the fetching of the
     15        butterfly also needs to be protected by the cellLock.  The unshift / shift
     16        operations can move values around in the butterfly.  Hence, the fact that AI has
     17        fetched a butterfly pointer (while ensuring no structure change) is insufficient
     18        to guarantee that the values in the butterfly haven't shifted.
     19
     20        Having AI hold the cellLock the whole time (from before fetching the butterfly
     21        till after reading the value from it) eliminates this race.  Note: we only need
     22        to do this for ArrayStorage type arrays.
     23
     24        Note also that though AI is holding the cellLock in this case, we still need to
     25        ensure that the array structure hasn't changed around the fetching of the butterfly.
     26        This is because operations other than unshift and shift are guarded by this
     27        protocol, and not the cellLock.
     28
     29        * dfg/DFGAbstractInterpreterInlines.h:
     30        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
     31        * runtime/JSArray.cpp:
     32        (JSC::JSArray::unshiftCountSlowCase):
     33
    1342018-12-16  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
    235
  • trunk/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h

    r239324 r239325  
    19021902                    return false;
    19031903
    1904                 WTF::loadLoadFence();
    1905                 Butterfly* butterfly = array->butterfly();
    1906 
    1907                 WTF::loadLoadFence();
    1908                 StructureID structureIDLate = array->structureID();
    1909 
    1910                 if (structureIDEarly != structureIDLate)
    1911                     return false;
    1912 
    1913                 Structure* structure = m_vm.getStructure(structureIDLate);
    19141904                if (node->arrayMode().arrayClass() == Array::OriginalCopyOnWriteArray) {
     1905
     1906                    WTF::loadLoadFence();
     1907                    Butterfly* butterfly = array->butterfly();
     1908
     1909                    WTF::loadLoadFence();
     1910                    StructureID structureIDLate = array->structureID();
     1911
     1912                    if (structureIDEarly != structureIDLate)
     1913                        return false;
     1914
     1915                    Structure* structure = m_vm.getStructure(structureIDLate);
     1916
    19151917                    if (!isCopyOnWrite(structure->indexingMode()))
    19161918                        return false;
     
    19451947
    19461948                if (node->arrayMode().type() == Array::ArrayStorage || node->arrayMode().type() == Array::SlowPutArrayStorage) {
    1947                     if (!hasAnyArrayStorage(structure->indexingMode()))
    1948                         return false;
    1949 
    1950                     if (structure->typeInfo().interceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero())
    1951                         return false;
    1952 
    19531949                    JSValue value;
    19541950                    {
    19551951                        // ArrayStorage's Butterfly can be half-broken state.
    19561952                        auto locker = holdLock(array->cellLock());
     1953
     1954                        WTF::loadLoadFence();
     1955                        Butterfly* butterfly = array->butterfly();
     1956
     1957                        WTF::loadLoadFence();
     1958                        StructureID structureIDLate = array->structureID();
     1959
     1960                        if (structureIDEarly != structureIDLate)
     1961                            return false;
     1962
     1963                        Structure* structure = m_vm.getStructure(structureIDLate);
     1964                        if (!hasAnyArrayStorage(structure->indexingMode()))
     1965                            return false;
     1966
     1967                        if (structure->typeInfo().interceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero())
     1968                            return false;
    19571969
    19581970                        ArrayStorage* storage = butterfly->arrayStorage();
  • trunk/Source/JavaScriptCore/runtime/JSArray.cpp

    r237129 r239325  
    335335bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool addToFront, unsigned count)
    336336{
     337    ASSERT(cellLock().isLocked());
     338
    337339    ArrayStorage* storage = ensureArrayStorage(vm);
    338340    Butterfly* butterfly = storage->butterfly();
Note: See TracChangeset for help on using the changeset viewer.