1012694 - Assertion failure: str1->length() == str2->length(), at jit/IonCaches.cpp:2955

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase asserts on mozilla-central revision 41a54c8add09 (run with --fuzzing-safe --ion-eager).

Comment 1

[Christian Holler (:decoder)]

Another fragile ARM issue, I attached it because the test is whitespace-sensitive. The part with the comment depends on the size of the comment, making it shorter makes the test no longer reproduce. Marked s-s because it's unclear if there is some corruption behind this.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/85e85da78909
user:        Jan de Mooij
date:        Fri May 16 12:26:21 2014 +0200
summary:     Bug 1008590 - Don't store chars pointer for inline strings, store JSString length and flags separately. r=luke

This iteration took 482.207 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from Jan based on comment 2.

Comment 4

[Jan de Mooij [:jandem]]

I'm pretty sure this is another assembler buffer issue, it's possible bug 1008590 exposed it. Leaving the needinfo for mjrosenb.

Comment 5

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> I'm pretty sure this is another assembler buffer issue, it's possible bug
> 1008590 exposed it. Leaving the needinfo for mjrosenb.

fwiw this still reproduces on a significantly simplified asm buffer, and there is bad code that appears to be produced in this path:

MacroAssembler-arm.h:
void branch32(Condition cond, const Address &lhs, Imm32 rhs, Label *label) {
        load32(lhs, ScratchRegister);
        branch32(cond, ScratchRegister, rhs, label);
    }

The ScratchRegister is not available here, unless the rhs is a small immediate, because the ScratchRegister is used internally to load the rhs.

Comment 6

[Douglas Crosher [:dougc]]

And the large immediate rhs is coming from here:

IonCaches.cpp:
bool
GetElementIC::attachGetProp(JSContext *cx, HandleScript outerScript, IonScript *ion,
                            HandleObject obj, const Value &idval, HandlePropertyName name,
                            void *returnAddr)
{
...
    // Check the length.
    masm.branch32(Assembler::NotEqual, Address(scratch, JSString::offsetOfLength()),
                  Imm32(name->length()), &failures);
...
}

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Thanks for tracking this down Douglas! This patch fixes it by using secondScratchReg_.

It's a pre-existing issue, but I don't know if other callers also pass in large immediates. Will try to find out.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

There's another function that needs the same fix.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> It's a pre-existing issue, but I don't know if other callers also pass in
> large immediates. Will try to find out.

And they do, in visitStoreTypedArrayElementHole for instance:

    masm.branch32(Assembler::BelowOrEqual, ToOperand(lir->length()), Imm32(idx), &skip);

