1070962 - Crash [@ js::jit::JitFrameIterator::operator++] with RegExp

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 5bd6e09f074e (run with --no-threads --fuzzing-safe):


test();
function test() {
  function v() {
    try { 
	test(/x{2147483647,2147483648}x/.test('1'), false); 
    } catch (actual) {}
  }
  v();
}

Comment 1

[Christian Holler (:decoder)]

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
js::jit::JitFrameIterator::operator++ (this=0x7fffffdfd648) at js/src/jit/IonFrames.cpp:312
312         frameSize_ = prevFrameLocalSize();
#0  js::jit::JitFrameIterator::operator++ (this=0x7fffffdfd648) at js/src/jit/IonFrames.cpp:312
#1  0x0000000000a5f561 in js::FrameIter::settleOnActivation (this=0x7fffffdfd5f0) at js/src/vm/Stack.cpp:568
#2  0x0000000000a5fa2e in js::FrameIter::FrameIter (this=0x7fffffdfd5f0, cx=<optimized out>, savedOption=<optimized out>) at js/src/vm/Stack.cpp:658
#3  0x000000000083d86f in NonBuiltinFrameIter (cx=<optimized out>, this=0x7fffffdfd5f0, opt=<optimized out>) at js/src/vm/Stack.h:1759
#4  PopulateReportBlame (cx=<optimized out>, report=0x7fffffdfda80) at js/src/jscntxt.cpp:326
#5  0x0000000000882b2f in js_ReportErrorNumberVA (cx=0x19861c0, flags=0, callback=0x83b0f0 <js_GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=104, argumentsType=js::ArgumentsAreASCII, ap=0x7fffffdfdb48) at js/src/jscntxt.cpp:814
#6  0x0000000000883a8f in JS_ReportErrorNumberVA (cx=<optimized out>, errorCallback=<optimized out>, userRef=<optimized out>, errorNumber=<optimized out>, ap=<optimized out>) at js/src/jsapi.cpp:5702
#7  0x0000000000883b16 in JS_ReportErrorNumber (cx=<optimized out>, errorCallback=<optimized out>, userRef=<optimized out>, errorNumber=<optimized out>) at js/src/jsapi.cpp:5691
r13     0xba1   2977
=> 0x6cc3b4 <js::jit::JitFrameIterator::operator++()+36>:       mov    0x8(%r13),%r12


Marking s-s because this doesn't look like a null-deref although the pointer (0xba1) is below 0x1000.

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Christian Holler (:decoder) from comment #1)
> Marking s-s because this doesn't look like a null-deref although the pointer
> (0xba1) is below 0x1000.

\o/ Victory! at least it does not seems to be exploitable.

This address is a magic number made to cause crashes while indicating that we are making an attempt to mark the stack during a bailout.

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Bailouts.h#105

I will investigate.

Comment 4

[Nicolas B. Pierron [:nbp]]

In a bailout, we try to report an error during the execution of RRegExpTest::recover.  The function PopulateReportBlame is called to fill the fields of the report.  In order to fill the fields, it creates a NonBuiltinFrameIter.

The NonBuiltinFrameIter is a FrameIter, which create a JitFrameIterator with the FAKE_ION_TOP.  This cause the crash that we see here.

One solution would be to move the FAKE_ION_TOP after the recovery of instruction and instrument the JitActivation to create an IonBailoutIterator instead of a JitFrameIterator when the top frame is bailing.  This solution will solve the SEGV issue.

Still, this solution is incorrect as RRegExpTest::recover is not executed under the bailout, which means that any stack reported for the bailing frame would refer to current top-level function, but not to the top-level of the original regexp.test call.  This problem also exists for all other recover instructions which are doing a JS_ReportError.

Making the JS_Report report the stack frame in which the instruction was supposed to be might be more challenging.  This implies keeping additional frame context for each recover instruction (even keeping references to removed frames).

Personally, I am thinking that we should just fix the SEGV and report an incorrect script name, as the alternative sounds way too complex for the benefit of getting the correct location in the error message.

Jan, which approach would you suggest?

Comment 5

[Jan de Mooij [:jandem]]

