906286 - Canonicalize NaN values stored to float arrays in JS_MORE_DETERMINISTIC builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

try {
    x = [];
    x[7] = 1
    Array.prototype.push.call(x, (new Set))
    a2 = x.concat(0)
    a = new ArrayBuffer(4);
    b = new Float32Array(a);
    c = new Uint8Array(a);
    Array.prototype.sort.call(a2, (function() {
        b[0] = 0 / 0;
        c[2] = 2;
        print('FOO');
        return b[0]
    }))
} catch (e) {}


shows three instances of "FOO" on a 64-bit debug deterministic threadsafe js shell on m-c rev 8ad1e4c838c8 with --ion-eager or without any CLI arguments, but shows only two instances of "FOO" with only --baseline-eager.

Setting needinfo for jandem as previously requested. (Pass in the testcase as a CLI argument to reproduce)

Comment 1

[Jan de Mooij [:jandem]]

Hm this one is complicated. It looks like the problem is the 0 / 0... When this runs in the VM, NumberDiv returns js_NaN (0x7ff8000000000000). When the baseline JIT adds an IC stub for it (just a divsd instruction on x86), we also get a NaN value but it has the sign bit set: 0xfff8000000000000

The difference is observable if the value is stored to a Float32Array and some of its bytes are modified (the Uint8Array here)...

Comment 2

[Jan de Mooij [:jandem]]

Consider this testcase:
---
function f() {
    var a = new ArrayBuffer(4);
    var b = new Float32Array(a);
    var c = new Uint8Array(a);
    for (var i=0; i<2; i++)
	b[0] = 0 / 0;
    print(c[3].toString(16));
}
f();
---
Output:

js (interpreter)    : 7f
js --baseline-eager : ff
d8                  : ff
Safari              : ff
Opera 12.16         : 7f

If I use NaN instead of 0 / 0, I get 7f everywhere except d8. Probably because they self-host their NaN definition as 0 / 0...

I'm not sure how to fix this. I have to check the spec but I assume the behavior is not specified.. It's pretty gross, but maybe we could canonicalize the result of divsd in more-determinstic builds to work around this..

Comment 3

[Luke Wagner [:luke]]

Don't worry, this type of NaN-representation-variation-as-observed-by-typed-arrays is allowed by the spec.  Originally, the issue came up because the NaN canonicalization required by NaN-boxing was observable, but now it seems you've found a case which doesn't even involve that.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #3)
> Don't worry, this type of
> NaN-representation-variation-as-observed-by-typed-arrays is allowed by the
> spec.

OK thanks, then we just need to find a workaround for the fuzzers..

Gary / Jesse, we could try to exclude tests that use Float32Array or Float64Array for differential testing. I can also make the engine print to stderr if we create a Float32Array or Float64Array in more-deterministic builds, let me know if I should do that.

It's a bit unfortunate to filter out these arrays completely for differential testing, but I'm afraid if we don't do that it will be a whack-a-mole game to find all places where we create different NaN values.

Any thoughts?

Comment 5

[Dave Herman [:dherman]]

> OK thanks, then we just need to find a workaround for the fuzzers..
> 
> Gary / Jesse, we could try to exclude tests that use Float32Array or
> Float64Array for differential testing. I can also make the engine print to
> stderr if we create a Float32Array or Float64Array in more-deterministic
> builds, let me know if I should do that.

Without specific knowledge of how the fuzzer works, I'd think it could have a notion of an "any NaN" predicate, as distinct from "exactly the same NaN." These are both very easy to define precisely, and you can build general-purpose JS value comparison predicates out of either.

If you're only talking about differential testing that may be ok, but you *definitely* want to be carefully testing NaN in the fuzzers. That's one of those smelly corner cases that you can easily imagine becoming part of an exploit.

> It's a bit unfortunate to filter out these arrays completely for
> differential testing, but I'm afraid if we don't do that it will be a
> whack-a-mole game to find all places where we create different NaN values.
> 
> Any thoughts?

I'm not sure there's *that* many places that produce NaN values. Off the top of my head, I'd guess semantically the only places that can produce a new NaN value are:

- arithmetic operators
- Math functions
- coercions to number
- typed arrays

Of course in the implementation you have to deal with these in each of the engines. But at least in theory it'd be a nice whole-system property to have a solid understanding of.

Dave

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Dave Herman [:dherman] from comment #5)
> Without specific knowledge of how the fuzzer works, I'd think it could have
> a notion of an "any NaN" predicate, as distinct from "exactly the same NaN."
> These are both very easy to define precisely, and you can build
> general-purpose JS value comparison predicates out of either.

The fuzzer generates a random JS script, runs it twice (with different shell flags) and compares the output. If there's a difference, we know there must be a bug in the JIT or somewhere else.

The problem is that if a JIT generates a different NaN value, and we store it to a float array and create, say, an Int8Array with the same ArrayBuffer, it's easy to generate a random program that prints different values depending on the integer value we read back. For the fuzzer it's impossible to know this is what happened, based on the output.

Another example: to negate a double, both C++ and JIT compilers typically do a XOR to flip the sign bit, even for NaN values.. However, what if a JIT does "double * -1" instead of "-double"? That will likely not use a XOR and hence not flip the sign bit. Like this:

function f() {
    var a = new ArrayBuffer(4);
    var b = new Float32Array(a);
    var c = new Uint8Array(a);
    for (var i=0; i<5000; i++)
	b[0] = -NaN;
    print(c[3].toString(16));
}
f();

Ion:    7f (Ion does -1 * NaN, sign bit not set)
No-Ion: ff (interpreter XORs the sign bit)

Comment 7

[Jan de Mooij [:jandem]]

What we could do is this: in JS_MORE_DETERMINISTIC builds, whenever the VM or a JIT writes to a float typed array, we check whether the value is NaN and *always* write a NaN-value with the sign bit not set. That shouldn't require too many changes I think.

(JS_MORE_DETERMINISTIC builds are special builds used for differential testing, so the performance hit there doesn't really matter.)

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

In JS_MORE_DETERMINISTIC builds, canonicalize NaN values stored to typed arrays.

Turned out to be pretty straight-forward and fixes the 3 tests in this bug. Not adding a testcase though because this is JS_MORE_DETERMINISTIC-builds only.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 792722 [details] [diff] [review]
Patch

nice

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Since this is JS_MORE_DETERMINISTIC-only, setting checkin-needed in the hopes that this will make it into m-c by tomorrow, for me to start more differential testing runs.

Comment 11

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78e4439bad2d

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78e4439bad2d

Comment 13

[Jesse Ruderman]

\o/