Summary: | ARMv7: Crash due to use after free of AssemblerBuffer | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, rniwa | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | Windows 2000 | ||||||||||
Attachments: |
|
Description
Michael Saboff
2013-11-19 15:17:56 PST
Created attachment 217341 [details]
Patch
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.
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 Created attachment 217344 [details]
Fixed inadvertent typo
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. 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? (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. Created attachment 217374 [details]
Updated patch
Changed the JITFinalizer() to take a resolved MacroAssemblerCodePtr instead of a MacroAssembler::Label.
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?
Committed r159577: <http://trac.webkit.org/changeset/159577> (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" |