1738676 - Differential Testing: Miscomputation regarding positive/negative 0

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

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

Steps to reproduce:

During differential fuzzing, I encountered a miscomputation regarding negative 0.
The attached sample computes different values, depending on whether ion is enabled or disable.

function main() {
    let v65;
    new Uint8Array(37367);
    const v29 = {}; 

    let v60 = 256;
    let v63 = 0;
    let v64;
    do {
        v64 = --v60;
        v65 = v64 * 0;
        const v66 = v63++;
        v29[v65] = v66;
    } while (v63 < 512);
    print(v64);  // -256 with and without ion
    print(Object.is(v65, -0)); // false with ion; true if ion disabled
}
main();

Actual results:

Running the sample with ion disabled (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 --no-ion diff.js) the computation in v65 is negative 0.

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 --gc-zeal=14 diff.js) the computation in v65 is positive 0.

Comment 1

[lukas.bernhard]

I've just been looking into the jit-spew output; here is my guess of what's happening: When the loop triggers OSR, the use of v65 in Object.is is still unknown to WarpBuilder. This "allows" lowering of v64 * 0 to a MulI LIR instruction (instead of MulD); in turn losing the sign.
Hence probably not security but I'll leave it to you.

Comment 2

[Iain Ireland [:iain]]

Took a quick look at this. It's edge case analysis again, similar to bug 1719884, but more straightforward. In NeedNegativeZeroCheck, if the only uses of a Mul are either resume points or allow-listed, then we decide that a negative zero check isn't needed. However, this means that when we hit the end of the loop and bail out (because we've never executed the print statements before), the value we recover is +0.

We've already talked about getting rid of EdgeCaseAnalysis entirely, since it's mostly (but not entirely) redundant with range analysis. Last time we decided to do the spot fix, and wait to see whether additional issues appeared. This is an additional issue, which might mean it's time to pull the trigger on removing EdgeCaseAnalysis. The downside is that we lose a few small optimizations. One alternative would be to return true in NeedNegativeZeroCheck if any consumer is a resume point. A very quick assert-based experiment shows that we would still remove a non-zero number of negative zero checks in Octane. I'm not sure whether that justifies keeping the optimization around, though.

This isn't a security problem: no delicate optimizations in Warp can depend on the sign of this zero, because if any consumers cared about the sign, we wouldn't have removed the check.

(Aside: note that we can safely lower this to a MulI so long as we don't also remove the negative zero check here.)

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1738676: Preserve negative zero checks for results used by resume points r=jandem

This makes late edgecase analysis more conservative. An assertion in MMul::analyzeEdgeCaseBackward verifies that edgecase analysis does still manage to remove some negative zero checks in Octane, so we can let edgecase analysis live for a little while longer.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dcd3a0aefcb8
Preserve negative zero checks for results used by resume points r=jandem

Comment 5

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/dcd3a0aefcb8