Closed
Bug 1395100
Opened 7 years ago
Closed 7 years ago
Assertion failure: cmpret == 0, at js/src/jit/arm/Simulator-arm.cpp:1074
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | + | fixed |
firefox57 | + | fixed |
People
(Reporter: gkw, Assigned: luke)
References
Details
(6 keywords, Whiteboard: [fuzzblocker][jsbugmon:update][post-critsmash-triage])
Attachments
(2 files, 1 obsolete file)
6.69 KB,
text/plain
|
Details | |
7.94 KB,
patch
|
jandem
:
review+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision db7f19e26e57 (build with --32 --enable-debug --enable-more-deterministic --enable-simulator=arm, run with --fuzzing-safe --no-threads --ion-eager --arm-sim-icache-checks --gc-zeal=14):
Object.getOwnPropertyNames(this);
for (var i = 0; i < 1; ++i) {
[Array];
[ArrayBuffer];
}
Backtrace:
#0 0x08550136 in js::jit::SimulatorProcess::checkICacheLocked (instr=0x56376f4c) at js/src/jit/arm/Simulator-arm.cpp:1074
#1 0x08552eea in js::jit::Simulator::instructionDecode (this=0xf7131000, instr=0x56376f4c) at js/src/jit/arm/Simulator-arm.cpp:4737
#2 0x08556a04 in js::jit::Simulator::execute<false> (this=0xf7131000) at js/src/jit/arm/Simulator-arm.cpp:4837
#3 js::jit::Simulator::callInternal (this=0xf7131000, entry=0x56356990 "\360O-\351\004\320M\342\020\212-\355\r\200\240\341h\220\235\345t\240\235", <incomplete sequence \345>) at js/src/jit/arm/Simulator-arm.cpp:4922
#4 0x08556ea9 in js::jit::Simulator::call (this=<optimized out>, entry=<optimized out>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:5005
/snip
For detailed crash information, see attachment.
![]() |
Reporter | |
Comment 1•7 years ago
|
||
![]() |
Reporter | |
Comment 2•7 years ago
|
||
autoBisect shows this is probably related to the following changeset:
The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/8190fc1ba510
user: Benjamin Bouvier
date: Wed Aug 23 16:04:02 2017 +0200
summary: Bug 1392105. r=luke
Luke, is bug 1392105 a likely regressor? (because :bbouvier is away)
Also setting s-s because bug 1392105 is s-s.
![]() |
Assignee | |
Comment 3•7 years ago
|
||
Hah, so I started to re-read the regressing patch to see if I could spot the bug. When I got to TraceOneDataRelocation() I decided to tidy it up not pass around a mutable Iter* everywhere b/c that's just a footgun with everyone calling the mutating iter->next() all over the place. Turns out that fixed the bug and, on further inspection I figured out that the bug was that GetPtr32Target() mutates 'iter' and by using 'iter' later (at the flush calls) instead of the original 'ins', the wrong bytes were getting flushed.
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
![]() |
Reporter | |
Updated•7 years ago
|
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
Updated•7 years ago
|
![]() |
Assignee | |
Comment 4•7 years ago
|
||
Also enables --gc-zeal flag in !JS_GC_ZEAL builds to fix opt builds.
Attachment #8902714 -
Attachment is obsolete: true
Attachment #8902714 -
Flags: review?(jdemooij)
Attachment #8903265 -
Flags: review?(jdemooij)
Comment 5•7 years ago
|
||
Comment on attachment 8903265 [details] [diff] [review]
fix-iter-use
Review of attachment 8903265 [details] [diff] [review]:
-----------------------------------------------------------------
Nice find, and definitely simpler this way.
Attachment #8903265 -
Flags: review?(jdemooij) → review+
Comment 6•7 years ago
|
||
Bug 1392105 landed on beta so this one should be backported too.
tracking-firefox56:
--- → ?
tracking-firefox57:
--- → ?
Updated•7 years ago
|
![]() |
Assignee | |
Comment 7•7 years ago
|
||
Comment on attachment 8903265 [details] [diff] [review]
fix-iter-use
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very hard, this involves icache invalidation only caught by the ARM simulator and possibly not exploitable in real hardware.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
They do, but again, it's just this theoretical icache invalidation. Also the regressing patch only landed two days ago.
Which older supported branches are affected by this flaw?
beta, because uplift of the regressing patch.
If not all supported branches, which bug introduced the flaw?
Bug 1392105
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Very easy to backport.
How likely is this patch to cause regressions; how much testing does it need?
Unlikely: simple bug and simple fix
Attachment #8903265 -
Flags: sec-approval?
Comment 8•7 years ago
|
||
Sec-approval+ for trunk.
We'll want a beta patch made and nominated as well.
Updated•7 years ago
|
Attachment #8903265 -
Flags: sec-approval? → sec-approval+
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/43040e59c2c2f440abe877e6bc4b934661491983
This grafts cleanly to Beta as-is. Just needs an approval nomination when ready :)
Flags: in-testsuite+
![]() |
Assignee | |
Comment 10•7 years ago
|
||
Comment on attachment 8903265 [details] [diff] [review]
fix-iter-use
Approval Request Comment
[Feature/Bug causing the regression]: bug 1392105
[User impact if declined]: probably nothing in practice (see comment 7)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes (manually in the shell)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple bug and simple fix
Attachment #8903265 -
Flags: approval-mozilla-beta?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Attachment #8903265 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•7 years ago
|
||
uplift |
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:update][post-critsmash-triage]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•