Attachment 343405 Details for Bug 186954 – Patch

[patch] Patch

bug-186954-20180622175237.patch (text/plain), 5.46 KB, created by Keith Miller on 2018-06-22 17:52:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2018-06-22 17:52:40 PDT

Size: 5.46 KB

patch

obsolete

>Subversion Revision: 233109
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 8942d1a9325b7c1ae9263bd6f917fc793806bdb8..a34ed7b2cb646c3c634102af818232165ecfe125 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,24 @@
>+2018-06-22  Keith Miller  <keith_miller@apple.com>
>+
>+        ObjectPropertyCondition and Graph::tryGetConstantProperty should lock the structure before calling getDirect
>+        https://bugs.webkit.org/show_bug.cgi?id=186954
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        It used to be that the propertyStorage of an object never shrunk
>+        so if you called getDirect with some offset it would never be an
>+        OOB read. However, this property storage can shrink when calling
>+        flattenDictionaryStructure. Fortunately, flattenDictionaryStructure
>+        holds the Structure's ConcurrentJSLock while shrinking. This patch,
>+        changes ObjectPropertyCondition::attemptToMakeEquivalenceWithoutBarrier
>+        and Graph::tryGetConstantProperty to use the Structure's
>+        ConcurrentJSLock when attempting to load an offset from an object.
>+
>+        * bytecode/PropertyCondition.cpp:
>+        (JSC::PropertyCondition::attemptToMakeEquivalenceWithoutBarrier const):
>+        * dfg/DFGGraph.cpp:
>+        (JSC::DFG::Graph::tryGetConstantProperty):
>+
> 2018-06-22  Saam Barati  <sbarati@apple.com>
> 
>         ensureWritableX should only convert away from CoW when it will succeed
>diff --git a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
>index 51d61c2173ab43a204e699a8ea93443acf71f7d0..a0ce1bb09e9d87bfe60afb4683a33262d6e612a3 100644
>--- a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
>+++ b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
>@@ -390,9 +390,15 @@ bool PropertyCondition::isValidValueForPresence(VM& vm, JSValue value) const
> PropertyCondition PropertyCondition::attemptToMakeEquivalenceWithoutBarrier(VM& vm, JSObject* base) const
> {
>     Structure* structure = base->structure(vm);
>-    if (!structure->isValidOffset(offset()))
>-        return PropertyCondition();
>-    JSValue value = base->getDirect(offset());
>+
>+    JSValue value;
>+    {
>+        // If we are flattening the structure at this time we might shrink the butterfly and read garbage.
>+        ConcurrentJSLocker locker(structure->lock());
>+        if (!structure->isValidOffset(offset()))
>+            return PropertyCondition();
>+        value = base->getDirect(offset());
>+    }
>     if (!isValidValueForPresence(vm, value))
>         return PropertyCondition();
>     return equivalenceWithoutBarrier(uid(), value);
>diff --git a/Source/JavaScriptCore/dfg/DFGGraph.cpp b/Source/JavaScriptCore/dfg/DFGGraph.cpp
>index 95c6412979d35773608bac2a9cd9f7c3553d8f82..3d006c48579f7aaa2aefb9729ecee510e426186d 100644
>--- a/Source/JavaScriptCore/dfg/DFGGraph.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGGraph.cpp
>@@ -1244,7 +1244,7 @@ JSValue Graph::tryGetConstantProperty(
>     
>     for (unsigned i = structureSet.size(); i--;) {
>         RegisteredStructure structure = structureSet[i];
>-        
>+
>         WatchpointSet* set = structure->propertyReplacementWatchpointSet(offset);
>         if (!set || !set->isStillValid())
>             return JSValue();
>@@ -1263,21 +1263,27 @@ JSValue Graph::tryGetConstantProperty(
>     //
>     // One argument in favor of this code is that it should definitely work because the butterfly
>     // is always set before the structure. However, we don't currently have a fence between those
>-    // stores. It's not clear if this matters, however. We don't ever shrink the property storage.
>-    // So, for this to fail, you'd need an access on a constant object pointer such that the inline
>-    // caches told us that the object had a structure that it did not *yet* have, and then later,
>-    // the object transitioned to that structure that the inline caches had alraedy seen. And then
>-    // the processor reordered the stores. Seems unlikely and difficult to test. I believe that
>-    // this is worth revisiting but it isn't worth losing sleep over. Filed:
>+    // stores. It's not clear if this matters, however. We only shrink the propertyStorage while
>+    // holding the Structure's lock. So, for this to fail, you'd need an access on a constant
>+    // object pointer such that the inlinecaches told us that the object had a structure that it
>+    // did not *yet* have, and then later,the object transitioned to that structure that the inline
>+    // caches had alraedy seen. And then the processor reordered the stores. Seems unlikely and
>+    // difficult to test. I believe that this is worth revisiting but it isn't worth losing sleep
>+    // over. Filed:
>     // https://bugs.webkit.org/show_bug.cgi?id=134641
>     //
>     // For now, we just do the minimal thing: defend against the structure right now being
>     // 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.
>-    
>-    if (!structureSet.toStructureSet().contains(object->structure(m_vm)))
>+
>+    Structure* structure = object->structure(m_vm);
>+    if (!structureSet.toStructureSet().contains(structure))
>+        return JSValue();
>+
>+    // If the mutator decided to flatten the object here we might shrink the butterfly and thus read garbage.
>+    ConcurrentJSLocker locker(structure->lock());
>+    if (!structure->isValidOffset(offset))
>         return JSValue();
>-    
>     return object->getDirect(offset);
> }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 186954: 343405 | 343410 | 343411