978811 - Assertion failure: MapTypeToTraceKind<T>::kind == GetGCThingTraceKind(thing), at gc/Marking.cpp:163

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 4cb766685b73 (run with --fuzzing-safe --ion-eager):


function testcase() {
        var obj = {};
        Object.defineProperty(obj, "prop", { get: testcase });
        try {
            obj.prop <<=  20;
        } catch (e) {
            testcase('' + Float32Array);
        }
}
testcase();

Comment 1

[Christian Holler (:decoder)]

Trace for the assertion:



Program received signal SIGSEGV, Segmentation fault.
0x0000000000505366 in CheckMarkedThing<js::jit::JitCode> (thing=0x7ffff68acc40, trc=0x180e310) at  js/src/gc/Marking.cpp:163
163         JS_ASSERT(MapTypeToTraceKind<T>::kind == GetGCThingTraceKind(thing));
(gdb) bt 32
#0  0x0000000000505366 in CheckMarkedThing<js::jit::JitCode> (thing=0x7ffff68acc40, trc=0x180e310) at  js/src/gc/Marking.cpp:163
#1  MarkInternal<js::jit::JitCode> (trc=0x180e310, thingp=0x7fffffdff040) at  js/src/gc/Marking.cpp:192
#2  0x000000000063067e in MarkJitExitFrame (frame=..., trc=0x180e310) at  js/src/jit/IonFrames.cpp:1062
#3  MarkJitActivation (activations=..., trc=0x180e310) at  js/src/jit/IonFrames.cpp:1159
#4  js::jit::MarkJitActivations (rt=<optimized out>, trc=0x180e310) at  js/src/jit/IonFrames.cpp:1192
#5  0x0000000000514ce6 in js::gc::MarkRuntime (trc=0x180e310, useSavedRoots=<optimized out>) at  js/src/gc/RootMarking.cpp:788
#6  0x000000000082bf28 in BeginMarkPhase (rt=0x180ded0) at  js/src/jsgc.cpp:3094
#7  0x0000000000838e74 in IncrementalCollectSlice (rt=0x180ded0, budget=0, reason=JS::gcreason::TOO_MUCH_MALLOC, gckind=js::GC_NORMAL) at  js/src/jsgc.cpp:4605
#8  0x000000000083b7be in GCCycle (rt=0x180ded0, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::TOO_MUCH_MALLOC) at  js/src/jsgc.cpp:4791
#9  0x000000000083be4b in Collect (rt=0x180ded0, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::TOO_MUCH_MALLOC) at  js/src/jsgc.cpp:4929
#10 0x000000000083cab6 in GCIfNeeded (cx=0x1822790) at  js/src/jsgc.cpp:5074
#11 js::gc::CheckAllocatorState<(js::AllowGC)1> (cx=<optimized out>, kind=js::gc::FINALIZE_STRING) at  js/src/jsgcinlines.h:440
#12 0x00000000008b4645 in AllocateNonObject<JSString, (js::AllowGC)1> (cx=0x1822790) at  js/src/jsgcinlines.h:517
#13 js_NewGCString<(js::AllowGC)1> (cx=<optimized out>) at  js/src/jsgcinlines.h:595
#14 0x00000000008b47d4 in new_<(js::AllowGC)1> (length=12298, 
    chars=0x3327d40 u"[e-mail]:5:13\[e-mail]:7:13\[e-mail]:7:13\[e-mail]:5:13\[e-mail]:5:13\[e-mail]:7:13\[e-mail]:7:13\[e-mail]:7:13\[e-mail]:7:13\ntestcase@mi"..., cx=0x1822790) at  js/src/vm/String-inl.h:200
#15 js_NewString<(js::AllowGC)1> (cx=0x1822790, 
    chars=0x3327d40 u"[e-mail]:5:13\[e-mail]:7:13\[e-mail]:7:13\[e-mail]:5:13\[e-mail]:5:13\[e-mail]:7:13\[e-mail]:7:13\[e-mail]:7:13\[e-mail]:7:13\ntestcase@mi"..., length=12298) at  js/src/jsstr.cpp:3969
#16 0x00000000009b7841 in js::StringBuffer::finishString (this=<optimized out>) at  js/src/vm/StringBuffer.cpp:63
#17 0x00000000007a920d in js::ComputeStackString (cx=0x1822790) at  js/src/jsexn.cpp:263
#18 0x00000000007aa126 in js_ErrorToException (userRef=<optimized out>, callback=<optimized out>, reportp=0x7fffffdfe780, message=0x2137040 "too much recursion", cx=0x1822790)
    at  js/src/jsexn.cpp:679
