Changeset 221607 in webkit


Ignore:
Timestamp:
Sep 4, 2017, 9:10:53 PM (8 years ago)
Author:
[email protected]
Message:

typeCheckHoistingPhase may emit a CheckStructure on the empty value which leads to a dereference of zero on 64 bit platforms
​https://bugs.webkit.org/show_bug.cgi?id=176317

Reviewed by Keith Miller.

JSTests:

  • stress/dont-crash-when-hoist-check-structure-on-tdz.js: Added.

(Foo):

Source/JavaScriptCore:

It turns out that TypeCheckHoistingPhase may hoist a CheckStructure up to
the SetLocal of a particular value where the value is the empty JSValue.
On 64-bit platforms, the empty value is zero. This means that the empty value
passes a cell check. This will lead to a crash when we dereference null to load
the value's structure. This patch teaches TypeCheckHoistingPhase to be conservative
in the structure checks it hoists. On 64-bit platforms, instead of emitting a
CheckStructure node, we now emit a CheckStructureOrEmpty node. This node allows
the empty value to flow through. If the value isn't empty, it'll perform the normal
structure check that CheckStructure performs. For now, we only emit CheckStructureOrEmpty
on 64-bit platforms since a cell check on 32-bit platforms does not allow the empty
value to flow through.

  • dfg/DFGAbstractInterpreterInlines.h:

(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):

  • dfg/DFGArgumentsEliminationPhase.cpp:
  • dfg/DFGClobberize.h:

(JSC::DFG::clobberize):

  • dfg/DFGConstantFoldingPhase.cpp:

(JSC::DFG::ConstantFoldingPhase::foldConstants):

  • dfg/DFGDoesGC.cpp:

(JSC::DFG::doesGC):

  • dfg/DFGFixupPhase.cpp:

(JSC::DFG::FixupPhase::fixupNode):

  • dfg/DFGNode.h:

(JSC::DFG::Node::convertCheckStructureOrEmptyToCheckStructure):
(JSC::DFG::Node::hasStructureSet):

  • dfg/DFGNodeType.h:
  • dfg/DFGObjectAllocationSinkingPhase.cpp:
  • dfg/DFGPredictionPropagationPhase.cpp:
  • dfg/DFGSafeToExecute.h:

(JSC::DFG::SafeToExecuteEdge::SafeToExecuteEdge):
(JSC::DFG::SafeToExecuteEdge::operator()):
(JSC::DFG::SafeToExecuteEdge::maySeeEmptyChild):
(JSC::DFG::safeToExecute):

  • dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::emitStructureCheck):
(JSC::DFG::SpeculativeJIT::compileCheckStructure):

  • dfg/DFGSpeculativeJIT.h:
  • dfg/DFGSpeculativeJIT32_64.cpp:

(JSC::DFG::SpeculativeJIT::compile):

  • dfg/DFGSpeculativeJIT64.cpp:

(JSC::DFG::SpeculativeJIT::compile):

  • dfg/DFGTypeCheckHoistingPhase.cpp:

(JSC::DFG::TypeCheckHoistingPhase::run):

  • dfg/DFGValidate.cpp:
  • ftl/FTLCapabilities.cpp:

(JSC::FTL::canCompile):

  • ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::compileNode):
(JSC::FTL::DFG::LowerDFGToB3::compileCheckStructureOrEmpty):

