Closed Bug 1070962 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 + wontfix
firefox35 + verified
firefox36 --- verified
firefox-esr31 --- unaffected

People

(Reporter: decoder, Assigned: nbp)

References

Details

(Keywords: crash, sec-low, testcase, Whiteboard: [jsbugmon:update][adv-main35+])

Crash Data

Attachments

(8 files, 2 obsolete files)

463 bytes, text/plain
Details
8.36 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.88 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.48 KB, patch
jandem
: review+
Details | Diff | Splinter Review
20.76 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.66 KB, patch
jandem
: review+
Details | Diff | Splinter Review
38.13 KB, patch
jandem
: review+
Details | Diff | Splinter Review
27.61 KB, patch
jandem
: review+
Details | Diff | Splinter Review
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();
}
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.
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(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.
Flags: needinfo?(nicolas.b.pierron)
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?
Flags: needinfo?(jdemooij)
What's the exception we throw? Is it OK to move this to the bailout path? Or is it a stack overflow?
(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.
(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?
Flags: needinfo?(jdemooij)
(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.
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.
Keywords: sec-low
Attachment #8502561 - Flags: review?(nicolas.b.pierron)
Attachment #8502561 - Flags: feedback?(shu)
Attachment #8502564 - Flags: review?(jdemooij)
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.
Flags: needinfo?(nicolas.b.pierron)
Attachment #8502561 - Flags: review?(nicolas.b.pierron) → review?(jdemooij)
Attachment #8502563 - Attachment is obsolete: true
Attachment #8502563 - Flags: review?(jdemooij)
Attachment #8502617 - Flags: review?(jdemooij)
Attachment #8502564 - Attachment is obsolete: true
Attachment #8502564 - Flags: review?(jdemooij)
Attachment #8502618 - Flags: review?(jdemooij)
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/
Attachment #8502557 - Flags: review?(jdemooij) → review+
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.)
Attachment #8502558 - Flags: review?(jdemooij) → review+
Attachment #8502559 - Flags: review?(jdemooij) → review+
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)
Attachment #8502560 - Flags: review?(jdemooij) → review+
Attachment #8502561 - Flags: review?(jdemooij) → review+
Attachment #8502617 - Flags: review?(jdemooij) → review+
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
Attachment #8502618 - Flags: review?(jdemooij) → review+
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 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.
Attachment #8502560 - Flags: feedback?(shu)
Attachment #8502561 - Flags: feedback?(shu)
Flags: needinfo?(nicolas.b.pierron)
[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.
Blocks: 1083781
Should I make a request for every patch, or what I mentioned in comment 30 is enough?
Flags: needinfo?(nicolas.b.pierron) → needinfo?(abillings)
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.
Flags: needinfo?(abillings)
(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.
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.
Tracking - please do a nomination approval on at least one of the attachments for branch uplift(s).
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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
Attachment #8502618 - Flags: approval-mozilla-aurora?
Attachment #8502618 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(abillings)
We could live without it on Beta and let it ride the trains.
Flags: needinfo?(abillings)
JSBugMon: This bug has been automatically verified fixed on Fx35
(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
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main35+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.