1761947 - Differential testing: miscompute when GC during scalar-replaced applyArray

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Steps to reproduce:

During differential fuzzing, I encountered a miscomputation. The attached sample computes different values, depending on whether ion is enabled or not. Reproduces on git commit: 6f18c1b2e02a0293f31d9c068c03d578d2602810

Due to the unusual circumstances to trigger the miscomputation I'm flagging this bug as s-s, just derestrict as applicable. The bug itself is quite volatile, it manifests only if a GC happens during OSR.

The command line which triggers the bug on my machine is:

obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --no-threads --cpu-count=1 -ion-offthread-compile=off --fuzzing-safe --differential-testing --ion-scalar-replacement=on --gc-zeal=14,29 sample.js

However, the value 29 (parameter to gc-zeal) might need some adaption. I found it easiest (during manual reduring of the fuzzer output) to run the sample in a loop until the miscompute prints [object Object] instead of 8.
Disabling scalar replacement via --ion-scalar-replacement=on prevents the bug from manifesting.

sample.js:

function main() {
  let v12 = 0;
  let v25 = undefined;
  let v26 = undefined;

  do {
      v12++;
      const v21 = [8];
      for (let v23 = 0; v23 < 4; v23++) {
          v25 = Object(...v21);
      }
  } while (v12 < 8);

  print(v25); // should be 8. is [object Object] if gc triggers during OSR
}

main();

loop.sh:

for i in {1..100}
do
   echo $i;
   obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --no-threads --cpu-count=1 --ion-offthread-compile=off --fuzzing-safe --differential-testing --ion-scalar-replacement=on --ion-osr=on --nursery-strings=off --ion-instruction-reordering=off --gc-zeal=14,$i diff.js;
done

.mozconfig (just in case)

CC=clang
CXX=clang++
ac_add_options --enable-application=js

ac_add_options --enable-js-fuzzilli
ac_add_options --enable-fuzzing
ac_add_options --disable-tests

ac_add_options --disable-shared-js
ac_add_options --enable-linker=lld
ac_add_options --enable-gczeal
ac_add_options --enable-optimize
ac_add_options --enable-debug

Comment 1

[Daniel Veditz [:dveditz]]

guessing sec-high because at a quick triage glance this smells like type confusion, but it might be more benign.

Comment 2

[Iain Ireland [:iain]]

Looked into this, and it turns out that it isn't security sensitive.

The problem is in scalar replacement code added in bug 1739650 to turn function.apply into a regular call when the argument array doesn't escape. Due to a quirk of how the iterator walks over the instructions, we don't update the resume point correctly. If the call triggers a compacting GC, and we throw away the jit code, then we recover an out-of-date version of the replaced array ([] instead of [8] in this case), and get incorrect results in the baseline interpreter. The value that we recover is stale, but valid, so there's no security risk.

OSR was a red herring here.

Here's a testcase that doesn't rely on gczeal:

// |jit-test| --fast-warmup; --no-threads; --ion-osr=off

let trigger = false;

function bar(x) {
  with ({}) {}
  if (trigger) {
    gc(foo, "shrinking");
    trigger = false;
  }
  return Object(x);
}

function foo() {
  let result = undefined;
  const arr = [8];
  for (var i = 0; i < 10; i++) {
    result = bar(...arr);
    assertEq(Number(result), 8);
  }
  return result;
}

with ({}) {}
for (var i = 0; i < 100; i++) {
  foo();
}

trigger = true;
foo();

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1761947: Visit stolen resume points in MNodeIterator r=jandem

EmulateStateOf<T>::run uses MNodeIterator to iterate over the instructions in a block. MNodeIterator has some internal cleverness to visit the resume point attached to an instruction, unless that instruction has been discarded. In visitApplyArray, we steal the resume point from the applyArray, attach it to a new call, and discard the applyArray. Because the applyArray was discarded, MNodeIterator skips the resume point. We don't update the list of stores, so the recovery code allocates an empty array instead of an initialized array, causing us to get the wrong answer if we invalidate due to GC inside the replaced spread call.

This patch changes MNodeIterator to store the resume point instead of the instruction, which simplifies the code somewhat and seems more robust.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1739650

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1739650

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1492888ff41
Visit stolen resume points in MNodeIterator r=jandem

Comment 7

[Cristina Cozmuta (:CrissCozmuta)]

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