The following testcase asserts on mozilla-central revision 80fed51ae074 (run with --ion-eager): new Function("\ actual = Array.indexOf();\ actual = .indexOf();\ actual += Array.indexOf(actual);\ ")();
Crash Trace: ==2306== Invalid read of size 4 ==2306== at 0x7339E3: js::ion::IonFrameIterator::ionScript() const (IonFrames.cpp:69) ==2306== by 0x7343AA: _ZN2js3ion16SnapshotIteratorC2ERKNS0_16IonFrameIteratorE.constprop.140 (IonFrames.cpp:736) ==2306== by 0x735E48: js::ion::GetPcScript(JSContext*, JSScript**, unsigned char**) (IonFrames.cpp:892) ==2306== by 0x41AE78: _ZNK2js12ContextStack13currentScriptEPPhNS0_26MaybeAllowCrossCompartmentE.constprop.461 (Stack-inl.h:564) ==2306== by 0x423653: JS::CompileOptions::CompileOptions(JSContext*) (jscntxtinlines.h:466) ==2306== by 0x525258: js::CloneScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSFunction*>, JS::Handle<JSScript*>) (jsscript.cpp:2267) ==2306== by 0x61365E: JSRuntime::cloneSelfHostedFunctionScript(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JSFunction*>) (SelfHosting.cpp:371) ==2306== by 0x47A47F: JSFunction::initializeLazyScript(JSContext*) (jsfun.cpp:1055) ==2306== by 0x4A278A: TypeConstraintPropagateThis::newType(JSContext*, js::types::TypeSet*, js::types::Type) (jsfun.h:205) ==2306== by 0x4914EB: js::types::TypeSet::addType(JSContext*, js::types::Type) (jsinferinlines.h:1150) ==2306== by 0x495D25: js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) (jsinfer.cpp:5637) ==2306== by 0x6F21C1: js::ion::ReflowTypeInfo(unsigned int) (jsinferinlines.h:931) ==2306== Address 0xfffffffffffffffc is not stack'd, malloc'd or (recently) free'd The crash seems to be near null, but using a negative offset. Marking S-s so this can be investigated first.
The first bad revision is: changeset: 119958:b0f81f058496 user: Jan de Mooij date: Sat Jan 26 16:40:06 2013 +0100 summary: Bug 825379 - Fix JSContext::findVersion to work with Ion frames. r=dvander
Jan, could you take a look at this? I'm not sure if bug 825379 exposed a pre-existing bug or if this is new. It looks like we're entering the VM from within a post-bailout update (i.e. ReflowTypeInfo), which does not have an exit frame, and therefore we skip the OptimizedJS frame.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 119958:b0f81f058496 user: Jan de Mooij date: Sat Jan 26 16:40:06 2013 +0100 summary: Bug 825379 - Fix JSContext::findVersion to work with Ion frames. r=dvander This iteration took 12.481 seconds to run.
Crash Signature: [@ js::ion::IonFrameIterator::ionScript] [@ js::ion::GetPcScript]
Summary: Assertion failure: type() == IonFrame_OptimizedJS, at ion/IonFrames.cpp:855 Crash [@ js::ion::IonFrameIterator::ionScript] → Crash [@ js::ion::IonFrameIterator::ionScript] or [@ js::ion::GetPcScript] or Assertion failure: type() == IonFrame_OptimizedJS, at ion/IonFrames.cpp:855
The problem is that we bail the first JS frame and we look for the script while it is still marked as running in Ion. I guess a simple fix would be to clear the runningInIon flag from the bailed frame after the bailout instead of doing so in the ThunkToInterpreter function.
Niko: can you confirm that this fix the bug you had on your branch?
Attachment #708726 - Flags: review?(dvander) → review+
Comment on attachment 708726 [details] [diff] [review] Remove the runningInIon flag at the end of convertFrame. [Security approval request comment] How easily could an exploit be constructed based on the patch? Moving the line show that the problem was in-between, and that it is related to stack iteration as the flag is only used during stack iterations. It does not pin-point which function, but this can be found statically by looking at a call graph of functions used in generateBailoutTail. Then I don't know how easy it is to use this to determine which JS function should be used to reproduce a similar bug. This assertion protects against bad frame read. And the information read out of it might cause TI to register some type information at the wrong place. So at-least random memory read, and potentially generating bad TI info. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Nothing except the patch it-self, but this would still be difficult for me. Which older supported branches are affected by this flaw? aurora, beta, 18. not b2g18 (does not use IonMonkey) If not all supported branches, which bug introduced the flaw? Bug 825379 exhibit one show case. In practice it could be present since IonMonkey & Self-hosted landing. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch should be identical on all branches (not tested) How likely is this patch to cause regressions; how much testing does it need? Regressions are unlikely, except in case of OOM, where this might not be well defined.
Attachment #708726 - Flags: sec-approval?
Since we didn't notice the problem until bug 825379 exposed it (made the conditions more likely?) we should be safe enough not trying to ram this into Firefox 19 Beta right at the end.
Comment on attachment 708726 [details] [diff] [review] Remove the runningInIon flag at the end of convertFrame. sec-approval = dveditz
Attachment #708726 - Flags: sec-approval? → sec-approval+
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::ion::IonFrameIterator::ionScript] [@ js::ion::GetPcScript] → [@ js::ion::IonFrameIterator::ionScript] [@ js::ion::GetPcScript]
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 708726 [details] [diff] [review] Remove the runningInIon flag at the end of convertFrame. [Approval Request Comment] Bug caused by (feature/regressing bug #): IonMonkey & self-hosted User impact if declined: Potential random memory reads in self-hosted jitted functions. Testing completed (on m-c, etc.): in m-c since 5h (comment 13). Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: N/A
Attachment #708726 - Flags: approval-mozilla-aurora?
Attachment #708726 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #708726 - Flags: feedback?(nmatsakis) → feedback+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main20+]
You need to log in before you can comment on or make changes to this bug.