1645795 - NaN canonicalization and register spilling results in value truncation

Status: RESOLVED DUPLICATE of bug 1928622

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Carl Smith]

Consider the following script:

function foo() {
  const arr = new Float32Array(10);
  let loop = 0;
  for (; loop <= 100; loop = loop || 9007199254740992) {
  }
  var something = {};
  arr[0]
}

function main() {
  for(var idx = 0; idx < 0x100; idx++) {
    foo()
  }
}
main();

When run with the following command line flags and built with JS_MORE_DETERMINISTIC it triggers an assertion failure:

--no-threads --cpu-count=1 --ion-offthread-compile=off --baseline-warmup-threshold=10 --ion-warmup-threshold=100  --ion-check-range-analysis

The assertion that is triggered is:

Assertion failure: Double input should be equal or higher than Lowerbound.

The lower bound in question is the loop exit condition, this is 100 in the sample, whereas the value observed at runtime is (incorrectly) 0. Range analysis correctly types the loop variable to [100, ?] after the loop is exited, but it is in fact 0 at the time of the AssertRangeF LIR operation. The reason seems to be related to the JS_MORE_DETERMINISTIC build flag. If enabled, non-canonical NaNs are canonicalized when the compiler spills the registers to memory.

What happens in the above sample is that 9007199254740992 is 0x5a000000 as Float32. During Register spilling the higher dword of the xmm register combined with 0x5a000000 in the lower dword is considered a non-canonical NaN which then gets converted to a canonical NaN, which therefore truncates the lower dword to 0. After the registers get repopulated it triggers the assertion during the AssertRangeF check.

As this is apparently only triggered when compiled with JS_MORE_DETERMINISTIC, it likely has no security impact in default builds, but I’m reporting it as a security issue as a precaution. I also have not managed to reuse the register and observe the 0 during runtime and I am not sure if this is possible. If it is possible this could lead to a wrongly typed range which might have more severe security implications.

Comment 1

[Jan de Mooij [:jandem]]

Thanks for the report.

It doesn't repro for me on Linux64 when I build with --enable-debug --disable-optimize --enable-more-deterministic and use the shell flags in comment 0. Which branch/revision are you on?

Comment 2

[Carl Smith]

For me it works on the current release tag (commit 919cae7500c47d6fa4e515a32ad98a07b649c00d) from https://github.com/mozilla/gecko-dev.

Comment 3

[Jan de Mooij [:jandem]]

Ah yeah it reproduces for me on release. The culprit is the storeDouble in DumpAllRegs, but if ENABLE_WASM_SIMD is defined we use a different code path that stores the register without canonicalizing.

I think storeDouble doing NaN-canonicalization by default for every store is weird, maybe we should split it in two functions and require the caller to pick one based on whether they want canonicalization in more-deterministic builds. We could keep this bug open for that but since it requires --enable-more-deterministic I'll open this up.

Comment 4

[Jan de Mooij [:jandem]]

Fixed by bug 1928622.