Assertion failure: base() != other.reg(), at js/src/jit/MoveResolver.h:109
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox66 | --- | unaffected |
firefox67 | --- | fixed |
firefox68 | --- | fixed |
People
(Reporter: gkw, Assigned: nbp)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [jsbugmon:update])
Attachments
(2 files)
13.22 KB,
text/plain
|
Details | |
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
The following testcase crashes on mozilla-central revision 52b03bc34899 (build with --enable-debug --enable-more-deterministic --enable-simulator=arm64, run with --fuzzing-safe --no-threads --ion-eager --ion-sincos=on):
x = [0];
for (let i = 0; i < 1; ++i) {
try {
Math.cos(x[0])(Math.sin(x[0]));
} catch (e) {}
}
Backtrace:
#0 js::jit::MoveOperand::aliases (this=<optimized out>, other=...) at js/src/jit/MoveResolver.h:108
#1 0x0000562b91fafc78 in js::jit::MoveResolver::addOrderedMove (this=0x7f2470396b88, move=...) at js/src/jit/MoveResolver.cpp:312
#2 0x0000562b91fab36a in js::jit::MoveResolver::resolve (this=0x7f2470396b88) at js/src/jit/MoveResolver.cpp:290
#3 0x0000562b91c92c77 in js::jit::MacroAssembler::callWithABIPre (this=0x7f2470396040, stackAdjust=<optimized out>, callFromWasm=<optimized out>) at js/src/jit/arm64/MacroAssembler-arm64.cpp:794
#4 0x0000562b91faaa0f in js::jit::MacroAssembler::callWithABINoProfiler (this=0x7f2470396040, fun=0x7f246fe940c8, result=js::jit::MoveOp::GENERAL, check=js::jit::CheckUnsafeCallWithABI::Check) at js/src/jit/MacroAssembler.cpp:3150
/snip
For detailed crash information, see attachment.
Likely another Ion on ARM64 issue.
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
autobisectjs shows this is probably related to the following changeset:
The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/afb2e1e1665f
user: Sean Stangl
date: Thu Mar 07 03:57:23 2019 +0000
summary: Bug 1528869 - Enable IonMonkey in the ARM64 shell, but keep it disabled in the browser. r=nbp
Setting needinfo? from Sean/Nicolas again.
Reporter | ||
Updated•6 years ago
|
Comment 3•6 years ago
|
||
This is a JS fuzzing bug and sdetar said the team will fix as many as they can in 67 but will not be able to get to all. Marking them fix-optional.
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
I can reproduce this issue as well. Taking.
Assignee | ||
Comment 6•6 years ago
|
||
For information: I have a one line work-around for this issue, but I want to understand this problem better before posting it, to make sure I am not missing other cases.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
This bug is one instance of a bug which which ""might"" be experienced on all platforms except MacOSX, since Bug 984018.
This bug is a MoveResolver bug which might read a non-normalized double from the stack, which contains stale data including forged data with a JS::Value pointer meant to read/write any memory. One can potentially use this bug to get random memory access.
The problem here is that MSinCos uses passABIArg to call js::math_sincos_impl
. However passABIArg is unable to handle cycles with a random base pointer given as address, it has to be a register which is not used by the call convention as an argument register.
Here references to the Sin and Cos values are reserved and referenced by the params
register which is freely allocated by the register allocator. This implies that if the params
register matches the second argument register. This implies that the second reference would be equal to the first one, and that the js::math_sincos_impl
function would not override the stack space. This stack space is then read back out of the stack pointer and stored as a double value, which is assumed to be in a canonical form (NaN). Making this double value an argument of a non-JIT-ed function will allow the non-JIT-ed function to interpret this non-cannonicalized double as a JS::Value, with a forged pointer to the stack, which might emulate an Array with wide bounds.
[…]
While double checking the generated code on all platform, I noticed that this is a non-issue because Sin and Cos address computations happen to be inverted, so even if there is a register override case, it would only be overwritten with the same value or not be reused.
The instruction order written here:
https://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#9214-9218
Is inverted by:
https://searchfox.org/mozilla-central/source/js/src/jit/MoveResolver.cpp#252-255
And thus even if the code is incorrect, this is non-exploitable, and works as expected except for this assertion. (I am amazed …)
I will note that I have not seen any other instance of this issue while proof-reading the rest of uses of passABIArg.
Assignee | ||
Comment 8•6 years ago
|
||
Pushed by npierron@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1778699fa005 Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand. r=sstangl
Comment 10•6 years ago
|
||
Backout by opoprus@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0a10d3faab9e Backed out changeset 1778699fa005 for spidermonkey bustages on a CLOSED TREE.
Comment 11•6 years ago
|
||
Backed out changeset 1778699fa005 (bug 1534492) for spidermonkey bustages
Backout: https://hg.mozilla.org/integration/autoland/rev/0a10d3faab9eb28b4fecd19c05944871f4e6f466
Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=238114507&revision=1778699fa005874daead175293e374af9c7a092d
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238114507&repo=autoland&lineNumber=1016
Comment 12•6 years ago
|
||
Pushed by npierron@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b23278bf5294 Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand. r=sstangl
Comment 13•6 years ago
|
||
bugherder |
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 9055501 [details]
Bug 1534492 - Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand.
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: None
- User impact if declined: debug-builds crashes or fuzzing false-positive reports.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): force register allocation into picking registers which are avoiding assertions in debug builds.
- String changes made/needed: None
Comment 15•6 years ago
|
||
Comment on attachment 9055501 [details]
Bug 1534492 - Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand.
Low risk assertion fix, minimal patch, approved for 67 beta 10, thanks.
Comment 16•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Description
•