Location:
trunk
Files:
1 added
21 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r221601 r221607  
     12017-09-04  Saam Barati  <[email protected]>
     2
     3        typeCheckHoistingPhase may emit a CheckStructure on the empty value which leads to a dereference of zero on 64 bit platforms
     4        https://bugs.webkit.org/show_bug.cgi?id=176317
     5
     6        Reviewed by Keith Miller.
     7
     8        * stress/dont-crash-when-hoist-check-structure-on-tdz.js: Added.
     9        (Foo):
     10
    1112017-09-03  Yusuke Suzuki  <[email protected]>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r221602 r221607  
     12017-09-04  Saam Barati  <[email protected]>
     2
     3        typeCheckHoistingPhase may emit a CheckStructure on the empty value which leads to a dereference of zero on 64 bit platforms
     4        https://bugs.webkit.org/show_bug.cgi?id=176317
     5
     6        Reviewed by Keith Miller.
     7
     8        It turns out that TypeCheckHoistingPhase may hoist a CheckStructure up to
     9        the SetLocal of a particular value where the value is the empty JSValue.
     10        On 64-bit platforms, the empty value is zero. This means that the empty value
     11        passes a cell check. This will lead to a crash when we dereference null to load
     12        the value's structure. This patch teaches TypeCheckHoistingPhase to be conservative
     13        in the structure checks it hoists. On 64-bit platforms, instead of emitting a
     14        CheckStructure node, we now emit a CheckStructureOrEmpty node. This node allows
     15        the empty value to flow through. If the value isn't empty, it'll perform the normal
     16        structure check that CheckStructure performs. For now, we only emit CheckStructureOrEmpty
     17        on 64-bit platforms since a cell check on 32-bit platforms does not allow the empty
     18        value to flow through.
     19
     20        * dfg/DFGAbstractInterpreterInlines.h:
     21        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
     22        * dfg/DFGArgumentsEliminationPhase.cpp:
     23        * dfg/DFGClobberize.h:
     24        (JSC::DFG::clobberize):
     25        * dfg/DFGConstantFoldingPhase.cpp:
     26        (JSC::DFG::ConstantFoldingPhase::foldConstants):
     27        * dfg/DFGDoesGC.cpp:
     28        (JSC::DFG::doesGC):
     29        * dfg/DFGFixupPhase.cpp:
     30        (JSC::DFG::FixupPhase::fixupNode):
     31        * dfg/DFGNode.h:
     32        (JSC::DFG::Node::convertCheckStructureOrEmptyToCheckStructure):
     33        (JSC::DFG::Node::hasStructureSet):
     34        * dfg/DFGNodeType.h:
     35        * dfg/DFGObjectAllocationSinkingPhase.cpp:
     36        * dfg/DFGPredictionPropagationPhase.cpp:
     37        * dfg/DFGSafeToExecute.h:
     38        (JSC::DFG::SafeToExecuteEdge::SafeToExecuteEdge):
     39        (JSC::DFG::SafeToExecuteEdge::operator()):
     40        (JSC::DFG::SafeToExecuteEdge::maySeeEmptyChild):
     41        (JSC::DFG::safeToExecute):
     42        * dfg/DFGSpeculativeJIT.cpp:
     43        (JSC::DFG::SpeculativeJIT::emitStructureCheck):
     44        (JSC::DFG::SpeculativeJIT::compileCheckStructure):
     45        * dfg/DFGSpeculativeJIT.h:
     46        * dfg/DFGSpeculativeJIT32_64.cpp:
     47        (JSC::DFG::SpeculativeJIT::compile):
     48        * dfg/DFGSpeculativeJIT64.cpp:
     49        (JSC::DFG::SpeculativeJIT::compile):
     50        * dfg/DFGTypeCheckHoistingPhase.cpp:
     51        (JSC::DFG::TypeCheckHoistingPhase::run):
     52        * dfg/DFGValidate.cpp:
     53        * ftl/FTLCapabilities.cpp:
     54        (JSC::FTL::canCompile):
     55        * ftl/FTLLowerDFGToB3.cpp:
     56        (JSC::FTL::DFG::LowerDFGToB3::compileNode):
     57        (JSC::FTL::DFG::LowerDFGToB3::compileCheckStructureOrEmpty):
     58
    1592017-09-04  Saam Barati  <[email protected]>
    260

  • trunk/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h

    r221602 r221607  
    24072407        break;
    24082408    }
     2409
     2410    case CheckStructureOrEmpty: {
     2411        AbstractValue& value = forNode(node->child1());
     2412
     2413        bool mayBeEmpty = value.m_type & SpecEmpty;
     2414        if (!mayBeEmpty)
     2415            m_state.setFoundConstants(true);
     2416
     2417        SpeculatedType admittedTypes = mayBeEmpty ? SpecEmpty : SpecNone;
     2418        filter(value, node->structureSet(), admittedTypes);
     2419        break;
     2420    }
    24092421       
    24102422    case CheckStructureImmediate: {

  • trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp

    r221528 r221607  
    371371                    break;
    372372
     373                case CheckStructureOrEmpty:
    373374                case CheckStructure: {
    374375                    if (!m_candidates.contains(node->child1().node()))
    … …  
    10971098                }
    10981099
     1100                case CheckStructureOrEmpty:
    10991101                case CheckStructure:
    11001102                    if (!isEliminatedAllocation(node->child1().node()))

  • trunk/Source/JavaScriptCore/dfg/DFGClobberize.h

    r221602 r221607  
    959959    }
    960960       
     961    case CheckStructureOrEmpty:
    961962    case CheckStructure:
    962963        read(JSCell_structureID);

  • trunk/Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp

    r221601 r221607  
    162162            }
    163163
    164                
     164            case CheckStructureOrEmpty: {
     165                const AbstractValue& value = m_state.forNode(node->child1());
     166                if (value.m_type & SpecEmpty)
     167                    break;
     168                node->convertCheckStructureOrEmptyToCheckStructure();
     169                changed = true;
     170                FALLTHROUGH;
     171            }
    165172            case CheckStructure:
    166173            case ArrayifyToStructure: {

  • trunk/Source/JavaScriptCore/dfg/DFGDoesGC.cpp

    r221602 r221607  
    116116    case DeleteByVal:
    117117    case CheckStructure:
     118    case CheckStructureOrEmpty:
     119    case CheckStructureImmediate:
    118120    case GetExecutable:
    119121    case GetButterfly:
    … …  
    264266    case ForwardVarargs:
    265267    case PutHint:
    266     case CheckStructureImmediate:
    267268    case PutStack:
    268269    case KillStack:

  • trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

    r221602 r221607  
    15841584        case PutHint:
    15851585        case CheckStructureImmediate:
     1586        case CheckStructureOrEmpty:
    15861587        case MaterializeNewObject:
    15871588        case MaterializeCreateActivation:

  • trunk/Source/JavaScriptCore/dfg/DFGNode.h

    r221602 r221607  
    431431        setOpAndDefaultFlags(CheckStructure);
    432432        m_opInfo = set;
     433    }
     434
     435    void convertCheckStructureOrEmptyToCheckStructure()
     436    {
     437        ASSERT(op() == CheckStructureOrEmpty);
     438        setOpAndDefaultFlags(CheckStructure);
    433439    }
    434440
    … …  
    17171723        switch (op()) {
    17181724        case CheckStructure:
     1725        case CheckStructureOrEmpty:
    17191726        case CheckStructureImmediate:
    17201727        case MaterializeNewObject:

  • trunk/Source/JavaScriptCore/dfg/DFGNodeType.h

    r221602 r221607  
    204204    macro(DeleteByVal, NodeResultBoolean | NodeMustGenerate) \
    205205    macro(CheckStructure, NodeMustGenerate) \
     206    macro(CheckStructureOrEmpty, NodeMustGenerate) \
    206207    macro(GetExecutable, NodeResultJS) \
    207208    macro(PutStructure, NodeMustGenerate) \

  • trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

    r221560 r221607  
    893893            break;
    894894
     895        case CheckStructureOrEmpty:
    895896        case CheckStructure: {
    896897            Allocation* allocation = m_heap.onlyLocalAllocation(node->child1().node());

  • trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp

    r221602 r221607  
    10701070        case PutHint:
    10711071        case CheckStructureImmediate:
     1072        case CheckStructureOrEmpty:
    10721073        case MaterializeNewObject:
    10731074        case MaterializeCreateActivation:
    … …  
    10821083        case LazyJSConstant:
    10831084        case CallDOM: {
    1084             // This node should never be visible at this stage of compilation. It is
    1085             // inserted by fixup(), which follows this phase.
     1085            // This node should never be visible at this stage of compilation.
    10861086            DFG_CRASH(m_graph, m_currentNode, "Unexpected node during prediction propagation");
    10871087            break;

  • trunk/Source/JavaScriptCore/dfg/DFGSafeToExecute.h

    r221602 r221607  
    3737    SafeToExecuteEdge(AbstractStateType& state)
    3838        : m_state(state)
    39         , m_result(true)
    4039    {
    4140    }
    … …  
    4342    void operator()(Node*, Edge edge)
    4443    {
     44        m_maySeeEmptyChild |= !!(m_state.forNode(edge).m_type & SpecEmpty);
     45
    4546        switch (edge.useKind()) {
    4647        case UntypedUse:
    … …  
    111112   
    112113    bool result() const { return m_result; }
     114    bool maySeeEmptyChild() const { return m_maySeeEmptyChild; }
    113115private:
    114116    AbstractStateType& m_state;
    115     bool m_result;
     117    bool m_result { true };
     118    bool m_maySeeEmptyChild { false };
    116119};
    117120
    … …  
    134137    if (!safeToExecuteEdge.result())
    135138        return false;
     139
     140    if (safeToExecuteEdge.maySeeEmptyChild()) {
     141        // We conservatively assume if the empty value flows into a node,
     142        // it might not be able to handle it (e.g, crash). In general, the bytecode generator
     143        // emits code in such a way that most node types don't need to worry about the empty value
     144        // because they will never see it. However, code motion has to consider the empty
     145        // value so it does not insert/move nodes to a place where they will crash. E.g, the
     146        // type check hoisting phase needs to insert CheckStructureOrEmpty instead of CheckStructure
     147        // for hoisted structure checks because it can not guarantee that a particular local is not
     148        // the empty value.
     149        switch (node->op()) {
     150        case CheckNotEmpty:
     151        case CheckStructureOrEmpty:
     152            break;
     153        default:
     154            return false;
     155        }
     156    }
    136157
    137158    // NOTE: This tends to lie when it comes to effectful nodes, because it knows that they aren't going to
    … …  
    215236    case DefineAccessorProperty:
    216237    case CheckStructure:
     238    case CheckStructureOrEmpty:
    217239    case GetExecutable:
    218240    case GetButterfly:

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

    r221602 r221607  
    78927892}
    78937893
    7894 void SpeculativeJIT::compileCheckStructure(Node* node, GPRReg cellGPR, GPRReg tempGPR)
     7894void SpeculativeJIT::emitStructureCheck(Node* node, GPRReg cellGPR, GPRReg tempGPR)
    78957895{
    78967896    ASSERT(node->structureSet().size());
    … …  
    79377937    case KnownCellUse: {
    79387938        SpeculateCellOperand cell(this, node->child1());
    7939         compileCheckStructure(node, cell.gpr(), InvalidGPRReg);
     7939        emitStructureCheck(node, cell.gpr(), InvalidGPRReg);
    79407940        noResult(node);
    79417941        return;
    … …  
    79557955        JITCompiler::Jump done = m_jit.jump();
    79567956        cell.link(&m_jit);
    7957         compileCheckStructure(node, valueRegs.payloadGPR(), tempGPR);
     7957        emitStructureCheck(node, valueRegs.payloadGPR(), tempGPR);
    79587958        done.link(&m_jit);
    79597959        noResult(node);

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h

    r221601 r221607  
    29072907    void compileIsFunction(Node*);
    29082908    void compileTypeOf(Node*);
    2909     void compileCheckStructure(Node*, GPRReg cellGPR, GPRReg tempGPR);
    29102909    void compileCheckStructure(Node*);
     2910    void emitStructureCheck(Node*, GPRReg cellGPR, GPRReg tempGPR);
    29112911    void compilePutAccessorById(Node*);
    29122912    void compilePutGetterSetterById(Node*);

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp

    r221602 r221607  
    56585658        break;
    56595659    }
     5660
     5661    case CheckStructureOrEmpty:
     5662        DFG_CRASH(m_jit.graph(), node, "CheckStructureOrEmpty only used in 64-bit DFG");
     5663        break;
    56605664
    56615665    case LastNodeType:

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

    r221602 r221607  
    46354635    }
    46364636       
     4637    case CheckStructureOrEmpty: {
     4638        SpeculateCellOperand cell(this, node->child1());
     4639        GPRReg cellGPR = cell.gpr();
     4640        MacroAssembler::Jump isEmpty;
     4641        if (m_interpreter.forNode(node->child1()).m_type & SpecEmpty)
     4642            isEmpty = m_jit.branchTest64(MacroAssembler::Zero, cellGPR);
     4643
     4644        emitStructureCheck(node, cellGPR, InvalidGPRReg);
     4645
     4646        if (isEmpty.isSet())
     4647            isEmpty.link(&m_jit);
     4648
     4649        noResult(node);
     4650        break;
     4651    }
     4652
    46374653    case CheckStructure: {
    46384654        compileCheckStructure(node);

  • trunk/Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp

    r221196 r221607  
    179179                   
    180180                    if (iter->value.m_structure) {
     181                        // Note: On 64-bit platforms, cell checks allow the empty value to flow through.
     182                        // This means that this structure check may see the empty value as input. We need
     183                        // to emit a node that explicitly handles the empty value. Most of the time, CheckStructureOrEmpty
     184                        // will be folded to CheckStructure because AI proves that the incoming value is
     185                        // definitely not empty.
     186                        static_assert(is64Bit() || !(SpecCellCheck & SpecEmpty), "");
    181187                        insertionSet.insertNode(
    182                             indexForChecks, SpecNone, CheckStructure,
     188                            indexForChecks, SpecNone, is64Bit() ? CheckStructureOrEmpty : CheckStructure,
    183189                            originForChecks.withSemantic(origin.semantic),
    184190                            OpInfo(m_graph.addStructureSet(iter->value.m_structure)),

  • trunk/Source/JavaScriptCore/dfg/DFGValidate.cpp

    r221602 r221607  
    277277                    VALIDATE((node), !!node->child1());
    278278                    VALIDATE((node), !!node->cellOperand()->value() && node->cellOperand()->value().isCell());
     279                    break;
     280                case CheckStructureOrEmpty:
     281                    VALIDATE((node), is64Bit());
     282                    VALIDATE((node), !!node->child1());
     283                    VALIDATE((node), node->child1().useKind() == CellUse);
    279284                    break;
    280285                case CheckStructure:

  • trunk/Source/JavaScriptCore/ftl/FTLCapabilities.cpp

    r221602 r221607  
    6666    case BitURShift:
    6767    case CheckStructure:
     68    case CheckStructureOrEmpty:
    6869    case DoubleAsInt32:
    6970    case ArrayifyToStructure:

  • trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

    r221602 r221607  
    645645            compileCheckStructure();
    646646            break;
     647        case CheckStructureOrEmpty:
     648            compileCheckStructureOrEmpty();
     649            break;
    647650        case CheckCell:
    648651            compileCheckCell();
    … …  
    27622765            DFG_CRASH(m_graph, m_node, "Bad use kind");
    27632766            return;
     2767        }
     2768    }
     2769
     2770    void compileCheckStructureOrEmpty()
     2771    {
     2772        ExitKind exitKind;
     2773        if (m_node->child1()->hasConstant())
     2774            exitKind = BadConstantCache;
     2775        else
     2776            exitKind = BadCache;
     2777
     2778        LValue cell = lowCell(m_node->child1());
     2779        bool maySeeEmptyValue = m_interpreter.forNode(m_node->child1()).m_type & SpecEmpty;
     2780        LBasicBlock notEmpty;
     2781        LBasicBlock continuation;
     2782        LBasicBlock lastNext;
     2783        if (maySeeEmptyValue) {
     2784            notEmpty = m_out.newBlock();
     2785            continuation = m_out.newBlock();
     2786            m_out.branch(m_out.isZero64(cell), unsure(continuation), unsure(notEmpty));
     2787            lastNext = m_out.appendTo(notEmpty, continuation);
     2788        }
     2789
     2790        checkStructure(
     2791            m_out.load32(cell, m_heaps.JSCell_structureID), jsValueValue(cell),
     2792            exitKind, m_node->structureSet(),
     2793            [&] (RegisteredStructure structure) {
     2794                return weakStructureID(structure);
     2795            });
     2796
     2797        if (maySeeEmptyValue) {
     2798            m_out.jump(continuation);
     2799            m_out.appendTo(continuation, lastNext);
    27642800        }
    27652801    }
Note: See TracChangeset for help on using the changeset viewer.