953270 - Assertion failure: JSVAL_IS_DOUBLE_IMPL(l), at js/Value.h:406 or Assertion failure: l.asBits <= JSVAL_SHIFTED_TAG_MAX_DOUBLE, at js/Value.h:636 or Crash [@ JSObject::getGeneric] or Crash [@ getClass] with SIMD

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 451f47a70238 (run with --fuzzing-safe):


var int32x4 = SIMD.int32x4;
var a = int32x4((4294967295), 200, 300, 400);
var c = SIMD.int32x4.bitsToFloat32x4(a);
assertEq(c.x, 1.401298464324817e-43);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

The crash address is partially controllable, here's a 64 bit trace:


Program received signal SIGSEGV, Segmentation fault.
getClass (this=0x7fffe0000000) at js/src/gc/Barrier.cpp:60
60      } // namespace js
#0  getClass (this=0x7fffe0000000) at js/src/gc/Barrier.cpp:60
#1  getOps (this=0x7fffe0000000) at js/src/vm/ObjectImpl.h:1286
#2  JSObject::getGeneric (cx=0x13c08f0, obj=..., receiver=..., id=..., vp=...) at js/src/jsobj.h:1002
#3  0x00000000006e0f6a in getProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x13c08f0) at js/src/jsobj.h:1026
#4  js::ValueToSource (cx=0x13c08f0, v=...) at js/src/jsstr.cpp:4059
#5  0x00000000006539d0 in JS_ValueToSource (cx=<optimized out>, valueArg=...) at js/src/jsapi.cpp:419
#6  0x000000000040cdac in ToSource (bytes=<synthetic pointer>, vp=..., cx=0x13c08f0) at js/src/shell/js.cpp:1370
#7  AssertEq (cx=0x13c08f0, argc=2, vp=0x1461520) at js/src/shell/js.cpp:1400
rax     0xe0000000      140736951484416
=> 0x47bd57 <JSObject::getGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)+7>: mov    0x8(%rax),%rax


The 0xe0 byte in 0x7fffe0000000 is controllable by the first argument of int32x4. It might be possible to control it even further. I assume there's some integer overflow/truncation going on somewhere. If one manages to control this address, one controls an object, so I assume sec-critical.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, failed due to error (try manually).

Comment 4

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

No idea why this failed, but here's the bisection range:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/abdeb55e8a16
user:        Ivan Jibaja
date:        Wed Dec 18 16:28:32 2013 -0500
summary:     Bug 946042 - Add all SIMD functions to the interpreter. r=till

Comment 5

[Christian Holler (:decoder)]

Also needinfo from Till to get this bug going :)

Comment 6

[Christian Holler (:decoder)]

Till, can you also answer if this feature is enabled in the browser nightly builds already now? I can see a lot of changes have been made to code that are not ifdef'ed but some stuff is behind an ifdef (for which I don't know if it's enabled in Nightly or not). Thanks!

Comment 7

[Till Schneidereit [:till]]

Forwarding to Niko, as he knows way more about this than I do.

(In reply to Christian Holler (:decoder) from comment #6)
> Till, can you also answer if this feature is enabled in the browser nightly
> builds already now? I can see a lot of changes have been made to code that
> are not ifdef'ed but some stuff is behind an ifdef (for which I don't know
> if it's enabled in Nightly or not). Thanks!

SIMD is enabled in Nightlies, yes. So having this marked as sec-critical until proven to be harmless makes sense.

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug953270.diff

Canonicalize NaN as part of the lane accessor.

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8357050 [details] [diff] [review]
Bug953270.diff

Ideally, this would actually be a patch ;)

Comment 10

[Niko Matsakis [:nmatsakis]]

Attachment: Bug953270.diff

This time with actual patch.

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8359305 [details] [diff] [review]
Bug953270.diff

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

r=me with the builtin/SIMD.h include reverted to its old position, and the commit message's format fixed.

::: js/src/builtin/SIMD.cpp
@@ +14,4 @@
>  #include "jsapi.h"
>  #include "jsfriendapi.h"
> +
> +#include "builtin/SIMD.h"

The cpp file's own header is always first, to prevent it from accidentally depending on not-included other headers.

Comment 12

[Niko Matsakis [:nmatsakis]]

> The cpp file's own header is always first, to prevent it from accidentally
> depending on not-included other headers.

Oh, right, forgot. Here I was trying to be a good boy and enforcing SM rules. ;)

Comment 13

[Niko Matsakis [:nmatsakis]]

https://tbpl.mozilla.org/?tree=Try&rev=e04d57969bfe

Comment 14

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d982bf3158d

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4d982bf3158d

Comment 16

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.