1708839 - Incorrect truncation after folding away fallible unbox

Status: RESOLVED FIXED

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

Description

[Jose Martinez tr0y4]

VULNERABILITY DETAILS
The following PoC generates a Firefox JIT incorrect optimization of typed array values, which leads to optimized function return value to be different than unoptimized function return value, similarly to a Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=880207

VERSION
Firefox Version: 88.0 64 bits + Stable
Operating System: Windows 10 x64 + Linux Ubuntu 20 x64

REPRODUCTION CASE

z = (a)=>{let m = new Uint32Array([-1]); let h = m[0]; let r = m[0]; if (a) h = undefined; if (a) r = 0xff; return h > r}

console.log(z(false)) //prints false
for (let i = 0; i < 0x5000; ++i) z(true)
console.log(z(false)) //prints true

ADDITIONAL INFORMATION
Type of bug: optimized function return value is different than unoptimized

on Linux Ubuntu 20 x64 at ./js interpreter: optimized function return value is different than unoptimized

on Windows 10 x64 at Firefox Browser: optimized function return value is different than unoptimized

CREDIT INFORMATION
Reporter credit: Jose Martinez tr0y4 from VerSprite Inc.

Comment 1

[Jan de Mooij [:jandem]]

Thanks for the bug report!

Simplified test below. Iain, can you take a look? It seems related to the RA truncation code.

function f(a) {
    let m = new Uint32Array([-1]);
    let h = m[0];
    let r = m[0];
    if (a) {
        h = undefined;
    }
    if (a) {
        r = 0xff;
    }
    return h > r;
};
assertEq(f(false), false);
for (let i = 0; i < 0x5000; ++i) {
    f(true);
}
assertEq(f(false), false);

Comment 2

[Iain Ireland [:iain]]

The good news is that I'm pretty sure this is another example of a bug where we recover the wrong result on bailout, but it's not a security issue, because baseline works with untyped values.

The bug occurs in the handling of h > r. We transpile the following CacheIR:

      GuardIsUndefined              inputId 0
      GuardIsNumber                 inputId 1
      LoadBooleanResult             val false
      ReturnFromIC   

which produces MIR like this:

56 phi constant53:Int32 loadUnboxedScalar46:Double
57 guardValue phi51:Value
58 unbox phi56 to Double (fallible)
59 constant false

Note that the loadUnboxedScalar for m[0] has MIRType::Double because Int32 can't represent values greater than 0x7fffffff. Note also that neither of the operands is marked as implicitly used, because the guards are explicit uses.

When we apply type policies, phi56 is assigned MIRType::Double. We then insert box phi56:Double before unbox58, because it expects a boxed operand. Later, GVN sees that we are boxing and unboxing a double value, and removes both instructions. But now phi56 has no non-resume-point uses, and since it isn't marked as a guard / implicitly used, range analysis decides that we can truncate it.

I think the problem here is that phi56 is not marked as implicitly used. The simplest fix might be in MUnbox::foldsTo: if we fold MUnbox(MBox(x)) => x, and the unbox is fallible, marking x as implicitly used fixes the problem.

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1708839: Add implicit use after folding away fallible unbox r=jandem

Comment 4

[Iain Ireland [:iain]]

Opening this up and renaming it because it's not a security issue.

We talked about this in the Warp meeting earlier today. The current fix is a bit ad-hoc, but it's not clear what a better approach would be. If similar bugs arise in the future, we will look into the possibility of marking instructions as guards (or implicitly used?) if they are created by the CacheIR transpiler and don't have any other uses, but there's a possibility that this might prevent valid optimizations.

Fortunately the design of Warp means that bugs like this (recovering the wrong value on bailout) aren't exploitable, because we're always recovering to a safe place in baseline, and baseline doesn't make any assumptions based on types.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c70ab56a19b
Add implicit use after folding away fallible unbox r=jandem

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8c70ab56a19b