Changeset 244287 in webkit

Timestamp:

Apr 15, 2019 1:44:33 PM (5 years ago)

Author:

sbarati@apple.com

Message:

mergeOSREntryValue is wrong when the incoming value does not match up with the flush format
​https://bugs.webkit.org/show_bug.cgi?id=196918

Reviewed by Yusuke Suzuki.

r244238 lead to some debug failures because we were calling checkConsistency()
before doing fixTypeForRepresentation when merging in must handle values in
CFA. This patch fixes that.

However, as I was reading over mergeOSREntryValue, I realized it was wrong. It
was possible it could merge in a value/type outside of the variable's flushed type.
Once the flush format types are locked in, we can't introduce a type out of
that range. This probably never lead to any crashes as our profiling injection
and speculation decision code is solid. However, what we were doing is clearly
wrong, and something a fuzzer could have found if we fuzzed the must handle
values inside prediction injection. We should do that fuzzing:
​https://bugs.webkit.org/show_bug.cgi?id=196924

• dfg/DFGAbstractValue.cpp:

(JSC::DFG::AbstractValue::mergeOSREntryValue):

• dfg/DFGAbstractValue.h:
• dfg/DFGCFAPhase.cpp:

(JSC::DFG::CFAPhase::injectOSR):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGAbstractValue.cpp (modified) (2 diffs)
• dfg/DFGAbstractValue.h (modified) (2 diffs)
• dfg/DFGCFAPhase.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-04-15  Saam barati  <sbarati@apple.com>
+
+        mergeOSREntryValue is wrong when the incoming value does not match up with the flush format
+        https://bugs.webkit.org/show_bug.cgi?id=196918
+
+        Reviewed by Yusuke Suzuki.
+
+        r244238 lead to some debug failures because we were calling checkConsistency()
+        before doing fixTypeForRepresentation when merging in must handle values in
+        CFA. This patch fixes that.
+        
+        However, as I was reading over mergeOSREntryValue, I realized it was wrong. It
+        was possible it could merge in a value/type outside of the variable's flushed type.
+        Once the flush format types are locked in, we can't introduce a type out of
+        that range. This probably never lead to any crashes as our profiling injection
+        and speculation decision code is solid. However, what we were doing is clearly
+        wrong, and something a fuzzer could have found if we fuzzed the must handle
+        values inside prediction injection. We should do that fuzzing:
+        https://bugs.webkit.org/show_bug.cgi?id=196924
+
+        * dfg/DFGAbstractValue.cpp:
+        (JSC::DFG::AbstractValue::mergeOSREntryValue):
+        * dfg/DFGAbstractValue.h:
+        * dfg/DFGCFAPhase.cpp:
+        (JSC::DFG::CFAPhase::injectOSR):
+
 2019-04-15  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp

@@ -181 +181 @@
 }
 
-bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value)
-{
+bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value, VariableAccessData* variable, Node* node)
+{
+    FlushFormat flushFormat = variable->flushFormat();
+
+    {
+        if (flushFormat == FlushedDouble && value.isNumber())
+            value = jsDoubleNumber(value.asNumber());
+        SpeculatedType incomingType = resultFor(flushFormat) == NodeResultInt52 ? int52AwareSpeculationFromValue(value) : speculationFromValue(value);
+        SpeculatedType requiredType = typeFilterFor(flushFormat);
+        if (incomingType & ~requiredType)
+            return false;
+    }
+
     AbstractValue oldMe = *this;
     
@@ -208 +219 @@
     }
     
-    checkConsistency();
     assertIsRegistered(graph);
+
+    fixTypeForRepresentation(graph, resultFor(flushFormat), node);
+
+    checkConsistency();
     
     return oldMe != *this;

trunk/Source/JavaScriptCore/dfg/DFGAbstractValue.h

@@ -49 +49 @@
 class Graph;
 struct Node;
+class VariableAccessData;
 
 struct AbstractValue {
@@ -304 +305 @@
     }
     
-    bool mergeOSREntryValue(Graph&, JSValue);
+    bool mergeOSREntryValue(Graph&, JSValue, VariableAccessData*, Node*);
     
     void merge(SpeculatedType type)

trunk/Source/JavaScriptCore/dfg/DFGCFAPhase.cpp

@@ -189 +189 @@
             
             AbstractValue& target = block->valuesAtHead.operand(operand);
-            changed |= target.mergeOSREntryValue(m_graph, value.value());
-            target.fixTypeForRepresentation(
-                m_graph, resultFor(node->variableAccessData()->flushFormat()), node);
+            changed |= target.mergeOSREntryValue(m_graph, value.value(), node->variableAccessData(), node);
         }