1534492 - Assertion failure: base() != other.reg(), at js/src/jit/MoveResolver.h:109

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

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

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.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

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.

Comment 3

[Neha Kochar [:neha]]

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.

Comment 4

[Sean Stangl [:sstangl]]

This still reproduces. Setting to P2 for tracking.

Comment 5

[Nicolas B. Pierron [:nbp]]

I can reproduce this issue as well. Taking.

Comment 6

[Nicolas B. Pierron [:nbp]]

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.

Comment 7

[Nicolas B. Pierron [:nbp]]

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.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1534492 - Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a10d3faab9e
Backed out changeset 1778699fa005 for spidermonkey bustages on a CLOSED TREE.

Comment 11

[Oana Pop-Rus]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b23278bf5294

Comment 14

[Nicolas B. Pierron [:nbp]]

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

[Pascal Chevrel:pascalc (PTO until April 26)]

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

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d7c9a41c240