Closed Bug 858940 Opened 7 years ago Closed 7 years ago

BaselineCompiler: Assertion failure: false (unsupported relocation), at arm/Assembler-arm.cpp:688 or Crash on Heap with invalid jumps

Categories

(Core :: JavaScript Engine, defect, critical)

ARM
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
firefox25 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: mjrosenb)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [jsbugmon:ignore][fuzzblocker][adv-main23-])

Attachments

(1 file)

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


new TestCase( SECTION, "", "r", eval("x = new Boolean(true); x.charAt=String.prototype.charAt;x.charAt(1)"));
new TestCase( SECTION, "", "u", eval("x = new Boolean(true); x.charAt=String.prototype.charAt;x.charAt(2)"));
new TestCase( SECTION, "", "e", eval("x = new Boolean(true); x.charAt=String.prototype.charAt;x.charAt(3)"));
new TestCase( SECTION, "", "", eval("x = new Boolean(true); x.charAt=String.prototype.charAt;x.charAt(4)"));
new TestCase( SECTION, "", "", eval("x = new Boolean(true); x.charAt=String.prototype.charAt;x.charAt(-1)") );
new TestCase( SECTION, "","", "r", eval("x = new Boolean(true); x.charAt=String.prototype.charAt;x.charAt(true)") );
new TestCase( SECTION, "", "t", eval("x = new Boolean(true); x.charAt=String.prototype.charAt;x.charAt(false)") );
new TestCase( SECTION, "", "", eval("x=new String();x.charAt(0)") );
new TestCase( SECTION, "", "", eval("x=new String();x.charAt(1)") );
new TestCase( new (function (SECTION = this) {}), "", "", eval("x=new String();x.charAt(-1)") );
new TestCase( SECTION, "", "", eval("x=new String();x.charAt(Number.NaN)") );
new TestCase( SECTION, "", "", eval("x=new String();x.charAt(Number.POSITIVE_INFINITY)") );
new TestCase( SECTION, "", "", eval("x=new String();x.charAt(Number.NEGATIVE_INFINITY)") );
new TestCase( SECTION, "", "1", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(0)") );
new TestCase( SECTION, "", "2", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(1)") );
new TestCase( SECTION,  "", "3", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(2)") );
new TestCase( SECTION, "", "4", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(3)") );
new TestCase( SECTION, "", "5", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(4)") );
new TestCase( SECTION, "", "6", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(5)") );
new TestCase( SECTION, "", "7", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(6)") );
new TestCase( SECTION, "", "8", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(7)") );
new TestCase( SECTION, "", "9", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(8)") );
new TestCase( SECTION, "", "0", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(9)") );
new TestCase( SECTION, "", "", "0x3" );
new TestCase( SECTION, "", "4", eval("var MYOB = new MyObject(1234567890); MYOB.charAt(Math.PI)") );
new TestCase( SECTION, "", "b", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(2)") );
new TestCase( SECTION, "", "j", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(3)") );
new TestCase( SECTION, "", "e", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(4)") );
new TestCase( SECTION, "", "c", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(5)") );
new TestCase( SECTION, "", "t", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(6)") );
new TestCase( SECTION, "", " ", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(7)") );
new TestCase( SECTION, "", "O", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(8)") );
new TestCase( SECTION, "", "b", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(9)") );
new TestCase( SECTION, "", eval("var MYOB = new MyOtherObject(1234567890); MYOB.charAt(10)") );
function MyObject( value ) {}
function MyOtherObject(value) {}
The test doesn't crash with --no-baseline so I assume it's a baseline-related thing. This test does not crash in an opt-build, however I got other tests that crash opt builds with various traces and invalid jumps to bogus addresses. All of these tests assert with this message in debug builds so I assume it's the same issue. Therefore, marking s-s and sec-critical.

This is also a fuzzblocker because it triggers quite often and produces all sorts of traces.
Blocks: BaselineFuzz
Keywords: crash, sec-critical
Whiteboard: [jsbugmon:ignore][fuzzblocker]
Marty, can you take a look? :)
Flags: needinfo?(mrosenberg)
ok, after trying for a while to reproduce this, I believe that it only reproduces under qemu.  I'll try to look into exactly why it is only triggered on qemu, but this likely means it isn't actually a security concern.
Flags: needinfo?(mrosenberg)
I wouldn't rely on the fact that it only reproduces under qemu so far, until we've actually found out the reason for the problem, because the test is highly fragile. Even the slightest change makes it not reproduce anymore and if qemu and real hardware are only slightly different, this could already lead to the non-reproducing test.
ok, I've found the bug and have a patch that fixes this particular crash.
Unfortunately, it exposes a bug or two in some interactions between BC and the arm backend.
Assignee: general → mrosenberg
Marty any updates on this bug? Is there value taking your patch for this bug and spinning new bugs for the secondary issues?
Flags: needinfo?(mrosenberg)
I suspect this is the same as https://bugzilla.mozilla.org/show_bug.cgi?id=871290
My original solution was to just actually fix the cause, and make BC record the correct offset for all instructions, including when pools are forcibly placed.  When I made that change, BC started throwing assertions all over the place because the offset that it took before it generated an instruction didn't match the offset of the first instruction (because there was a pool in the way).  I decided to just let this sit until I'd finished re-writing the assembler buffer to not have this flaw, but somehow or other stumbled upon the idea to just *ignore* the pools when we go to read any data that was there.
Flags: needinfo?(mrosenberg)
Did the work landed on June 14 over in bug 871290 fix this one?
whoops, my face is red on this one.  I fixed the issue in the constructor for the instruction iterator, but I forgot to verify that all code-inspection paths actually went through the instruction iterator.  as a godawful shim, just quickly route through the instruction iterator, which knows how to handle this case.
Attachment #774277 - Flags: review?
Attachment #774277 - Flags: review? → review?(Jacob.Bramley)
Comment on attachment 774277 [details] [diff] [review]
/home/mjrosenb/patches/nonIterPath-r0.patch

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

I don't think I'm really qualified to review this one. The code looks sane and matches your description, but I can't vouch for its completeness, or if it is the correct fix.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +342,5 @@
>  {
>      int32_t imm = imm_.value;
> +    if (i) {
> +        // If the access goes through an iterator (which this doesn't) then
> +        // all issues pertaining to reading guard instructions.

The comment doesn't make a lot of sense.
Attachment #774277 - Flags: review?(Jacob.Bramley) → review+
landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/302fb81f0729
I'll try to re-write the comment to be more sensible after we confirm this actually fixes the problem.
https://hg.mozilla.org/mozilla-central/rev/302fb81f0729
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Backed out for frequent crashes in the Android test suite (bug 893376, bug 894251). My spidey sense is telling me that if we crash this easily there, we'll probably have a topcrash on our hands too once this starts shipping in a nightly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/da9f2ffcfc4d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla25 → ---
Flags: needinfo?(mrosenberg)
Is there a reason that this didn't get sec-approval since baseline compiler is in 23 and higher?
Comment on attachment 774277 [details] [diff] [review]
/home/mjrosenb/patches/nonIterPath-r0.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Landing IM
User impact if declined: Crashes
Testing completed (on m-c, etc.): On m-i
Risk to taking this patch (and alternatives if risky): Not risky at all.
String or IDL/UUID changes made by this patch:
Attachment #774277 - Flags: approval-mozilla-beta?
Attachment #774277 - Flags: approval-mozilla-aurora?
Comment on attachment 774277 [details] [diff] [review]
/home/mjrosenb/patches/nonIterPath-r0.patch

Let's get this uplifted asap for our final beta.
Attachment #774277 - Flags: approval-mozilla-beta?
Attachment #774277 - Flags: approval-mozilla-beta+
Attachment #774277 - Flags: approval-mozilla-aurora?
Attachment #774277 - Flags: approval-mozilla-aurora+
I'm not sure if I'm reading this correctly, but if a fix is not landed on central (the original one was backed out in comment 13 / comment 14, I think), should this still be landed on branches?
Comment on attachment 774277 [details] [diff] [review]
/home/mjrosenb/patches/nonIterPath-r0.patch

You're right, comment 14 was misleading and it looked landed.  Not approving for uplift in that case.
Attachment #774277 - Flags: approval-mozilla-beta-
Attachment #774277 - Flags: approval-mozilla-beta+
Attachment #774277 - Flags: approval-mozilla-aurora-
Attachment #774277 - Flags: approval-mozilla-aurora+
So given that, we'll have to wontfix here for FF23 since we're going to build our final beta within the hour.
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #18)
> I'm not sure if I'm reading this correctly, but if a fix is not landed on
> central (the original one was backed out in comment 13 / comment 14, I
> think), should this still be landed on branches?

(In reply to lsblakk@mozilla.com [:lsblakk] from comment #19)
> Comment on attachment 774277 [details] [diff] [review]
> /home/mjrosenb/patches/nonIterPath-r0.patch
> 
> You're right, comment 14 was misleading and it looked landed.  Not approving
> for uplift in that case.

It was on inbound. He didn't post the updated cset link.

https://hg.mozilla.org/mozilla-central/rev/b09cbe63a886
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(mrosenberg) → in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 774277 [details] [diff] [review]
/home/mjrosenb/patches/nonIterPath-r0.patch

Noted, let's get this uplifted please.
Attachment #774277 - Flags: approval-mozilla-beta-
Attachment #774277 - Flags: approval-mozilla-beta+
Attachment #774277 - Flags: approval-mozilla-aurora-
Attachment #774277 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:ignore][fuzzblocker] → [jsbugmon:ignore][fuzzblocker][adv-main23-]
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Group: core-security
You need to log in before you can comment on or make changes to this bug.