Closed Bug 805299 Opened 8 years ago Closed 8 years ago

IonMonkey: Crash [@ 0xdeadbeee]


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed
firefox20 + fixed
firefox-esr10 --- unaffected


(Reporter: decoder, Assigned: mjrosenb)



(Keywords: crash, sec-critical, testcase, Whiteboard: [jsbugmon:ignore][adv-main18-])

Crash Data


(3 files)

Attached file Testcase for shell
The attached testcase crashes on mozilla-central revision 1c3e4cb1f754 (run with --ion-eager).
Crash trace:

Program received signal SIGSEGV, Segmentation fault.
0xdeadbeee in ?? ()
(gdb) bt
#0  0xdeadbeee in ?? ()
#1  0x0047f4e4 in js::ion::AutoFlushCache::~AutoFlushCache (this=0x40030788, __in_chrg=<value optimized out>) at /home/decoder/mozilla-central/js/src/ion/arm/Assembler-arm.cpp:2474
#2  0x00d8d690 in ?? ()
Cannot access memory at address 0x0
#3  0x00d8d690 in ?? ()
Cannot access memory at address 0x0
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
Summary: Crash [@ 0xdeadbeee] → IonMonkey: Crash [@ 0xdeadbeee]
Whiteboard: [jsbugmon:ignore]
Assignee: general → mrosenberg
What's the status here? This is marked tracking# for 18, which is entering beta this week.
A quick patch would be to just abort compilation when we hit this case.  This case should basically never come up.  The issue has to do with pools, and is a bad fix for bug 759464.
I'm currently trying to cook up an actual reasonable way to handle this case.
This patch causes compilation to abort when we detect a case that we were previously handling incorrectly.  There were a few cases where assertions were then triggering due to the world not looking quite right, so explicit checks needed to be added.
Attachment #685092 - Flags: review?(dvander)
Comment on attachment 685092 [details] [diff] [review]

Review of attachment 685092 [details] [diff] [review]:

::: js/src/ion/IonMacroAssembler.h
@@ +499,5 @@
>      }
>      void link(IonCode *code) {
> +        if (m_buffer.oom())
> +            return;

Better to make this JS_ASSERT(!m_buffer.oom()), and then check for OOM where needed in ion::Linker::newCode().

::: js/src/ion/RangeAnalysis.cpp
@@ +532,5 @@
>                      return false;
>              }
>          }
>          iters++;
> +        if (iters >= numBlocks * 1000)

Part of this patch?
Attachment #685092 - Flags: review?(dvander) → review+
Marty, are you planning to land this reviewed patch?
Also, please nominate for beta and aurora approval once it sticks on trunk.
Backed out because this breaks debug builds:
My bad.  tbpl logs point right at the culprit: I directly reference m_buffer in shared code inside of JS_ASSERT, so it will crash only in debug builds on all non-arm platforms.
Marty - can you continue this investigation? This is an sg:crit regression from FF17 and we're going to build with b4 Tuesday. Would be really great to get a fix uplifted before then.
yeah, I'm working on it, and fighting with mochitests, as usual.
landed again:
This time with a try run first.
my script occasionally generates bogus urls.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): IM landing + 763333
User impact if declined: mysterious crashes, possibly incorrect behavior
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): 
This patch is the safe approach. I'm just nuking the code paths that are trick/were questionable, and causing us to not compile the code with IonMonkey.  Sadly, it uses the OOM mechanism, and that isn't quite perfect.  If there are paths where OOM conditions need to be checked, and they aren't we may throw an assertion.
String or UUID changes made by this patch:
Attachment #690055 - Flags: approval-mozilla-beta?
Attachment #690055 - Flags: approval-mozilla-aurora?
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 690055 [details] [diff] [review]
patch that was actually landed on m-i

Low risk enough that we can take this for beta 4 which goes to build tomorrow, please go ahead with branch uplift.
Attachment #690055 - Flags: approval-mozilla-beta?
Attachment #690055 - Flags: approval-mozilla-beta+
Attachment #690055 - Flags: approval-mozilla-aurora?
Attachment #690055 - Flags: approval-mozilla-aurora+
(In reply to Marty Rosenberg [:mjrosenb] from comment #19)
> landed on both aurora and beta

Setting flags accordingly.
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][adv-main18-]
Can this be put in testsuite?
Flags: in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.