#19 ReportError (cx=0x1822790, message=0x2137040 "too much recursion", reportp=0x7fffffdfe780, callback=<optimized out>, userRef=<optimized out>) at  js/src/jscntxt.cpp:307
#20 0x00000000007aaaeb in js_ReportErrorNumberVA (cx=0x1822790, flags=0, callback=0x768960 <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0, errorNumber=26, argumentsType=js::ArgumentsAreASCII, 
    ap=0x7fffffdfe848) at  js/src/jscntxt.cpp:837
#21 0x00000000007ab5ff in JS_ReportErrorNumberVA (cx=<optimized out>, errorCallback=<optimized out>, userRef=<optimized out>, errorNumber=<optimized out>, ap=<optimized out>)
    at  js/src/jsapi.cpp:5547
#22 0x00000000007ab686 in JS_ReportErrorNumber (cx=<optimized out>, errorCallback=<optimized out>, userRef=<optimized out>, errorNumber=<optimized out>) at  js/src/jsapi.cpp:5536
#23 0x000000000065b140 in js::jit::HandleException (rfe=0x7fffffdfefb0) at  js/src/jit/IonFrames.cpp:681



This bug really doesn't reproduce well. You will have to run this a few times to reproduce (maybe 10-20 times), and it can reproduce immediately (after a second), or can take up to 30 seconds.  Marked s-s because it's a GC related assertion.

Comment 2

[Andrew McCreight [:mccr8]]

It looks like maybe we're tracing JIT code here, so I'd guess it could be a JIT problem.

Comment 3

[Andrew McCreight [:mccr8]]

jandem, is there somebody who can investigate this?

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

In HandleException we try to bail out for a try-catch. Then we try to report a stack overflow and this triggers a GC and we end up in MarkJitExitFrame, where we crash because we think we have a real exit frame instead of a fake one.

The problem is that HandleException calls EnsureExitFrame and this can leave us with JitFrame_Entry => JitFrame_Exit frames and isFakeExitFrame returns false in this case instead of true.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> In HandleException we try to bail out for a try-catch. Then we try to report
> a stack overflow and this triggers a GC and we end up in MarkJitExitFrame,
> where we crash because we think we have a real exit frame instead of a fake
> one.
> 
> The problem is that HandleException calls EnsureExitFrame and this can leave
> us with JitFrame_Entry => JitFrame_Exit frames and isFakeExitFrame returns
> false in this case instead of true.

Didn't we had disabled all GC in the bailout path?
Shouldn't we root the values which are listed within the snapshot of the recovered frame?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> Didn't we had disabled all GC in the bailout path?
> Shouldn't we root the values which are listed within the snapshot of the
> recovered frame?

In this case the GC is triggered under js_ReportOverRecursed, we don't have a snapshot in this case. When we have a bailout snapshot, we return without doing anything that can GC.

We could file a separate bug to add AutoAssertNoGC to ExceptionHandlerBailout and other bailout functions..

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> The problem is that HandleException calls EnsureExitFrame and this can leave
> us with JitFrame_Entry => JitFrame_Exit frames and isFakeExitFrame returns
> false in this case instead of true.

Q: Shouldn't we also root the ResumeFromException pointer, it sounds that the "Value exception" which is stored in it would not be marked.  Or forbid GCs in HandleException too?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Q: Shouldn't we also root the ResumeFromException pointer, it sounds that
> the "Value exception" which is stored in it would not be marked.

When we set rfe->exception, we return immediately from HandleExceptionBaseline and HandleException, so it's never live across a GC.

> Or forbid GCs in HandleException too?

We could do that, but HandleException can call debugger hooks etc and enter JS, so we might want to GC there. Not sure if it matters.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8398874 [details] [diff] [review]
Patch

Review of attachment 8398874 [details] [diff] [review]:
-----------------------------------------------------------------

EnsureExitFrame does not create the EntryFrame, it just match if it exists.
Couldn't we identify the HandleException fake exit frame (as we do for DOM / OOL IC calls) (probably in handleFailureWithHandler, or in C++ code), such as we can skip/assert-not-present in MarkJitExitFrame.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8398874 [details] [diff] [review]
Patch

Review of attachment 8398874 [details] [diff] [review]:
-----------------------------------------------------------------

