Bug 119860 (CVE-2013-5195) - Crash during exception unwinding
Summary: Crash during exception unwinding
Alias: CVE-2013-5195
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
Keywords: InRadar
: 119902 (view as bug list)
Depends on:
Reported: 2013-08-15 13:36 PDT by Oliver Hunt
Modified: 2013-11-20 16:39 PST (History)
8 users (show)

See Also:

Patch (10.70 KB, patch)
2013-08-16 16:25 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (11.43 KB, patch)
2013-08-16 16:31 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (13.21 KB, patch)
2013-08-19 12:19 PDT, Oliver Hunt
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2013-08-15 13:36:48 PDT
Friendly test case:

function g() { 
	(eval("-7") = 0);
for (;;) try { g() } catch(e) {}
1   0x10b9c4680 WTFCrash
2   0x10b74402f JSC::JSActivation* JSC::jsCast<JSC::JSActivation*>(JSC::JSValue)
3   0x10b73f190 JSC::Interpreter::unwindCallFrame(JSC::StackIterator&, JSC::JSValue)
4   0x10b740362 JSC::Interpreter::throwException(JSC::ExecState*&, JSC::JSValue&, unsigned int)
5   0x10b76124f JSC::genericThrow(JSC::VM*, JSC::ExecState*, JSC::JSValue, unsigned int)
6   0x10b7614a6 JSC::jitThrow(JSC::VM*, JSC::ExecState*, JSC::JSValue, JSC::ReturnAddressPtr)
7   0x10b782146 cti_vm_throw
8   0x10b7830c0 jscGeneratedNativeCode
9   0x10b75eb07 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*)
10  0x10b741cc9 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
11  0x10b5486f1 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
12  0x10b40b167 runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow> const&, bool)
13  0x10b40a85c jscmain(int, char**)
14  0x10b40a6be main
15  0x7fff8def75fd start
Comment 1 Oliver Hunt 2013-08-15 16:48:46 PDT
As far as i can make out OSR exit is overwriting the activation register with undefined, but not the arguments register
Comment 2 Radar WebKit Bug Importer 2013-08-15 16:49:20 PDT
Comment 3 Oliver Hunt 2013-08-16 16:25:12 PDT
Created attachment 208963 [details]
Comment 4 Filip Pizlo 2013-08-16 16:27:08 PDT
Comment on attachment 208963 [details]

Why doesn't this have changes to DFGInPlaceAbstractState, and probably elsewhere, to remove Throw/ThrowReferenceError from the list of block terminals and add Unreachable to that same list?
Comment 5 Oliver Hunt 2013-08-16 16:31:00 PDT
Created attachment 208964 [details]
Comment 6 Filip Pizlo 2013-08-16 18:26:21 PDT
Comment on attachment 208964 [details]

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

Please change the comment in DFGAbstractInterpreter.h that mentions Throw as a terminal.

Please change the comment in DFGFlushLivenessAnalysis that mentions Throw as a terminal.  Ditto in DFGLivenessAnalysis.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1497
> +    case Unreachable:
> +        break;

Since Throw/ThrowReferenceError do setIsValid(false) and presumably all other uses of Unreachable will also be dominated by something that always exits/terminates, shouldn't this be RELEASE_ASSERT_NOT_REACHED()?

> Source/JavaScriptCore/dfg/DFGClobberize.h:114
> +    case Unreachable:

Since this is a control construct, shouldn't it write(SideState)?

> Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp:447
> +    case Unreachable:
> +        return false;

Why isn't this in the above case block, along with the cfaBranchDirection assertion?

> Source/JavaScriptCore/dfg/DFGNodeType.h:252
>      macro(Throw, NodeMustGenerate) \
>      macro(ThrowReferenceError, NodeMustGenerate) \

Shouldn't you be removing these from the block that has the "block terminals" comment?  Wouldn't they be more appropriate in another block of node types with a comment like "these always exit but aren't terminals to allow liveness hooks after them"?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4649
> +    case Unreachable:
> +        // We should not ever reach this code
> +        m_jit.breakpoint();
> +        noResult(node);
> +        break;

Shouldn't this be a RELEASE_ASSERT_NOT_REACHED() because the *compiler* shouldn't even reach this node?

(Ditto in the 32-bit compiler.)
Comment 7 Oliver Hunt 2013-08-19 12:19:49 PDT
Created attachment 209107 [details]
Comment 8 WebKit Commit Bot 2013-08-19 12:22:14 PDT
Attachment 209107 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/dfg-activation-register-overwritten-in-throw-expected.txt', u'LayoutTests/fast/js/dfg-activation-register-overwritten-in-throw.html', u'LayoutTests/fast/js/script-tests/dfg-activation-register-overwritten-in-throw.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGClobberize.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushLivenessAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSafeToExecute.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp']" exit_code: 1
Source/JavaScriptCore/dfg/DFGNodeType.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 18 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Filip Pizlo 2013-08-19 12:38:02 PDT
Comment on attachment 209107 [details]

Comment 10 Oliver Hunt 2013-08-19 12:39:09 PDT
Committed r154290: <http://trac.webkit.org/changeset/154290>
Comment 11 Oliver Hunt 2013-08-20 14:39:17 PDT
*** Bug 119902 has been marked as a duplicate of this bug. ***