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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


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

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

[Jesse Ruderman]

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

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

jandem, is bug 825379 possibly related?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 6

[Christian Holler (:decoder)]

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.

Comment 7

[Nicolas B. Pierron [:nbp — off until 29-09]]

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.

Comment 8

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: Remove the runningInIon flag at the end of convertFrame.

Niko: can you confirm that this fix the bug you had on your branch?

Comment 9

[Nicolas B. Pierron [:nbp — off until 29-09]]

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.

Comment 10

[Daniel Veditz [:dveditz]]

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 11

[Daniel Veditz [:dveditz]]

Comment on attachment 708726 [details] [diff] [review]
Remove the runningInIon flag at the end of convertFrame.

sec-approval = dveditz

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/979de88d617d

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/979de88d617d

Comment 14

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 15

[Nicolas B. Pierron [:nbp — off until 29-09]]

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

Comment 16

[Nicolas B. Pierron [:nbp — off until 29-09]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/741f0584eb8a