So this could be an ARM-only sec-high or sec-crit :(

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Jan de Mooij [:jandem] from comment #9)
> So this could be an ARM-only sec-high or sec-crit :(

Assume the worst - setting sec-critical.

Comment 11

[Douglas Crosher [:dougc]]

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

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

Looks good.

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +892,1 @@
>          }

Perhaps consider a source comment like the one for:

branch32(Condition cond, AbsoluteAddress lhs, Register rhs, Label *label)
 ... // ma_cmp will use the scratch register.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

sec-critical => tracking.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Patch

I noticed branchTest32 has the same problem. We really need to fix bug 986680.

Comment 14

[Douglas Crosher [:dougc]]

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

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

Looks good. Well spotted.

Comment 15

[Jan de Mooij [:jandem]]

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

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's possible by looking at the callers... Not sure it's really exploitable but it probably is.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
There are some comments, but even without them the problem is pretty obvious from the code changes.

> Which older supported branches are affected by this flaw?
All, most likely.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to backport.

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

Comment 16

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

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

sec-approval+ for checkin on July 1 or later. We should have branch patches prepped to go in once it is green there.

Comment 17

[Douglas Crosher [:dougc]]

Attachment: Backport for Beta 31.

Here's the backport for beta (31).

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Douglas Crosher [:dougc] from comment #17)
> Created attachment 8441086 [details] [diff] [review]
> Use the second scratch register to avoid clobbering the scratch register.
> 
> Here's the backport for beta (31).

Thanks!

Comment 19

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

Moving checkin up a week to earlier in the cycle after ping by Douglas on dates.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42f98b5cfcf2

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/42f98b5cfcf2

Comment 22

[Douglas Crosher [:dougc]]

Needs uplifting to Beta 31 and Aurora 32. The patch does not need modification for Aurora 32, and there is a backport patch for Beta 31.

Comment 23

[Douglas Crosher [:dougc]]

(In reply to Douglas Crosher [:dougc] from comment #22)
> Needs uplifting to Beta 31 and Aurora 32. The patch does not need
> modification for Aurora 32, and there is a backport patch for Beta 31.

Try run for Aurora 32: https://tbpl.mozilla.org/?tree=Try&rev=db4e37c37732

Try run for Beta 31: https://tbpl.mozilla.org/?tree=Try&rev=7934c67ce6a4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

You still need to go through the regular approval process for Aurora/Beta uplifts. sec-approval is only for landing on trunk. Also, does this need an esr24 patch as well?

Comment 25

[Douglas Crosher [:dougc]]

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

Approval Request Comment
[Feature/regressing bug #]: Long standing issues.

[User impact if declined]: Crashes, bad computation, maybe exploitable.

[Describe test coverage new/current, TBPL]: m-c, passes the test case, jit-tests.

[Risks and why]: Low risk because it's a small obvious problem.

[String/UUID change made/needed]: n/a

Comment 26

[Douglas Crosher [:dougc]]

Comment on attachment 8441086 [details] [diff] [review]
Backport for Beta 31.

Approval Request Comment
[Feature/regressing bug #]: Long standing issues.

[User impact if declined]: Crashes, bad computation, maybe exploitable.

[Describe test coverage new/current, TBPL]: m-c, passes the test case, jit-tests.

[Risks and why]: Low risk because it's a small obvious problem.

[String/UUID change made/needed]: n/a

Comment 27

[Douglas Crosher [:dougc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> You still need to go through the regular approval process for Aurora/Beta
> uplifts. sec-approval is only for landing on trunk.

Ok, thanks.

> Also, does this need an esr24 patch as well?

ESR 24 is affected. The Beta 31 patch looks like it would apply to ESR 24 but I have not tried it. Approval to uplift to ESR 24 has also been requested, lets see.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6968f4e3e383
https://hg.mozilla.org/releases/mozilla-beta/rev/9f3d7d823bc3
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1cba3e880e7e
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/f99b87d0b208
https://hg.mozilla.org/releases/mozilla-esr24/rev/e740d3af5dea

Comment 29

[Christian Holler (:decoder)]

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

Comment 30

[Kamil Jozwiak [:kjozwiak]]

Updated m-c to revision 41a54c8add09 and used the following configurations:

../configure --enable-debug --disable-optimize --enable-arm-simulator
../configure --enable-arm-simulator
../configure --enable-debug --disable-optimize
../configure

Once configured, I ran the following commands:
- ./js --fuzzing-safe --ion-eager poc.js
- valgrind ./js --fuzzing-safe --ion-eager poc.js

I couldn't reproduce the original issue as per comment #0. Decoder, was there other configuration flags that were being used originally?

Comment 31

[Kamil Jozwiak [:kjozwiak]]

Talked to decoder in IRC and said he used the following flags during configuration:
- ../configure --enable-debug --enable-optimize --disable-threadsafe --enable-arm-simulator

Reproduced the original issue with m-c using changset 41a54c8add09:

Assertion failure: str1->length() == str2->length(), at /home/kjozwiak/mozilla/mozilla-central/js/src/jit/IonCaches.cpp:2955
Segmentation fault (core dumped)

Went through the following verification:

- firefox31 using revision 6befadcaa685 [original crash isn't occurring, valgrind didn't report any issues]
- firefox-esr24 using revision 31b0c1ff3c0b [original crash isn't occurring, valgrind didn't report any issues]