What's the exception we throw? Is it OK to move this to the bailout path? Or is it a stack overflow?

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> What's the exception we throw? Is it OK to move this to the bailout path? Or
> is it a stack overflow?

the first one is a stack overflow, the second one might be the same.
This is already on the bailout path.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Personally, I am thinking that we should just fix the SEGV and report an
> incorrect script name, as the alternative sounds way too complex for the
> benefit of getting the correct location in the error message.

Yeah, this makes sense. But I also wonder if we should really use recover instructions for code that can throw exceptions like this..

Will fixing the iteration code also make the object metadata callback work, so that we can remove the suppression code for that?

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > Personally, I am thinking that we should just fix the SEGV and report an
> > incorrect script name, as the alternative sounds way too complex for the
> > benefit of getting the correct location in the error message.
> 
> Yeah, this makes sense. But I also wonder if we should really use recover
> instructions for code that can throw exceptions like this..

I think we should, because this is needed for optimizations such as escape analysis.  So if we want to remove the allocations, we have to use recover instructions to do the allocations.

> Will fixing the iteration code also make the object metadata callback work,
> so that we can remove the suppression code for that?

Depends what you mean by fixing and by work.  If by work you mean that we would have a stack, then yes, but it is unlikely to be correct unless we attach a stack/location for each recover instruction.

Comment 9

[Andrew McCreight [:mccr8]]

This sounds like a safe crash, so I'm going to mark this as low.  Please clear the flag or let me know if this is wrong.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Register IonBailoutIterator on the JitActivation.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Remove GetTopIonJSScript overloads.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Ensure consistent construction of JitFrameIterator.

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - JitFrameIterator use BailoutData when it starts on a bailout frame.

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: part 5 - rematerializeFrames no longer use IonBailoutIterator.

Comment 15

[Nicolas B. Pierron [:nbp]]

Attachment: part 6 - IonBailoutIterator no longer inherit from JitFrameIterator.

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: part 7 - Rename IonBailoutIterator to BailoutFrameInfo.

Comment 17

[Nicolas B. Pierron [:nbp]]

These patches are changing the way we create JitFrameIterators, such as they are always created with a JitActivation.  The JitActivation is modified to hold a BailoutData pointer (previously known as IonBailoutIterator).  This pointer is registered by the BailoutData when it is created and removed when we exit the function.

When a JitFrameIterator is created, it look on the activation if we are under a bailout, if this is the case, then the constructor replace the JitFrame_Exit by a JitFrame_Bailout which corresponds to a scripted frame.  In some functions, when we are on a JitFrame_Bailout, then we are reading the infos from the bailout data registered on the JitActivation.

This implies that any JitFrameIterator created under a bailout can now safely iterate the inline frames of the JitActivation, which solve this issue of iterating the stack for creating exceptions.  Still it does not solve the issue of having the stack which correspond to the recover instruction original location.

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: part 6 - IonBailoutIterator no longer inherit from JitFrameIterator.

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: part 7 - Rename IonBailoutIterator to BailoutFrameInfo.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8502557 [details] [diff] [review]
part 1 - Register IonBailoutIterator on the JitActivation.

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

::: js/src/vm/Stack.cpp
@@ +1440,5 @@
>      MOZ_ASSERT(ionRecovery_.empty());
> +
> +    // The Ion Bailout Iterator should have unregistered it-self from the
> +    // JitActivations.
> +    MOZ_ASSERT(!ionBailoutIterator_);

Nit: s/it-self/itself, JitActivation.

::: js/src/vm/Stack.h
@@ +1328,5 @@
>      typedef Vector<RInstructionResults, 1> IonRecoveryMap;
>      IonRecoveryMap ionRecovery_;
>  
> +    // If we are bailing out from Ion, then this field should be a non-null
> +    // pointer which reference the IonBailoutIterator used to walk the inner

Nit: s/reference/references/

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 8502558 [details] [diff] [review]
part 2 - Remove GetTopIonJSScript overloads.

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

Nice. (We should rename it to GetTopJitJSScript but we can do that later in a public bug.)

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8502560 [details] [diff] [review]
part 4 - JitFrameIterator use BailoutData when it starts on a bailout frame.

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

