1654947 - Issue involving bitwise operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    return x >>> Math.round >>> x, x;
}
let m = [0, 2 ** 31-1];
let y;
let z;
for (let i = 0; i < 2; i++) {
    for (let j = 0; j < 3; j++) {
        y = f(m[j]);
        if (j === 1) {
            z = y;
        }
    }
}
assertEq(z, 2147483647);

AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Assertion fails on latest m-c rev 08c29f9d8779 with --fuzzing-safe --no-threads --ion-eager, using clang 9.0.1, passes when run with --fuzzing-safe --no-threads --baseline-eager --no-ion. Also passes when run on rev 73aeebb42476, the parent of the potential regressor on both sets of flags.

$ ./js-dbg-64-dm-linux-x86_64-4d570314ac00 --fuzzing-safe --no-threads --ion-eager 2testcase.js
2testcase.js:15:9 Error: Assertion failed: got 31, expected 2147483647
Stack:
  @2testcase.js:15:0

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4d570314ac00
user:        Jan de Mooij
date:        Wed Jul 22 17:11:39 2020 +0000
summary:     Bug 1647602 part 2 - Make GuardToInt32 match other guard instructions. r=iain

May be related to bug 1654930 but that bug needs --no-sse3 (or --no-ssse3 or --no-sse4 or --no-sse41) and also involves Math.cbrt, this one does not.

Unsure how bad this is, so marking s-s and setting needinfo? from :jandem. Please open up if this is not the case.

Comment 1

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

I can get the assert to fail as well, by changing Math.round to Math.fround, so it isn't just a Math.round issue.

Comment 2

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

function f(x) {
    return x >> [] >> x, x;
}
let m = [0, 2 ** 31-1];
let y;
let z;
for (let i = 0; i < 2; i++) {
    for (let j = 0; j < 3; j++) {
        y = f(m[j]);
        if (j === 1) {
            z = y;
        }
    }
}
assertEq(z, 2147483647);

This reproduces as well. I'd hazard another guess that this involves bitwise operations instead of Math.*.

Comment 4

[Jan de Mooij [:jandem]]

The shift instructions incorrectly mutate their rhs register (by masking it with 0x1F). Bug 1647602 part 2 happened to make that observable but the code was incorrect long before that. I did an audit of Int32OperandId uses and I think these are the only problematic ones.

I'm not sure if this is exploitable. You always get a masked value in range 0-31. Combined with range analysis that could be bad, but this also involves using boxed Values and RA doesn't optimize those AFAIK.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1654947 - Don't clobber RHS register for shift instructions. r?iain!

The masking was unnecessary anyway on x86/x64/arm64 because the CPU does it (and
we are already relying on that in Ion). For arm32 handle this in the flexible*
MacroAssembler implementation by using the scratch register.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1647602

Comment 7

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

https://hg.mozilla.org/integration/autoland/rev/f5c234cd9add0d84481a1bd17b6519739829f8e6
https://hg.mozilla.org/mozilla-central/rev/f5c234cd9add

Comment 8

[Daniel Veditz [:dveditz]]

This looks like a correctness bug rather than a security bug. Leaving the bug hidden for now in case I'm wrong about that.