1716231 - Differential Testing: Different output message involving Math.fround

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

During fuzzing, I encountered a differential execution seemingly related to Math.fround optimization in ion. This is a first impression and might be misleading.
The following testcase produces different results on mozilla-central git commit 077501b34cca91763ae04f4633a42fddd919fdbd depending on whether ion is enabled or disabled.

function main() {
    const v0 = [0,0]; // removing surpresses the bug
    const v49 = 67109020;
    let v138;

    for (let v102 = 0; v102 < 14; v102++) {
        for (let v117 = 0; v117 < 6; v117++) {}

        const v136 = Math.fround(0);
        // doesn't trigger for Math.{atan(0), sqrt(0), atan2(0,0), hypot(0)}
        v138 = v136 || v49;
    }
    // without ion, v138 is 67109020. with ion 67109020 + 4

    return v138;
}

const result = main();
print(result);

Actual results:

Running the sample with ion disabled (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --baseline-warmup-threshold=10 --fuzzing-safe --differential-testing --no-ion diff.js) the value 67109020 in returned (as expected, just the value of const v49).

If ion is enabled (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --ion-offthread-compile=off --baseline-warmup-threshold=10 --ion-warmup-threshold=100 --fuzzing-safe --differential-testing diff.js) the execution returns 67109024, so 4 larger than the expected result.

Marking as security as the precise implications haven't been studied further.

Comment 1

[Steven DeTar [:sdetar]]

:Jandem, :iireland, could one of you investigate this new bug (if you have not already)

Comment 2

[Iain Ireland [:iain]]

This is a bug in the float32 optimization in GuessPhiType, exposed by my branch-pruning changes and the interaction with on-stack replacement. Here's a slightly simplified / annotated version of the testcase.

function call_with_no_ic_data() {}

function main() {
    // This executes in the C++ interpreter before we allocate
    // an ICScript, so it doesn't have any IC data recorded.
    // This means we have an unconditional bailout on the path
    // between the function entry and the loop, so branch
    // pruning removes that edge, and the only way into the
    // loop is via OSR. This could be anything so long as it
    // uses an IC.
    call_with_no_ic_data();

    // This value can't be precisely represented as a 32-bit float.
    const too_big_for_float32 = 67109020;
    let result;

    // We OSR in this loop.
    for (let i = 0; i < 100; i++) {
        const float32 = Math.fround(0);

	// Create a phi with one float32-typed input and one 
        // OSRValue input. This is where things go wrong.
        result = float32 || too_big_for_float32;
    }
    return result;
}

print(main());

The operands to the phi are ordered with the OSRValue before the Float32 value. The bug is in GuessPhiType: we set convertibleToFloat32 to false when we see an OSRValue, but that doesn't actually prevent us from setting it to true again later when we see a Float32. (If the operands were in the other order, we would first set the type to Float32, then update it to Double when we saw the OSRValue.)

In the long term, the fix is probably to split the float32 analysis out into its own pass instead of trying to do it during type analysis (bug 1655773). In the short term, we can do a spot-fix.

I think this is unlikely to be exploitable. Range analysis for MToFloat32 was disabled in bug 944321. Jan, thoughts?

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1716231: Clean up float32 handling in GuessPhiType r=jandem

Comment 4

[Jan de Mooij [:jandem]]

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

I think this is unlikely to be exploitable. Range analysis for MToFloat32 was disabled in bug 944321. Jan, thoughts?

Makes sense and matches what we've done in the past for float32 analysis bugs. Since the fix is simple, probably worth requesting uplift to 90.

Comment 5

[Iain Ireland [:iain]]

Comment on attachment 9226919 [details]
Bug 1716231: Clean up float32 handling in GuessPhiType r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: A JS value too large to be precisely represented as a 32-bit float is incorrectly rounded to the closest representable 32-bit float. This can produce incorrect results. We don't see an obvious mechanism to turn this into an out-of-bounds read, because range analysis is already disabled for conversions to float32, but we can't rule out the possibility.
• 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): The fix is simple. It disables an optimization in a superset of the cases where we already disabled that optimization.

The alternative is to let this ride the trains.

• String changes made/needed:

Comment 6

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/6b5a72d078e7b29013a17ff87dbb5c7893bcdc8a
Clean up float32 handling in GuessPhiType r=jandem

Comment 7

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/6b5a72d078e7

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 9226919 [details]
Bug 1716231: Clean up float32 handling in GuessPhiType r=jandem

approved for 90.0b9

Comment 9

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1f0a48237a14