Closed Bug 1738676 Opened 7 months ago Closed 6 months ago

Differential Testing: Miscomputation regarding positive/negative 0

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox96 --- fixed

People

(Reporter: lukas.bernhard, Assigned: iain)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.

Group: firefox-core-security → core-security
Component: Untriaged → JavaScript Engine: JIT
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64
Group: core-security → javascript-core-security

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.

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.)

Group: javascript-core-security
Blocks: sm-jits
Severity: -- → S4
Priority: -- → P2

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.

Assignee: nobody → iireland
Status: NEW → ASSIGNED
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dcd3a0aefcb8
Preserve negative zero checks for results used by resume points r=jandem
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch
You need to log in before you can comment on or make changes to this bug.