Closed Bug 1012694 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

ARM
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox30 --- wontfix
firefox31 + verified
firefox32 + verified
firefox33 + verified
firefox-esr24 31+ verified
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main31+][adv-esr24.7+])

Attachments

(3 files, 2 obsolete files)

Attached file Testcase for shell
The attached testcase asserts on mozilla-central revision 41a54c8add09 (run with --fuzzing-safe --ion-eager).
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.
Flags: needinfo?(mrosenberg)
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Needinfo from Jan based on comment 2.
Flags: needinfo?(jdemooij)
I'm pretty sure this is another assembler buffer issue, it's possible bug 1008590 exposed it. Leaving the needinfo for mjrosenb.
Flags: needinfo?(jdemooij)
(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.
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);
...
}
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8437684 - Flags: review?(dtc-moz)
Attached patch Patch (obsolete) — Splinter Review
There's another function that needs the same fix.
Attachment #8437684 - Attachment is obsolete: true
Attachment #8437684 - Flags: review?(dtc-moz)
Attachment #8437691 - Flags: review?(dtc-moz)
(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 :(
(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 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.
Attachment #8437691 - Flags: review?(dtc-moz) → review+
sec-critical => tracking.
Attached patch PatchSplinter Review
I noticed branchTest32 has the same problem. We really need to fix bug 986680.
Attachment #8437691 - Attachment is obsolete: true
Attachment #8439151 - Flags: review?(dtc-moz)
Comment on attachment 8439151 [details] [diff] [review]
Patch

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

Looks good. Well spotted.
Attachment #8439151 - Flags: review?(dtc-moz) → review+
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.
Attachment #8439151 - Flags: sec-approval?
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 7/1]
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.
Attachment #8439151 - Flags: sec-approval? → sec-approval+
Here's the backport for beta (31).
Flags: needinfo?(mrosenberg)
(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!
Moving checkin up a week to earlier in the cycle after ping by Douglas on dates.
Whiteboard: [jsbugmon:update][checkin on 7/1] → [jsbugmon:update][checkin on 6/24]
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f98b5cfcf2
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [jsbugmon:update][checkin on 6/24] → [jsbugmon:update]
https://hg.mozilla.org/mozilla-central/rev/42f98b5cfcf2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8441086 - Attachment description: Use the second scratch register to avoid clobbering the scratch register. → Backport for Beta 31.
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.
Keywords: checkin-needed
(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
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 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
Attachment #8439151 - Flags: approval-mozilla-aurora?
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
Attachment #8441086 - Flags: approval-mozilla-esr24?
Attachment #8441086 - Flags: approval-mozilla-beta?
(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.
Attachment #8439151 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8441086 - Flags: approval-mozilla-esr24?
Attachment #8441086 - Flags: approval-mozilla-esr24+
Attachment #8441086 - Flags: approval-mozilla-beta?
Attachment #8441086 - Flags: approval-mozilla-beta+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx32
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main31+][adv-esr24.7+]
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?
Flags: needinfo?(choller)
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]
QA Whiteboard: [qa!]
Flags: needinfo?(choller)
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.