1571918 - Differential Testing: Different output message on ARM32 involving Math.atan2

Status: RESOLVED FIXED

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

Description

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

function g(f, x) {
    var y = [];
    for (var j = 0; j < x.length; ++j) {
        for (var k = 0; k < x.length; ++k) {
            y.push(f(x[j], x[k]));
        }
    }
    print(uneval(y));
}
function f1(x) {
    return (-!(x | 0) > -(x | 0));
}
g(f1, []);
function f2(x, y) {
    return (+f1(+f1((x ? Math.fround(Math.fround(x) || y | 0) : Math.fround(x)) != x > 0), +f1(Math.atan2(!x | 0, Number.MAX_SAFE_INTEGER)) - !(x | 0)));
}
g(f2, [2147483649, 0]);

$ ./js-dbg-32-dm-armsim32-linux-x86_64-747f5a90f7d8 --fuzzing-safe --no-threads --ion-eager testcase.js
[]
[1, 1, 0, 0]

$ ./js-dbg-32-dm-armsim32-linux-x86_64-747f5a90f7d8 --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis testcase.js
[]
[1, 1, 1, 1]

Tested this on m-c rev 747f5a90f7d8.

My configure flags are:

AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig 'CXX="clang++ -m32 -msse2 -mfpmath=sse"' 'CC="clang -m32 -msse2 -mfpmath=sse"' sh ./configure --target=i686-pc-linux --enable-simulator=arm --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/098a1fc45a87
user: Iain Ireland
date: Wed Jun 12 18:30:12 2019 +0000
summary: Bug 1401624: Part 3: Convert store/loadDouble to box/unboxDouble r=tcampbell,mgaudet

Setting s-s to be safe as I am not sure how bad this is. Iain, is bug 1401624 a likely regressor?

Also, is bug 1555169 related in any way?

Comment 1

[Iain Ireland [:iain]]

I'm looking at this. ARM32 is awkward, because rr doesn't work on the simulator.

I have vague suspicions that something is wrong with fround, which could also be the problem in bug 1555169, but I won't know until I make more progress understanding the real issue.

Comment 2

[Iain Ireland [:iain]]

It took parallel rr sessions walking backwards through the simulator, but I finally tracked down the source of this bug. The problem is in MoveResolver, in the code that works around ARM's weird overlapped floating point register file.

If a double move and a single (aka float) move alias each other, then we split the double move into two separate single moves. However, in this particular testcase, we create a move group containing:

A: Move [sp+72] --> f14
B: Move [sp+40] --> d6
C: Move d7 --> [sp+40]

(Note: sp+X is the memory at offset X from the stack pointer. Double register d7 overlaps with float registers f14 and f15.)

Because d7 and f14 overlap, A and C alias each other. We split C into two moves, giving us:

A: Move [sp+72] --> f14
B: Move [sp+40] --> d6
C1: Move f14 --> [sp+40]
C2: Move f15 --> [sp+44]

However, when we try ordering C2, we don't realize that it aliases the high half of B. We end up ordering these moves C2, B, C1, A. As a result, the value on the stack gets clobbered before we read it into d6, and we end up getting the wrong answer.

I believe this can be used to copy an arbitrary float into the high half of a Value, which probably leads to exploitable type-confusion. Bumping this up to sec-moderate.

The root of the problem is that by splitting C into two single moves, we retroactively force B to also be split, because it has become aliased by a single move. Unfortunately, we've already iterated past it and decided not to split it. We could use some sort of fix-point algorithm, but I think it's easier (and paints less of a spotlight) to divide the splitting logic into two halves. First we check to see if any double moves alias single moves. Second, if any do, we split all double moves into single moves.

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1571918: Split more double moves r=sstangl

Comment 4

[Iain Ireland [:iain]]

(Also, this is unrelated to bug 1555169.)

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Iain Ireland [:iain] from comment #2)

It took parallel rr sessions walking backwards through the simulator, but I finally tracked down the source of this bug.

Nice detective work, thanks for tracking this down!

Comment 6

[Iain Ireland [:iain]]

Comment on attachment 9085934 [details]
Bug 1571918: Split more double moves r=sstangl

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would be difficult. The problem fixed by this patch was already mentioned here. At the time we thought it was safe, because we had not considered moves to/from memory. This patch doesn't mention memory at all.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: Bug 1299147
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Trivial.
• How likely is this patch to cause regressions; how much testing does it need?: Pretty unlikely. At worst, this could cause slight performance regressions (arm32 only) due to additional register moves.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9085934 [details]
Bug 1571918: Split more double moves r=sstangl

As a sec-moderate issue, this doesn't need sec-approval to go into trunk. We only expect it for high and critical rated security issues (or issues without a rating, which will need one) that affect more than trunk.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/c74a30be936c3292f755f634bf64f2e03d3d6879
https://hg.mozilla.org/mozilla-central/rev/c74a30be936c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Too late for 69.