Changeset 201590 in webkit

Timestamp:

Jun 1, 2016, 9:26:32 PM (10 years ago)

Author:

fpizlo@apple.com

Message:

Structure::previousID() races with Structure::allocateRareData()
​https://bugs.webkit.org/show_bug.cgi?id=158280

Reviewed by Mark Lam.

The problem is that previousID() would test hasRareData() and then either load the
previous Structure from the rare data, or load it directly. allocateRareData() would set
the hasRareData() bit separately from moving the Structure pointer into the rare data. So
we'd have a race that would cause previousID() to sometimes return the rarae data instead
of the previous Structure.

The fix is to get rid of the hasRareData bit. We can use the structureID of the
previousOrRareData cell to determine if it's the previousID or the RareData. This fixes the
race and it's probably not any slower.

• runtime/Structure.cpp:

(JSC::Structure::Structure):
(JSC::Structure::allocateRareData):

• runtime/Structure.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/Structure.cpp (modified) (4 diffs)
• runtime/Structure.h (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-06-01  Filip Pizlo  <fpizlo@apple.com>
+
+        Structure::previousID() races with Structure::allocateRareData()
+        https://bugs.webkit.org/show_bug.cgi?id=158280
+
+        Reviewed by Mark Lam.
+        
+        The problem is that previousID() would test hasRareData() and then either load the
+        previous Structure from the rare data, or load it directly. allocateRareData() would set
+        the hasRareData() bit separately from moving the Structure pointer into the rare data. So
+        we'd have a race that would cause previousID() to sometimes return the rarae data instead
+        of the previous Structure.
+
+        The fix is to get rid of the hasRareData bit. We can use the structureID of the
+        previousOrRareData cell to determine if it's the previousID or the RareData. This fixes the
+        race and it's probably not any slower.
+
+        * runtime/Structure.cpp:
+        (JSC::Structure::Structure):
+        (JSC::Structure::allocateRareData):
+        * runtime/Structure.h:
+
 2016-06-01  Michael Saboff  <msaboff@apple.com>
 

trunk/Source/JavaScriptCore/runtime/Structure.cpp

@@ -207 +207 @@
     setDidTransition(false);
     setStaticFunctionsReified(false);
-    setHasRareData(false);
     setTransitionWatchpointIsLikelyToBeFired(false);
     setHasBeenDictionary(false);
@@ -239 +238 @@
     setDidTransition(false);
     setStaticFunctionsReified(false);
-    setHasRareData(false);
     setTransitionWatchpointIsLikelyToBeFired(false);
     setHasBeenDictionary(false);
@@ -270 +268 @@
     setDidTransition(true);
     setStaticFunctionsReified(previous->staticFunctionsReified());
-    setHasRareData(false);
     setHasBeenDictionary(previous->hasBeenDictionary());
  
@@ -824 +821 @@
 {
     ASSERT(!hasRareData());
-    StructureRareData* rareData = StructureRareData::create(vm, previous());
+    StructureRareData* rareData = StructureRareData::create(vm, previousID());
     WTF::storeStoreFence();
     m_previousOrRareData.set(vm, this, rareData);
-    WTF::storeStoreFence();
-    setHasRareData(true);
     ASSERT(hasRareData());
 }

trunk/Source/JavaScriptCore/runtime/Structure.h

@@ -275 +275 @@
     // Will just the prototype chain intercept this property access?
     JS_EXPORT_PRIVATE bool prototypeChainMayInterceptStoreTo(VM&, PropertyName);
-        
+    
+    bool hasRareData() const
+    {
+        return isRareData(m_previousOrRareData.get());
+    }
+    
     Structure* previousID() const
     {
         ASSERT(structure()->classInfo() == info());
-        if (hasRareData())
-            return rareData()->previousID();
-        return previous();
+        // This is so written because it's used concurrently. We only load from m_previousOrRareData
+        // once, and this load is guaranteed atomic.
+        JSCell* cell = m_previousOrRareData.get();
+        if (isRareData(cell))
+            return static_cast<StructureRareData*>(cell)->previousID();
+        return static_cast<Structure*>(cell);
     }
     bool transitivelyTransitionedFrom(Structure* structureToFind);
@@ -603 +611 @@
     DEFINE_BITFIELD(bool, didTransition, DidTransition, 1, 21);
     DEFINE_BITFIELD(bool, staticFunctionsReified, StaticFunctionsReified, 1, 22);
-    DEFINE_BITFIELD(bool, hasRareData, HasRareData, 1, 23);
-    DEFINE_BITFIELD(bool, hasBeenFlattenedBefore, HasBeenFlattenedBefore, 1, 24);
-    DEFINE_BITFIELD(bool, hasCustomGetterSetterProperties, HasCustomGetterSetterProperties, 1, 25);
-    DEFINE_BITFIELD(bool, didWatchInternalProperties, DidWatchInternalProperties, 1, 26);
-    DEFINE_BITFIELD(bool, transitionWatchpointIsLikelyToBeFired, TransitionWatchpointIsLikelyToBeFired, 1, 27);
-    DEFINE_BITFIELD(bool, hasBeenDictionary, HasBeenDictionary, 1, 28);
+    DEFINE_BITFIELD(bool, hasBeenFlattenedBefore, HasBeenFlattenedBefore, 1, 23);
+    DEFINE_BITFIELD(bool, hasCustomGetterSetterProperties, HasCustomGetterSetterProperties, 1, 24);
+    DEFINE_BITFIELD(bool, didWatchInternalProperties, DidWatchInternalProperties, 1, 25);
+    DEFINE_BITFIELD(bool, transitionWatchpointIsLikelyToBeFired, TransitionWatchpointIsLikelyToBeFired, 1, 26);
+    DEFINE_BITFIELD(bool, hasBeenDictionary, HasBeenDictionary, 1, 27);
 
 private:
@@ -694 +701 @@
         
     void pin();
-
-    Structure* previous() const
-    {
-        ASSERT(!hasRareData());
-        return static_cast<Structure*>(m_previousOrRareData.get());
+    
+    bool isRareData(JSCell* cell) const
+    {
+        return cell && cell->structureID() != structureID();
     }