JSObject::getDirectConcurrently should take the cell lock.

https://bugs.webkit.org/show_bug.cgi?id=257285 rdar://108166258 Reviewed by Yusuke Suzuki. `JSArray::unshiftCountWithArrayStorage` takes the cell lock and then the structure lock to prevent the compiler thread from accessing the butterfly before it is fully initialized. `JSObject::getDirectConcurrently` only takes the structure lock. This means that the compiler can take the structure lock, the cell can transition to a new structure, then unshift can mess up the butterfly, and finally the compiler thread proceeds to see garbage. The attached POC only reproduces if waits are introduced to extend the race window. It seems that the comment above cellLock is outdated, as our current concurrency protocol to prevent deadlocks is to take the cell lock then the structure lock. I could not find anywhere that uses the reverse, but if I missed something, a deadlock will be pretty easy to debug. * Source/JavaScriptCore/runtime/JSArray.cpp: (JSC::JSArray::unshiftCountWithArrayStorage): * Source/JavaScriptCore/runtime/JSCell.h: (JSC::JSCell::cellLock const): (JSC::JSCell::cellLock): Deleted. * Source/JavaScriptCore/runtime/JSObject.h: (JSC::JSObject::getDirectConcurrently const): Originally-landed-as: 259548.798@safari-7615-branch (b7e3ebd). rdar://108166258 Canonical link: https://commits.webkit.org/266435@main
WebKit · Jul 31, 2023 · 14530da · 14530da
1 parent 4a9d1a5
commit 14530da
Show file tree

Hide file tree

Showing 7 changed files with 123 additions and 22 deletions.
diff --git a/JSTests/stress/get-concurrently-should-take-cell-lock.js b/JSTests/stress/get-concurrently-should-take-cell-lock.js
@@ -0,0 +1,79 @@
+/*
+diff --git a/Source/JavaScriptCore/dfg/DFGGraph.cpp b/Source/JavaScriptCore/dfg/DFGGraph.cpp
+index 07aed18ca1b4..157a836cf50e 100644
+--- a/Source/JavaScriptCore/dfg/DFGGraph.cpp
++++ b/Source/JavaScriptCore/dfg/DFGGraph.cpp
+@@ -25,6 +25,7 @@
+ 
+ #include "config.h"
+ #include "DFGGraph.h"
++#include "runtime/JSCast.h"
+ 
+ #if ENABLE(DFG_JIT)
+ 
+@@ -1337,6 +1338,10 @@ JSValue Graph::tryGetConstantProperty(
+     Structure* structure = object->structure();
+     if (!structureSet.toStructureSet().contains(structure))
+         return JSValue();
++    
++    if (jsDynamicCast<JSArray*>(object)) {
++        usleep(1000 * 1000);
++    }
+ 
+     return object->getDirectConcurrently(structure, offset);
+ }
+diff --git a/Source/JavaScriptCore/runtime/JSArray.cpp b/Source/JavaScriptCore/runtime/JSArray.cpp
+index 7b8907517f9e..f18e5b3a9e0c 100644
+--- a/Source/JavaScriptCore/runtime/JSArray.cpp
++++ b/Source/JavaScriptCore/runtime/JSArray.cpp
+@@ -1033,6 +1033,9 @@ bool JSArray::unshiftCountWithArrayStorage(JSGlobalObject* globalObject, unsigne
+         Structure* structure = this->structure();
+         ConcurrentJSLocker structureLock(structure->lock());
+         Butterfly* newButterfly = storage->butterfly()->unshift(structure, count);
++
++        usleep(1000 * 2000);
++
+         storage = newButterfly->arrayStorage();
+         storage->m_indexBias -= count;
+         storage->setVectorLength(vectorLength + count);
+*/
+
+async function sleep(ms) {
+    return new Promise(resolve => {
+        setTimeout(() => {
+            resolve();
+        }, ms);
+    });
+}
+
+const container = {};
+
+function opt() {
+    const object = container.abc;
+
+    return object.x;
+}
+
+async function main() {
+    const array = [];
+    array[1000] = 1.1;
+    array.fill(1.1);
+    array.unshift(1.1);
+
+    array.__defineGetter__('x', () => 1);
+
+    container.abc = array;
+
+    for (let i = 0; i < 0x10000; i++) {
+        opt();
+    }
+
+    await sleep(250);
+
+    array.y = 0x2222;
+    array.unshift(1.1, 2.2, 3.3);
+
+    await sleep(2000);
+}
+
+main();
diff --git a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
@@ -230,13 +230,23 @@ ObjectPropertyCondition generateCondition(
         break;
     }
     case PropertyCondition::Equivalence: {
-        unsigned attributes;
-        PropertyOffset offset = structure->get(vm, concurrency, uid, attributes);
-        if (offset == invalidOffset)
-            return ObjectPropertyCondition();
-        JSValue value = object->getDirect(concurrency, structure, offset);
-        if (!value)
-            return ObjectPropertyCondition();
+        JSValue value;
+        {
+            Locker<JSCellLock> cellLocker { NoLockingNecessary };
+            if (concurrency != Concurrency::MainThread) {
+                cellLocker = Locker { object->cellLock() };
+                if (object->structure() != structure)
+                    return ObjectPropertyCondition();
+                // The structure might change from now on, but we are guaranteed to have a sane view of the butterfly.
+            }
+            unsigned attributes;
+            PropertyOffset offset = structure->get(vm, concurrency, uid, attributes);
+            if (offset == invalidOffset)
+                return ObjectPropertyCondition();
+            value = object->getDirect(cellLocker, concurrency, structure, offset);
+            if (!value)
+                return ObjectPropertyCondition();
+        }
         result = ObjectPropertyCondition::equivalence(vm, owner, object, uid, value);
         break;
     }

