1555169 - Differential Testing: Different output message on ARM32

Status: NEW

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

Description

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

function f() {
    return (Math.round(Math.tan(Math.atan2(Math.clz32(2 && !Math.cos()), ~!0))));
}
for (let i = 0; i < 2; ++i) {
    print(f());
}

$ ./js-dbg-32-dm-armsim32-linux-x86_64-d8321d0b2c5b --fuzzing-safe --no-threads --ion-eager testcase.js
-16
-15

$ ./js-dbg-32-dm-armsim32-linux-x86_64-d8321d0b2c5b --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
-16
-16

Tested this on m-c rev d8321d0b2c5b.

My configure flags are:

PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig 'CXX="clang++ -m32 -msse2 -mfpmath=sse"' AR=ar '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 --disable-cranelift

This seems to go way back even till m-c rev 25e99bc12482, setting s-s to be safe as I am not sure how bad this is.

Jan, do you mind taking a look as a start?

Comment 1

[Jan de Mooij [:jandem]]

ARM specific probably, maybe Sean can take a look.

Comment 2

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

for (let i = 0; i < 2; ++i) {
    print(Math.round(Math.fround(-0.8 - 9007199254740991 % (9007199254740990 >>> 0))));
}

$ ./js-dbg-32-dm-armsim32-linux-x86_64-50df4b75c9b6 --fuzzing-safe --no-threads --ion-eager testcase.js
-4194304
-4194303

$ ./js-dbg-32-dm-armsim32-linux-x86_64-50df4b75c9b6 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
-4194304
-4194304

Compile with PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig 'CC="clang -m32 -msse2 -mfpmath=sse"' AR=ar 'CXX="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

Tested on m-c rev 50df4b75c9b6 on Ubuntu 18.04 with clang version 8. This testcase variant also seems to go back to m-c rev 25e99bc12482 so I'm unable to get a good bisection.

Comment 3

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

Similar differential testing issues (bug 1571918) was found to be a sec-moderate, but that is unrelated to this, according to Iain.

Sean, re-ping please?

Comment 4

[Iain Ireland [:iain]]

I took a quick look at this yesterday. It looks like it's an edge case in our roundf code for arm32. I'm pretty sure it's not a security bug.

Comment 5

[Iain Ireland [:iain]]

The problem here is in our arm32 implementation of roundf.

We are trying to round the float32 value -4194303.75. Because it is negative, we end up in this path:

    // Add 0.5 to negative numbers, storing the result into tmp.
    ma_vneg_f32(input, tmp);
    loadConstantFloat32(0.5f, scratchFloat);
    ma_vadd_f32(tmp, scratchFloat, scratchFloat);

    // Adding 0.5 to a float input has chances to yield the wrong result, if
    // the input is too large. In this case, skip the -1 adjustment made below.
    compareFloat(scratchFloat, tmp);

    // Negative case, negate, then start dancing. This number may be positive,
    // since we added 0.5.
    // /!\ The conditional jump afterwards depends on these two instructions
    //     *not* setting the status flags. They need to not change after the
    //     comparison above.
    ma_vcvt_F32_U32(scratchFloat, tmp.uintOverlay());
    ma_vxfer(VFPRegister(tmp).uintOverlay(), output);

    Label flipSign;
    ma_b(&flipSign, Equal);

    // -output is now a correctly rounded value, unless the original value was
    // exactly halfway between two integers, at which point, it has been rounded
    // away from zero, when it should be rounded towards \infty.
    ma_vcvt_U32_F32(tmp.uintOverlay(), tmp);
    compareFloat(tmp, scratchFloat);
    as_sub(output, output, Imm8(1), LeaveCC, Equal);

    // Negate the output. Since INT_MIN < -INT_MAX, even after adding 1, the
    // result will still be a negative number.
    bind(&flipSign);
    as_rsb(output, output, Imm8(0), SetCC);

We negate the value (to make it positive) and add 0.5. However, this pushes it over a precision threshold: a float can represent 4194303.75, but 4194304.25 is rounded to 4194304. When we convert 4194304 to an integer and back, we obviously get the same value, so our algorithm believes we've rounded in the wrong direction and subtracts 1.

This might actually be the only input value for which this bug can occur. I'm not sure what the best fix is, but having worked through the logic I'm confident this is not a security bug.

Comment 6

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

Thanks Iain!

Comment 7

[Iain Ireland [:iain]]

IIRC, nbp set this to P1 because of concerns about range analysis. However, we don't do range analysis on MRound nodes, so we're safe there.

I'm bumping this down to P2. It would be nice to fix it, but floating point algorithms are hard, and I wouldn't want to introduce new bugs while fixing this one. ARM32 isn't as important these days, anyway.