Crash [@ js::ion::IonFrameIterator::ionScript] or [@ js::ion::GetPcScript] or Assertion failure: type() == IonFrame_OptimizedJS, at ion/IonFrames.cpp:855

VERIFIED FIXED in Firefox 20

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla21
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 wontfix, firefox19- affected, firefox20+ fixed, firefox21+ fixed, firefox-esr17 unaffected, b2g18 disabled)

Details

(Whiteboard: [jsbugmon:update][adv-main20+], crash signature)

Attachments

(1 attachment)

Reporter

Description

7 years ago
The following testcase asserts on mozilla-central revision 80fed51ae074 (run with --ion-eager):


new Function("\
actual = Array.indexOf([]);\
actual = [].indexOf();\
actual += Array.indexOf(actual);\
")();
Reporter

Comment 1

7 years ago
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.
Whiteboard: [jsbugmon:update,bisect]

Comment 2

7 years ago
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
jandem, is bug 825379 possibly related?
Blocks: 825379
Flags: needinfo?(jdemooij)
Keywords: regression
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
Flags: needinfo?(jdemooij)
Assignee

Updated

7 years ago
Duplicate of this bug: 835792
Assignee

Updated

7 years ago
Assignee: jdemooij → nicolas.b.pierron
Reporter

Updated

7 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter

Comment 6

7 years ago
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
Assignee

Comment 7

7 years ago
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.
Assignee

Comment 8

7 years ago
Niko: can you confirm that this fix the bug you had on your branch?
Attachment #708726 - Flags: review?(dvander)
Attachment #708726 - Flags: feedback?(nmatsakis)
Attachment #708726 - Flags: review?(dvander) → review+
Assignee

Comment 9

7 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/979de88d617d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Reporter

Updated

6 years ago
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::ion::IonFrameIterator::ionScript] [@ js::ion::GetPcScript] → [@ js::ion::IonFrameIterator::ionScript] [@ js::ion::GetPcScript]
Reporter

Comment 14

6 years ago
JSBugMon: This bug has been automatically verified fixed.
Assignee

Comment 15

6 years ago
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+
Assignee

Comment 16

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/741f0584eb8a
Crash Signature: [@ js::ion::IonFrameIterator::ionScript] [@ js::ion::GetPcScript] → [@ js::ion::IonFrameIterator::ionScript] [@ js::ion::GetPcScript]
Attachment #708726 - Flags: feedback?(nmatsakis) → feedback+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.