I'll accept the review but we need 3 follow-up:
 - Rename isFakeExitFrame to isBailoutExitFrame (as well as EnsureExitFrame should be renamed EnsureBailoutExitFrame)
 - EnsureBailoutExitFrame should assert that GC are suppressed during bailouts.
 - Assert that all exit frame filtered out by isBailoutExitFrame are produced by EnsureBailoutExitFrame.

I do not think that we are making any VM function call out of the Entry frame for the moment, but I do not want to have another security issue because we did not made the 3rd part of the follow-ups.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Jan, can we get this marked for sec-approval? so we can discuss when to get it checked in?

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8398874 [details] [diff] [review]
Patch

[Security approval request comment]
- How easily could an exploit be constructed based on the patch?
Not easily. Maybe with some targeted fuzzing.

- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

- Which older supported branches are affected by this flaw?
All.

- Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply (one-liner).

- How likely is this patch to cause regressions; how much testing does it need?
Unlikely.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to {N/A until Apr. 27} Nicolas B. Pierron [:nbp] from comment #10)
> I'll accept the review but we need 3 follow-up:
>  - Rename isFakeExitFrame to isBailoutExitFrame (as well as EnsureExitFrame
> should be renamed EnsureBailoutExitFrame)
>  - EnsureBailoutExitFrame should assert that GC are suppressed during
> bailouts.

EnsureExitFrame is not only called for bailouts. It's also called from HandleException when we're not bailing out.

>  - Assert that all exit frame filtered out by isBailoutExitFrame are
> produced by EnsureBailoutExitFrame.

What do you mean by this?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8398874 [details] [diff] [review]
Patch

sec-approval+ for checkin on trunk on May 20 or later (which is a few weeks into the next cycle).

That will be Firefox 32 so we'll want, at that time, 31 and 30 patches as well.

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to {N/A until Apr. 27} Nicolas B. Pierron [:nbp] from comment #10)
> > I'll accept the review but we need 3 follow-up:
> >  - Rename isFakeExitFrame to isBailoutExitFrame (as well as EnsureExitFrame
> > should be renamed EnsureBailoutExitFrame)
> >  - EnsureBailoutExitFrame should assert that GC are suppressed during
> > bailouts.
> 
> EnsureExitFrame is not only called for bailouts. It's also called from
> HandleException when we're not bailing out.

EnsureExitFrame is used to replace the current frame by an exit frame.  This means that all GC things which are on the stack are no longer marked.  This means that we need to copy everything in rooted places before being able to run a GC again.

HandleException is not doing a Bailout in the terms that it reconstruct a stack, but it unwinds the Jit frame.

> >  - Assert that all exit frame filtered out by isBailoutExitFrame are
> > produced by EnsureBailoutExitFrame.
> 
> What do you mean by this?

I mean that we should not skip a frame because we are interpreting it as being unwound.  If we were to do so, this means that we might forget to mark a Value which is part of the last script frame.

I want us to assert that isBailoutExitFrame is only matching frames which are produced EnsureBailoutExitFrame, as we use isBailoutExitFrame (isFakeExitFrame) to skip frames which have to be skipped.

Comment 16

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e280c3740a4a

Please nominate this for Aurora/Beta/ESR24 approval as soon as you get a chance :)

Comment 17

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Backed out for Android/B2G debug asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c610619df3

https://tbpl.mozilla.org/php/getParsedLog.php?id=40012808&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40012396&tree=Mozilla-Inbound

Comment 18

[Andrew McCreight [:mccr8]]

Jandem, can you fix your patch?  Thanks.

Comment 19

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21d0c653afa1

(https://tbpl.mozilla.org/?tree=Try&rev=95f79ca68c19)

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21d0c653afa1

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 8398874 [details] [diff] [review]
Patch

[Approval Request Comment]
User impact if declined: Potential sec bugs, crashes.
Fix Landed on Version: 32, but will likely be backported.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Older Ion bug; unknown.
User impact if declined: Potential sec bugs, crashes.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8398874 [details] [diff] [review]
Patch

Approval granted for ESR24. Please land ASAP.

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8e71359d176
https://hg.mozilla.org/releases/mozilla-beta/rev/ccf94b264d5d
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/0beb220faac2
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7bf17049d531
https://hg.mozilla.org/releases/mozilla-esr24/rev/fc1922e36212

Comment 24

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ccf94b264d5d

Comment 25

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 26

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-release/rev/d7eb4ba73ad7

Comment 27

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx30

Comment 28

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx31
JSBugMon: This bug has been automatically verified fixed on Fx32