Attachment 343410 Details for Bug 186954 – Patch

[patch] Patch

bug-186954-20180622183658.patch (text/plain), 8.41 KB, created by Keith Miller on 2018-06-22 18:37:00 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2018-06-22 18:37:00 PDT

Size: 8.41 KB

patch

obsolete

>Subversion Revision: 233109
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 8942d1a9325b7c1ae9263bd6f917fc793806bdb8..93f73f29afc5e72e54fa1defe88b9de00a7d8b1c 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,27 @@
>+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,
>+        adds a getDirectConcurrently that will safely try to load from the
>+        butterfly.
>+
>+        * bytecode/ObjectPropertyConditionSet.cpp:
>+        * bytecode/PropertyCondition.cpp:
>+        (JSC::PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint const):
>+        (JSC::PropertyCondition::attemptToMakeEquivalenceWithoutBarrier const):
>+        * dfg/DFGGraph.cpp:
>+        (JSC::DFG::Graph::tryGetConstantProperty):
>+        * runtime/JSObject.h:
>+        (JSC::JSObject::getDirectConcurrently const):
>+
> 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/ObjectPropertyConditionSet.cpp b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
>index 49389c1a16356c173a483fea1a73d912a064bf7f..2d1963a422ee3c376b4fd73bcf5ec3642767b5b5 100644
>--- a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
>+++ b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
>@@ -222,7 +222,9 @@ ObjectPropertyCondition generateCondition(
>         PropertyOffset offset = structure->getConcurrently(uid, attributes);
>         if (offset == invalidOffset)
>             return ObjectPropertyCondition();
>-        JSValue value = object->getDirect(offset);
>+        JSValue value = object->getDirectConcurrently(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
>index 51d61c2173ab43a204e699a8ea93443acf71f7d0..14c55345091207c78805bbc6fda7630ccb911521 100644
>--- a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
>+++ b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
>@@ -237,7 +237,7 @@ bool PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint(
>             return false;
>         }
> 
>-        JSValue currentValue = base->getDirect(currentOffset);
>+        JSValue currentValue = base->getDirectConcurrently(structure, currentOffset);
>         if (currentValue != requiredValue()) {
>             if (PropertyConditionInternal::verbose) {
>                 dataLog(
>@@ -390,10 +390,9 @@ 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());
>-    if (!isValidValueForPresence(vm, value))
>+
>+    JSValue value = base->getDirectConcurrently(structure, offset());
>+    if (!value || !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..deb876d8b05e2ea8e1a6a3543c0b68456ed4c4e6 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,22 +1263,24 @@ 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();
>-    
>-    return object->getDirect(offset);
>+
>+    return object->getDirectConcurrently(structure, offset);
> }
> 
> JSValue Graph::tryGetConstantProperty(JSValue base, Structure* structure, PropertyOffset offset)
>diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h
>index 819346c765eb5043bb8fd7fdcc51cd2985dd1a5c..40c33ad435354d6e421cabb153525b1647cd50f9 100644
>--- a/Source/JavaScriptCore/runtime/JSObject.h
>+++ b/Source/JavaScriptCore/runtime/JSObject.h
>@@ -717,6 +717,7 @@ public:
> 
>     // Fast access to known property offsets.
>     ALWAYS_INLINE JSValue getDirect(PropertyOffset offset) const { return locationForOffset(offset)->get(); }
>+    JSValue getDirectConcurrently(Structure* expectedStructure, PropertyOffset) const;
>     void putDirect(VM& vm, PropertyOffset offset, JSValue value) { locationForOffset(offset)->set(vm, this, value); }
>     void putDirectWithoutBarrier(PropertyOffset offset, JSValue value) { locationForOffset(offset)->setWithoutWriteBarrier(value); }
>     void putDirectUndefined(PropertyOffset offset) { locationForOffset(offset)->setUndefined(); }
>@@ -1320,6 +1321,18 @@ inline JSValue JSObject::getPrototype(VM& vm, ExecState* exec)
>     return getPrototypeMethod(this, exec);
> }
> 
>+// Normally, we never shrink the butterfly so if we know an offset is valid for some
>+// past structure then it should be valid for any new structure. However, we may sometimes
>+// shrink the butterfly when we are holding the Structure's ConcurrentJSLock, such as when we
>+// flatten an object.
>+inline JSValue JSObject::getDirectConcurrently(Structure* structure, PropertyOffset offset) const
>+{
>+    ConcurrentJSLocker locker(structure->lock());
>+    if (!structure->isValidOffset(offset))
>+        return { };
>+    return getDirect(offset);
>+}
>+
> // It is safe to call this method with a PropertyName that is actually an index,
> // but if so will always return false (doesn't search index storage).
> ALWAYS_INLINE bool JSObject::getOwnNonIndexPropertySlot(VM& vm, Structure* structure, PropertyName propertyName, PropertySlot& slot)

Actions: View | Formatted Diff | Diff

Attachments on bug 186954: 343405 | 343410 | 343411