Closed
Bug 1012694
Opened 10 years ago
Closed 10 years ago
Assertion failure: str1->length() == str2->length(), at jit/IonCaches.cpp:2955
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
People
(Reporter: decoder, Assigned: jandem)
Details
(4 keywords, Whiteboard: [jsbugmon:update][adv-main31+][adv-esr24.7+])
Attachments
(3 files, 2 obsolete files)
521 bytes,
application/javascript
|
Details | |
3.05 KB,
patch
|
dougc
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.93 KB,
patch
|
abillings
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr24+
|
Details | Diff | Splinter Review |
The attached testcase asserts on mozilla-central revision 41a54c8add09 (run with --fuzzing-safe --ion-eager).
Reporter | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(mrosenberg)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
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); ... }
Assignee | ||
Comment 7•10 years ago
|
||
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 | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
(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 :(
Assignee | ||
Updated•10 years ago
|
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → affected
tracking-firefox31:
--- → ?
tracking-firefox32:
--- → ?
tracking-firefox33:
--- → ?
Comment 10•10 years ago
|
||
(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.
Keywords: regression,
sec-critical
Comment 11•10 years ago
|
||
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+
Comment 12•10 years ago
|
||
sec-critical => tracking.
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox-esr24:
--- → affected
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 7/1]
Comment 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
Here's the backport for beta (31).
Updated•10 years ago
|
Flags: needinfo?(mrosenberg)
Assignee | ||
Comment 18•10 years ago
|
||
(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•10 years ago
|
||
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]
Updated•10 years ago
|
Keywords: checkin-needed
Comment 20•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•10 years ago
|
Attachment #8441086 -
Attachment description: Use the second scratch register to avoid clobbering the scratch register. → Backport for Beta 31.
Comment 22•10 years ago
|
||
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
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
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?
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → fixed
Keywords: checkin-needed
Comment 25•10 years ago
|
||
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 26•10 years ago
|
||
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?
Comment 27•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8439151 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
tracking-firefox-esr24:
--- → 31+
Updated•10 years ago
|
Attachment #8441086 -
Flags: approval-mozilla-esr24?
Attachment #8441086 -
Flags: approval-mozilla-esr24+
Attachment #8441086 -
Flags: approval-mozilla-beta?
Attachment #8441086 -
Flags: approval-mozilla-beta+
Comment 28•10 years ago
|
||
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
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 29•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed. JSBugMon: This bug has been automatically verified fixed on Fx32
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main31+][adv-esr24.7+]
Comment 30•10 years ago
|
||
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)
Comment 31•10 years ago
|
||
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)
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•