1720093 - Differential testing: incorrect float32 computation during OSR

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:

The following sample miscomputes when invoked via the following command line:

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 diff.js

function main() {
    let obj = {2294967295: 0, 2294967296: 1}; 
    let v34;

    for (let v32 = 0; v32 < 110; v32++) {
        let v33 = 2294967296;
        v34 = --v33; // this becomes a (sub (tofloat32 (constant 2294967296)) (tofloat32 (constant 1)))
        Math.fround(0); // needed to trigger float32 analysis
    }   
    try {
        throw 0;
    } catch(v53) { } 

    print(obj[v34]); // prints 1, should be 0. I suppose using v34 should prohibit the float32 conversion?
}
main();

Bisecting the issue identifies the first bad commit as: a53ba9e2a03c6e64e540bcb911555b60a06d9b54
=> see bug 1648005

This is might not actually be security-sensitive as (afaik) there is no range analysis when float32 values are involved.

Comment 1

[Jan de Mooij [:jandem]]

Iain, do you have time to look at this one, since you've fixed some OSR/Float32 bugs before?

Comment 2

[Iain Ireland [:iain]]

Sure, I'll take a look.

Comment 3

[Iain Ireland [:iain]]

The bug here is pretty straight-forward. We OSR in the loop. In the loop body, we have:

48 constant 2294967296
49 constant 1
50 sub constant48:Double constant49:Double [double]

The only uses of sub50 are the phis for v33 and v34 in the loop header. Those phis are only used in resume points, including the resume point we use when we bail out for the throw.

The loop also contains an unrelated fround, so float32 analysis is enabled. The analysis decides that the phis are potential float32 consumers: they aren't marked as implicitly used, and they don't have any visible uses that can't consume float32. Because of this, sub50 is converted to float32.

Later, GVN does constant folding and folds 2294967296 - 1 to 2294967296, because float32 can't represent 2294967295. When we hit the end of the loop and bail out, we recover the wrong value.

This isn't a security concern. In addition to range analysis being disabled for float32, this bug only happens because we don't know about any other consumer of v34, so there's no way to use the incorrect value in Warp-compiled code without disabling the bug.

I'm not sure what the best fix is. One observation is that this only occurs because WarpBuilder doesn't build any MIR after the try/catch, because we can't reach that code without going through the catch. If the try/catch is removed, then we end up with effectively the same MIR graph (because the code after the loop has no CacheIR), but we mark the phi for v34 as implicitly used because it's an input to a WarpBailout snapshot. Maybe the problem is just that the WarpBuilder code that skips over unreachable code doesn't distinguish between genuinely unreachable code and code that is unreachable in Warp (aka via a catch block).

I think the only ways to "exit" a function (in the sense that there's no edge leaving the block) that can actually resume execution in the same stack frame are yield and throw. build_Suspend already adds explicit uses for everything on the stack while storing them in the generator object; if we mark everything on the stack as implicitly used in build_Throw, that might fix the problem.

I'll test it out tomorrow.

Comment 4

[Jan de Mooij [:jandem]]

We disable certain optimizations if graph.hasTryBlock(). It's not ideal but might also work here.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1720093: Mark instructions that could be used after a catch as implicitly used r=jandem

These three throw instructions are the only place in WarpBuilder where we generate MUnreachable.

(Note that throws that happen before we see a try will not have their operands marked, but that's fine, because they can't be caught in this function.)

I think this patch probably allows us to remove code elsewhere that disables optimizations if we have a try block, but I'm not completely sold on the risk/reward.

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1720093: Disable float32 optimization for try blocks r=jandem

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2920fb65a643
Disable float32 optimization for try blocks r=jandem

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2920fb65a643