1122338 - Differential Testing: Different output message involving asm.js and SharedArrayBuffer

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

g = (function(stdlib, n, heap) {
    "use asm";
    var Float32ArrayView = new stdlib.Float32Array(heap);
    function f() {
        return +Float32ArrayView[0]
    }
    return f
})(this, {}, new SharedArrayBuffer(4096));
print(g())

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267 --fuzzing-safe --no-threads --ion-eager a1cjs.js
0

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267 --fuzzing-safe --no-threads --ion-eager --no-asmjs a1cjs.js
NaN

Tested this on m-c rev 89a190592267.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect is running.

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/cf9ed5c35329
user:        Lars T Hansen
date:        Tue Sep 16 18:45:31 2014 +0200
summary:     Bug 1054882 - Rollup patch.  r=till, r=sstangl, r=jorendorff

Lars, is bug 1054882 a likely regressor?

Comment 2

[Lars T Hansen [:lth]]

That patch would have changed this behavior, yes.  It is not legal to construct a non-shared view onto a shared buffer like the test case does, but before I landed that patch it was legal.  The behavior changed intentionally.

However, It's extremely distressing to me that an exception is not thrown here, so I'll investigate that.

Comment 3

[Lars T Hansen [:lth]]

The reason the view constructor is not throwing is arguably an oversight on my part: when a TypedArray constructor is presented with a SharedArrayBuffer object, it will not recognize it as an illegal case but treat it as the generic Array-like case, ie, it will look for a length property (which is not there, so it is taken to be zero), and create a new Float32Array on a new ArrayBuffer of the computed length (ie, zero).  I'll look into fixing that: bug 1122467.

As for the 0 vs NaN thing...  Since the length of Float32ArrayView within f is zero, then Float32ArrayView[0] reads as undefined (in plain JS in Spidermonkey at present), and +undefined is NaN.

The latest ES6 spec says that the correct value to read for an out-of-range access is indeed undefined (9.4.5.8, IntegerIndexedElementGet).

The implication at hand is that OdinMonkey gets the out-of-range read wrong.  Consider the following test case:

g = (function(stdlib, n, heap) {
    "use asm";
    var Float32ArrayView = new stdlib.Float32Array(heap);
    function f() {
        return +Float32ArrayView[1024]
    }
    return f
})(this, {}, new ArrayBuffer(4096));
print(g())

Here the printed value is NaN both in asm.js mode and in --no-asmjs mode, even though it is again an out of bounds access.  So something is special about the original test case.

It may actually be that what happens is that the length that asm.js uses for its bounds checking is derived from the heap, not from the view; in that case the construction of the view violates a fundamental invariant.  Marking bug s-s: an attacker can read and write raw memory by allocating a large SharedArrayBuffer, mapping it (wrongly) to a TypedArray, and then using out-of-bounds reads and writes on the TypedArray.  Nightly-only, though, so no cause for alarm.

Comment 4

[Lars T Hansen [:lth]]

Another (or a complementary) interpretation is that Odin, since it is optimizing range checks, needs to test after creating the views that their lengths correspond to the length that will be used for range checking.

Comment 5

[Lars T Hansen [:lth]]

The analysis in bug 1122467 suggests that the behavior of the Float32Array constructor should not change; as a consequence there needs to be some fix on the Odin side to guarantee that its assumptions about the view lengths are not wrong.

Comment 6

[Luke Wagner [:luke]]

Odin never creates views, they just exist in theory.  What I would have expected to happen in the given test case is a link-time error (just as if you had passed any other non-SharedArrayBuffer value).

As for the semantics of constructing shared views on non-shared buffers and vice versa: could we specify both of these specific cases to just throw instead of falling back to the generic-construct-a-new-view paths?

Comment 7

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #6)
> Odin never creates views, they just exist in theory.  What I would have
> expected to happen in the given test case is a link-time error (just as if
> you had passed any other non-SharedArrayBuffer value).

Then the mystery remains, I guess.

> As for the semantics of constructing shared views on non-shared buffers and
> vice versa: could we specify both of these specific cases to just throw
> instead of falling back to the generic-construct-a-new-view paths?

We can try, but we can't count on it.  After all TypedArray is the domain of TC39 at this point and TC39 is maximally lenient here.  Also, there's some utility to being able to create a non-shared view on a shared buffer and having the meaning be "make a local copy", even if the possibility of a race makes the utility questionable.

Comment 8

[Lars T Hansen [:lth]]

As Luke says, there's no out-of-bounds read here, it's just a failure to catch an error case in the linker.  We can remove s-s status.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1122338-mismatch.diff

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8551191 [details] [diff] [review]
bug1122338-mismatch.diff

Review of attachment 8551191 [details] [diff] [review]:
-----------------------------------------------------------------

For more recent constructs, it seems like TC39 is erring more on the side of "throw if things don't match", so I'd have hope.  The "construct a non-shared copy of a shared buffer" is a good use case, but if the non-shared case aliases, it seems weird to have the same constructor form alias in one case and not in the other.  I think .set could be used instead for a high-performance copy.

::: js/src/asmjs/AsmJSLink.cpp
@@ +538,5 @@
>  
>      Rooted<ArrayBufferObjectMaybeShared *> heap(cx);
>      if (module.hasArrayView()) {
> +        bool sab = false;
> +        if (IsArrayBuffer(bufferVal) || (sab = IsSharedArrayBuffer(bufferVal))) {

It seems like the logic could be flattened and reduced a bit:
  if (module.hasArrayView()) {
      if (module.isSharedView() && !IsSharedArrayBuffer(bufferVal))
          ...
      if (!module.isSharedView() && !IsArrayBuffer(bufferVal))
          ...
      heap = &AsAnyArrayBuffer(bufferVal);

Comment 11

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98a29b5792ef

Pushed sans test case.  Gary, do you take care of that or should I look into it?  If you run the original in the shell with the -w switch you will now see an asm.js error saying that there's a link error (and so the code runs as plain JS).

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98a29b5792ef

Comment 13

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

I verify this is fixed:

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-a6bbabebed2f --fuzzing-safe --no-threads --ion-eager --no-asmjs 1122338.js
NaN

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-a6bbabebed2f --fuzzing-safe --no-threads --ion-eager 1122338.js
NaN

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-a6bbabebed2f --fuzzing-safe --no-threads --ion-eager -w 1122338.js 1122338.js:warning: Successfully compiled asm.js code ()
1122338.js:1:5 warning: asm.js link error: unshared views can only be constructed onto ArrayBuffer
NaN

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-a6bbabebed2f --fuzzing-safe --no-threads --ion-eager --no-asmjs -w 1122338.js
1122338.js:warning: asm.js type error: Disabled by javascript.options.asmjs in about:config
NaN


Please feel free to land the testcase.

Comment 14

[Lars T Hansen [:lth]]

Test case, with the vice versa case also:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0db598f71a5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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