diff --git a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
@@ -282,6 +282,9 @@ bool PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint(
     }
 
     case Equivalence: {
+        Locker<JSCellLock> cellLocker { NoLockingNecessary };
+        if (concurrency != Concurrency::MainThread && base)
+            cellLocker = Locker { base->cellLock() };
         if (!base || base->structure() != structure) {
             // Conservatively return false, since we cannot verify this one without having the
             // object.
@@ -306,8 +309,8 @@ bool PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint(
             return false;
         }
 
-        JSValue currentValue = base->getDirect(concurrency, structure, currentOffset);
-        if (currentValue != requiredValue()) {
+        JSValue currentValue = base->getDirect(cellLocker, concurrency, structure, currentOffset);
+        if (currentValue != requiredValue() || !currentValue) {
             if (PropertyConditionInternal::verbose) {
                 dataLog(
                     "Invalid because the value is ", currentValue, " but we require ", requiredValue(),
@@ -509,9 +512,13 @@ bool PropertyCondition::isValidValueForPresence(JSValue value) const
 
 PropertyCondition PropertyCondition::attemptToMakeEquivalenceWithoutBarrier(JSObject* base) const
 {
-    Structure* structure = base->structure();
+    JSValue value;
+    {
+        Locker cellLocker { base->cellLock() };
+        Structure* structure = base->structure();
 
-    JSValue value = base->getDirectConcurrently(structure, offset());
+        value = base->getDirectConcurrently(cellLocker, structure, offset());
+    }
     if (!isValidValueForPresence(value))
         return PropertyCondition();
     return equivalenceWithoutBarrier(uid(), value);

diff --git a/Source/JavaScriptCore/dfg/DFGGraph.cpp b/Source/JavaScriptCore/dfg/DFGGraph.cpp
@@ -1341,11 +1341,12 @@ JSValue Graph::tryGetConstantProperty(
     // incompatible with the getDirect we're trying to do. The easiest way to do that is to
     // determine if the structure belongs to the proven set.
 
+    Locker cellLock { object->cellLock() };
     Structure* structure = object->structure();
     if (!structureSet.toStructureSet().contains(structure))
         return JSValue();
 
-    return object->getDirectConcurrently(structure, offset);
+    return object->getDirectConcurrently(cellLock, structure, offset);
 }
 
 JSValue Graph::tryGetConstantProperty(JSValue base, Structure* structure, PropertyOffset offset)

diff --git a/Source/JavaScriptCore/runtime/JSArray.cpp b/Source/JavaScriptCore/runtime/JSArray.cpp
@@ -1034,6 +1034,7 @@ bool JSArray::unshiftCountWithArrayStorage(JSGlobalObject* globalObject, unsigne
         Structure* structure = this->structure();
         ConcurrentJSLocker structureLock(structure->lock());
         Butterfly* newButterfly = storage->butterfly()->unshift(structure, count);
+
         storage = newButterfly->arrayStorage();
         storage->m_indexBias -= count;
         storage->setVectorLength(vectorLength + count);

diff --git a/Source/JavaScriptCore/runtime/JSCell.h b/Source/JavaScriptCore/runtime/JSCell.h
@@ -138,12 +138,12 @@ class JSCell : public HeapCell {
 
     // Each cell has a built-in lock. Currently it's simply available for use if you need it. It's
     // a full-blown WTF::Lock. Note that this lock is currently used in JSArray and that lock's
-    // ordering with the Structure lock is that the Structure lock must be acquired first.
+    // ordering with the Structure lock is that the cell lock must be acquired first.
 
     // We use this abstraction to make it easier to grep for places where we lock cells.
     // to lock a cell you can just do:
-    // Locker locker { cell->cellLocker() };
-    JSCellLock& cellLock() { return *reinterpret_cast<JSCellLock*>(this); }
+    // Locker locker { cell->cellLock() };
+    JSCellLock& cellLock() const { return *reinterpret_cast<JSCellLock*>(const_cast<JSCell*>(this)); }
 
     JSType type() const;
     IndexingType indexingTypeAndMisc() const;

diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h
@@ -823,8 +823,8 @@ class JSObject : public JSCell {
 
     // Fast access to known property offsets.
     ALWAYS_INLINE JSValue getDirect(PropertyOffset offset) const { return locationForOffset(offset)->get(); }
-    JSValue getDirect(Concurrency, Structure* expectedStructure, PropertyOffset) const;
-    JSValue getDirectConcurrently(Structure* expectedStructure, PropertyOffset) const;
+    JSValue getDirect(Locker<JSCellLock>&, Concurrency, Structure* expectedStructure, PropertyOffset) const;
+    JSValue getDirectConcurrently(Locker<JSCellLock>&, Structure* expectedStructure, PropertyOffset) const;
     void putDirectOffset(VM& vm, PropertyOffset offset, JSValue value) { locationForOffset(offset)->set(vm, this, value); }
     void putDirectWithoutBarrier(PropertyOffset offset, JSValue value) { locationForOffset(offset)->setWithoutWriteBarrier(value); }
 
@@ -1421,22 +1421,25 @@ inline JSValue JSObject::getPrototype(VM&, JSGlobalObject* globalObject)
 // shrink the butterfly when we are holding the Structure's ConcurrentJSLock, such as when we
 // flatten an object.
 IGNORE_RETURN_TYPE_WARNINGS_BEGIN
-ALWAYS_INLINE JSValue JSObject::getDirect(Concurrency concurrency, Structure* expectedStructure, PropertyOffset offset) const
+ALWAYS_INLINE JSValue JSObject::getDirect(Locker<JSCellLock>& cellLock, Concurrency concurrency, Structure* expectedStructure, PropertyOffset offset) const
 {
     switch (concurrency) {
     case Concurrency::MainThread:
         ASSERT(!isCompilationThread() && !Thread::mayBeGCThread());
         return getDirect(offset);
     case Concurrency::ConcurrentThread:
-        return getDirectConcurrently(expectedStructure, offset);
+        return getDirectConcurrently(cellLock, expectedStructure, offset);
     }
 }
 IGNORE_RETURN_TYPE_WARNINGS_END
 
-inline JSValue JSObject::getDirectConcurrently(Structure* structure, PropertyOffset offset) const
+inline JSValue JSObject::getDirectConcurrently(Locker<JSCellLock>&, Structure* expectedStructure, PropertyOffset offset) const
 {
-    ConcurrentJSLocker locker(structure->lock());
-    if (!structure->isValidOffset(offset))
+    // We always take the cell lock before the structure lock.
+    // We must take the cell lock to prevent places like JSArray::unshiftCountWithArrayStorage
+    // from changing the butterfly out from under us.
+    ConcurrentJSLocker locker { expectedStructure->lock() };
+    if (!expectedStructure->isValidOffset(offset))
         return { };
     return getDirect(offset);
 }