1720032 - Differential testing: live statement gets DCE'ed when optimizing MCheckReturn

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 triggers a differential execution when invokes such as:

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

function main() {
    class V14 extends Object {
        constructor() {
            super();
            let v57 = 0xffff;
            try {
                v57 &= 0xff; // this statement gets DCE'ed
                return 0;
            }
            catch(v62) { }

            print(v57 + 1); // should be 256 but becomes 65536
            for (let v72=0; v72<90; v72++) {}
        }
    };

    for (let i=0; i<15; i++) {
        const v97 = new V14();
    }
}
main();

Bisecting the issue identifies the first bad commit as 1919e45ad4279c6613071fa20d6688f784f69dc0
=> see bug 1617196

Comment 1

[André Bargull [:anba]]

Looks like the various MCheck instructions are simply missing AliasSet::ExceptionState.

A similar issue can be reproduced for MCheckThis when moving the super() call after the try-catch statement and then changing return 0 to this.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1720032 - Part 1: Mark Check{Return,This,ThisReinit} as throwing instructions. r=jandem!

Check{Return,This} can throw an exception and therefore must record this in
their alias set.

CheckThisReinit doesn't strictly require this change, because this instruction
is currently only used in situations where a resume point is constructed anway,
but it seemed more consistent to handle it the same way as Check{Return,This}.

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1720032 - Part 2: Use exception-state alias set for MCheckClassHeritage. r=jandem!

Let's change MCheckClassHeritage to use the same alias-set as the other
MCheck instructions for more consistency.

Depends on D119630

Comment 4

[lukas.bernhard]

Fuzzing just identified a testcase which I believe is quite similar to the testcase already posted. However, this instance reproduces even with the patches currently under review:

function main() {
  let v28;
  for (let v13 = 0; v13 < 4; v13++) {
      let v25 = 128n;
      try {
          do {
              v28 = [0, 0]; 
              const v29 = ++v28;
              v25 **= 65536n; // eventually throws RangeError: BigInt is too large to allocate
          } while (0 < 1); 
      } catch(v31) {
      }   
  
      for (let v46 = 0; v46 < 100; v46++) {}
  }
  print(v28); // v28 is NaN with --no-ion but [0, 0] with ion
}
main();

If I'm mistaken and this should be filed as a separate bug just let me know.

Comment 5

[André Bargull [:anba]]

BigInt operations were modelled after MConcat (string concatenation), which exhibits the same problem (see below). We don't want to change the alias-set of these operations because of performance reasons (see bug 1365782). We could try to give them an explicit resume point, so they will create a separate safepoint instead of reusing the previous one.

Here's a modified test case which shows the same behaviour when MConcat is used:

function main() {
  let v28;
  for (let v13 = 0; v13 < 4; v13++) {
      let v25 = "a".repeat(1<<20);
      let v1 = "a".repeat(1<<15);
      try {
          do {
              v28 = [0, 0]; 
              const v29 = ++v28;
              v25 += v1;
          } while (0 < 1); 
      } catch(v31) {
      }   
  
      for (let v46 = 0; v46 < 100; v46++) {}
  }
  print(v28); // v28 is NaN with --no-ion but [0, 0] with ion
}

main();

Comment 6

[Daniel Veditz [:dveditz]]

Is this likely to be exploitable, or is it just producing incorrect results?

Comment 7

[André Bargull [:anba]]

I don't think this is exploitable. In bug 1365782 we've openly discussed a similar issue and Ted even described the same bug as comment #5 (i.e. a stale value can be observed). Bug 1354275 and bug 1401014 are other similar cases where we've missed to create a resume point, they also weren't considered exploitable.

Comment 8

[André Bargull [:anba]]

(In reply to lukas.bernhard from comment #4)

If I'm mistaken and this should be filed as a separate bug just let me know.

Let's track this under a separate bug, because it also affects MConcat where we have to be more cautious not to cause performance regressions.

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1720032 - Part 3: Mark HomeObjectSuperBase as a throwing instruction. r=jandem!

Similar to part 1, also mark MHomeObjectSuperBase as a throwing instruction.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b8899b814dc
Part 1: Mark Check{Return,This,ThisReinit} as throwing instructions. r=jandem

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/3b8899b814dc