Bug 124611 – ARMv7: Crash due to use after free of AssemblerBuffer

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

One crash trace seen is:
* thread #1: tid = 0x18ef, 0x0015ffe2 JavaScriptCore`JSC::ARMv7Assembler::executableOffsetFor(int) + 58, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0x361672c)
    frame #0: 0x0015ffe2 JavaScriptCore`JSC::ARMv7Assembler::executableOffsetFor(int) + 58
    frame #1: 0x0015ffa2 JavaScriptCore`JSC::MacroAssemblerARMv7::executableOffsetFor(int) + 18
    frame #2: 0x0015ff80 JavaScriptCore`JSC::AssemblerLabel JSC::LinkBuffer::applyOffset<JSC::AssemblerLabel>(JSC::AssemblerLabel) + 20
    frame #3: 0x0015eb94 JavaScriptCore`JSC::LinkBuffer::locationOf(JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::Label) + 32
    frame #4: 0x0018e0ca JavaScriptCore`JSC::DFG::JITFinalizer::finalizeFunction() + 38
    frame #5: 0x001c18ba JavaScriptCore`JSC::DFG::Plan::finalizeWithoutNotifyingCallback() + 62
    frame #6: 0x00161504 JavaScriptCore`JSC::DFG::compileImpl(JSC::VM&, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue, JSC::OperandValueTraits<JSC::JSValue> > const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>, JSC::DFG::Worklist*) + 932
    frame #7: 0x00161116 JavaScriptCore`JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue, JSC::OperandValueTraits<JSC::JSValue> > const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>, JSC::DFG::Worklist*) + 138
    frame #8: 0x0027fdbc JavaScriptCore`operationOptimize + 1480
    frame #9: 0x44230b79

The issue is that the ARMv7Assembler object is a local in Plan::compileInThreadImpl() and has been freed. 

ANother more obscure stack trace is:
* thread #1: tid = 0x15f8, 0x00488726 JavaScriptCore`WTFCrash + 58 at Assertions.cpp:341, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x00488726 JavaScriptCore`WTFCrash + 58 at Assertions.cpp:341
    frame #1: 0x000ce5e0 JavaScriptCore`WTF::CrashOnOverflow::overflowed() + 8 at CheckedArithmetic.h:80
    frame #2: 0x000aa67e JavaScriptCore`WTF::Vector<WTF::Vector<JSC::DFG::OSRExit, 8ul, WTF::CrashOnOverflow>*, 0ul, WTF::CrashOnOverflow>::at(this=0x0125deb4, i=848195) + 54 at Vector.h:584
    frame #3: 0x000aa642 JavaScriptCore`WTF::Vector<WTF::Vector<JSC::DFG::OSRExit, 8ul, WTF::CrashOnOverflow>*, 0ul, WTF::CrashOnOverflow>::operator[](this=0x0125deb4, i=848195) + 18 at Vector.h:604
    frame #4: 0x000aa5ac JavaScriptCore`WTF::SegmentedVector<JSC::DFG::OSRExit, 8ul, 32ul>::segmentFor(this=0x0125deb0, index=6785564) + 28 at SegmentedVector.h:217
    frame #5: 0x000aa574 JavaScriptCore`WTF::SegmentedVector<JSC::DFG::OSRExit, 8ul, 32ul>::at(this=0x0125deb0, index=6785564) + 20 at SegmentedVector.h:128
    frame #6: 0x000aa18a JavaScriptCore`WTF::SegmentedVector<JSC::DFG::OSRExit, 8ul, 32ul>::operator[](this=0x0125deb0, index=6785564) + 18 at SegmentedVector.h:138
    frame #7: 0x001ba486 JavaScriptCore`compileOSRExit(exec=0x02bff7d8) + 246 at DFGOSRExitCompiler.cpp:61
    frame #8: 0x5dec2ca7
    frame #9: 0x0034e900 JavaScriptCore`llint_op_call + 218
…

[Michael Saboff]

<rdar://problem/15452643>

[Michael Saboff]

Created attachment 217341 [details]
Patch

[WebKit Commit Bot]

Attachment 217341 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/MacroAssembler.cpp', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp', u'Source/JavaScriptCore/dfg/DFGJITFinalizer.h']" exit_code: 1
Source/JavaScriptCore/assembler/MacroAssembler.cpp:1:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 217341 [details]
Patch

Attachment 217341 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/28628031

[Michael Saboff]

Created attachment 217344 [details]
Fixed inadvertent typo

[Geoffrey Garen]

Comment on attachment 217344 [details]
Fixed inadvertent typo

View in context: https://bugs.webkit.org/attachment.cgi?id=217344&action=review

> Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:44
> +    if (m_arityCheck.isSet())

The old code didn't check m_arityCheck.isSet(). Why do we need to check it now?

> Source/JavaScriptCore/dfg/DFGJITFinalizer.h:54
>      MacroAssembler::Label m_arityCheck;
> +    MacroAssemblerCodePtr m_withArityCheck;

It's not so great to have two data members with equivalent names, one of which works, and the other of which crashes your program. Can we remove the Label version of arityCheck? It appears to be used only in the JITFinalizer constructor function.

[Geoffrey Garen]

Comment on attachment 217344 [details]
Fixed inadvertent typo

View in context: https://bugs.webkit.org/attachment.cgi?id=217344&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        Changed JITFinalizer constructor to calculate and save the with-arity-check entry point.
> +        At that point, the assembler object is still valid.  In finalizeFunction(), we use that value

Which assembler object are you talking about? Was it locationOf() that was using the invalid assembler? Is there a risk that finalizeCodeWithoutDisassembly() will use the invalid assembler?

[Michael Saboff]

(In reply to comment #7)
> (From update of attachment 217344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217344&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        Changed JITFinalizer constructor to calculate and save the with-arity-check entry point.
> > +        At that point, the assembler object is still valid.  In finalizeFunction(), we use that value
> 
> Which assembler object are you talking about? Was it locationOf() that was using the invalid assembler? Is there a risk that finalizeCodeWithoutDisassembly() will use the invalid assembler?

The assembler object is the MacroAssembler* m_assembler in LinkBuffer.  Yes, it was LinkBuffer::locationOf() that was using the freed assembler.

finalizeCodeWithoutDisassembly() is making the buffer ready for execution.  It doesn't access the assembler.

The only reason that locationOf() accesses the assembler is due to branch compaction.  We store the delta in the assembler buffer as part of the compaction.  I didn't find any other uses of m_assembler after the code has been linked.

[Michael Saboff]

Created attachment 217374 [details]
Updated patch

Changed the JITFinalizer() to take a resolved MacroAssemblerCodePtr instead of a MacroAssembler::Label.

[Geoffrey Garen]

Comment on attachment 217374 [details]
Updated patch

r=me

It looks like, for the FTL JITFinalizer, we'll need to make a similar move away from Labels and toward post-relaxation pointers. Can you file a bug about that?

[Michael Saboff]

Committed r159577: <http://trac.webkit.org/changeset/159577>

[Michael Saboff]

(In reply to comment #10)
> (From update of attachment 217374 [details])
> r=me
> 
> It looks like, for the FTL JITFinalizer, we'll need to make a similar move away from Labels and toward post-relaxation pointers. Can you file a bug about that?

Added https://bugs.webkit.org/show_bug.cgi?id=124674 - "FTLJITFinalizer shouldn't keep Labels to the contained generated code"