::: js/src/jit/JitFrameIterator.h
@@ +59,5 @@
>      // JitActivation.
> +    JitFrame_Exit,
> +
> +    // A bailout frame is a special IonJS jit frame after a bailout, and before
> +    // the reconstruction of the BaselineJS frame.  From within C++, a bailout

Nice: one space after '.'

@@ +60,5 @@
> +    JitFrame_Exit,
> +
> +    // A bailout frame is a special IonJS jit frame after a bailout, and before
> +    // the reconstruction of the BaselineJS frame.  From within C++, a bailout
> +    // frame is always the last frame in a JitActivtion iff the bailout frame

Nit: JitActivation typo

::: js/src/vm/Stack-inl.h
@@ +354,5 @@
> +        } else if (data_.jitFrames_.isBailoutJS()) {
> +            // :TODO: (Bug 1070962) If we are introspecting the frame which is
> +            // being bailed, then we might be in the middle of recovering
> +            // instructions.  Stacking computeInstructionResults implies that we
> +            // might be recovering result twice.  In the mean time, to avoid

Nit: one space after '.' (twice)

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 8502618 [details] [diff] [review]
part 7 - Rename IonBailoutIterator to BailoutFrameInfo.

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

Thanks for cleaning this up, much nicer!

::: js/src/jit/Bailouts.h
@@ +113,5 @@
>  
>  // Must be implemented by each architecture.
>  
> +// This structure is constructed before recovering the baseline frames of a
> +// bailout.  It records all information extracted from the stack, and which are

Nit: "for a bailout"? Single space after '.'

@@ +114,5 @@
>  // Must be implemented by each architecture.
>  
> +// This structure is constructed before recovering the baseline frames of a
> +// bailout.  It records all information extracted from the stack, and which are
> +// needed for for the JitFrameIterator.

Nit: s/for for/for/

::: js/src/vm/Stack.h
@@ +1331,5 @@
>      IonRecoveryMap ionRecovery_;
>  
>      // If we are bailing out from Ion, then this field should be a non-null
> +    // pointer which reference the BailoutFrameInfo used to walk the inner
> +    // frames.  This field is used for all newly constructed JitFrameIterator to

Nit: JitFrameIterators

Comment 24

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8b34a91096
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4dd598af9ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aac2b77e38f
https://hg.mozilla.org/integration/mozilla-inbound/rev/191e57386752
https://hg.mozilla.org/integration/mozilla-inbound/rev/8876a417bd4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e00ce29c8c67

Comment 25

[Nicolas B. Pierron [:nbp]]

Hum … I was wondering why I only pushed 6 patches instead of 7, but the last patch (part 7) got merged into the part 6 by accident.  This is not a big deal.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All 6 backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4092b50ccb for B2G build bustage: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2960453&repo=mozilla-inbound

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Ignore that, the bustage was from a different push.
Relanded:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3aa08cca9a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/82d31104b072
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/553feb23aa52
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/67f89c977264
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe372c4fe4e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bd068c8342fc

Comment 28

[Shu-yu Guo [:shu]]

Comment on attachment 8502560 [details] [diff] [review]
part 4 - JitFrameIterator use BailoutData when it starts on a bailout frame.

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

Looks like you pushed it already.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aa3aa08cca9a	
https://hg.mozilla.org/mozilla-central/rev/82d31104b072	
https://hg.mozilla.org/mozilla-central/rev/553feb23aa52	
https://hg.mozilla.org/mozilla-central/rev/67f89c977264	
https://hg.mozilla.org/mozilla-central/rev/3fe372c4fe4e	
https://hg.mozilla.org/mozilla-central/rev/bd068c8342fc
This did not make the uplift to Aurora 35, so you'll need to land there separately

Comment 30

[Nicolas B. Pierron [:nbp]]

[Tracking Requested - why for this release]:

(In reply to Wes Kocher (:KWierso) from comment #29)
> This did not make the uplift to Aurora 35, so you'll need to land there
> separately

Hopefully, the current set of patches does not bring much light on the source of the issue except that this is in relation with the stack iteration during bailouts, and *the crash is not exploitable* (comment 3).

Sadly I only noticed that RRegExpTest/RRegExpExec (Bug 1038592, Bug 1038593) landed on Gecko 34 and not Gecko 35 as I mistakenly thought (because of RRegExpReplace, Bug 1050649).  In a similar way, the bug exists since Bug 1005532, which can OOM during the recovery of instructions.  The problem is that PopulateReportBlame use a stack iterator under a bailout.

Even if a similar issue exists in Gecko 32, I do not see any report from crash-stat of errors related to the crash address 0xba1 (Bug 1015145) with the same stack trace as comment 1.

I can make a tiny patch to disable RegExp recover instructions, but the problem is that if I do the same for MNewObject then this would shine the light on MNewDerivedTypedObject which should not be removed as it fixed an assertion issue (Bug 1004527).

Knowing that this issue is not exploitable and that this is a low crash volume, I suggest that we backport the current set of patches to Gecko 35 (just because it was one day late for the merge day and the bug number suggest it was here in Gecko 35), and let the rest ride the train.

Comment 31

[Nicolas B. Pierron [:nbp]]

Should I make a request for every patch, or what I mentioned in comment 30 is enough?

Comment 32

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

We just need the flags set somewhere so we can do explicit approvals for branches. If you have to make new patches for each, setting it on the patch is best.

Comment 33

[Nicolas B. Pierron [:nbp]]

(In reply to Al Billings [:abillings] from comment #32)
> We just need the flags set somewhere so we can do explicit approvals for
> branches. If you have to make new patches for each, setting it on the patch
> is best.

I do not have to make new patches, these one should apply cleanly, as the just got in after the merge.
I thought the tracking flag was enough.

Comment 34

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

I did not create the process, I just work within it.

I believe release management wants the template questions answered when you ask for approval on a patch. If all of the patches are going in, I'd think you could just use the largest part and set flags on that instead of marking it on seven parts. Tracking+ just means it will be approved unless there are issues.

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking - please do a nomination approval on at least one of the attachments for branch uplift(s).

Comment 36

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 37

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8502618 [details] [diff] [review]
part 7 - Rename IonBailoutIterator to BailoutFrameInfo.

Approval Request Comment
[Feature/regressing bug #]: Bug 1038592, Bug 1038593, … and any other since Bug 1005532

[User impact if declined]: Crash (not exploitable), low volume on 32 so far.

[Describe test coverage new/current, TBPL]: Since 2014-10-13.

[Risks and why]: This is a big chunk of code, but it is mostly cleaning up things such as I can sleep during nights.  Seriously, doing the same patches without these clean-up sounded way more risky, and harder to understand.  And these patches landed right after the merge to aurora :(

[String/UUID change made/needed]: N/A

Comment 38

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

Firefox 34 is marked as affected but given that this is sec-low, does it need to be uplifted to Beta? If so, is there a way to limit the size of the change that is uplifted for Beta?

Comment 39

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

We could live without it on Beta and let it ride the trains.

Comment 40

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

OK. Marking 34 as won't fix.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ccdd0dbcd0fc
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca143046f0c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/d6936b07c96e
https://hg.mozilla.org/releases/mozilla-aurora/rev/59bf3da9cb79
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c3df43383cb
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ec4746b298c

Comment 42

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx35

Comment 43

[Nicolas B. Pierron [:nbp]]

(In reply to Lawrence Mandel [:lmandel] from comment #38)
> Firefox 34 is marked as affected but given that this is sec-low, does it
> need to be uplifted to Beta? If so, is there a way to limit the size of the
> change that is uplifted for Beta?

I could make a tiny patch to disable the feature which are causing these crashes, but this would give hints on how to cause such crashes.  So, knowing that this is a low volume on released version so far, I prefer if we can let it ride the train without giving hints.

The tiny patch consist of returning false from canRecoverOnBailout [1] functions, for any instruction which might report an error during the execution of the recover instruction, except for MNewDerivedTypedObject (Bug 1004527).

[1] http://dxr.mozilla.org/mozilla-central/search?q=canRecoverOnBailout+path%3Ajs%2Fsrc%2Fjit%2FMIR.h&case=true