1366096 - Javascript Hits Hard Assert in SIMD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Natalie Silvanovich]

Attachment: ff_crash.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

Loaded the attached html file in the browser


Actual results:

Firefox crashes due to hitting MOZ_ASSERT. I found this issue during security testing, and I don't think it is a security problem, but I set the security flag because I'm not 100% sure you can't do anything bad before hitting the assert.


Expected results:

The page works correctly (this is a unit test from ChakraCore).

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Nightly crash report:
https://crash-stats.mozilla.com/report/index/9424fe1b-9658-4742-9521-2d4750170519

In a debug build:
Hit MOZ_CRASH(unexpected type in visitWasmStoreGlobalVar) at c:/builds/moz2_slave/m-in-w64-d-0000000000000000000/build/src/js/src/jit/shared/CodeGenerator-shared.cpp:1622

INFO: Last good revision: e5f0088f8ca2ebd070812488f4296e81ca111ad2
INFO: First bad revision: c19c99878a6076e928690f45b37403b110cd5482
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e5f0088f8ca2ebd070812488f4296e81ca111ad2&tochange=c19c99878a6076e928690f45b37403b110cd5482

INFO: Looks like the following bug has the  changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1136226

Not sure this is s-s since we're hitting an intentional crash. Can you please take a look, Dan?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Sorry, phonebook fail. Jakob, maybe you can look :)

Comment 3

[Dan Gohman [:sunfish]]

SIMD is Nightly-only, so this doesn't affect any release branches.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Crashes for me on release builds going back to 49.

Comment 5

[Luke Wagner [:luke]]

The constructors are not exposed in 49, but it looks like we're not gating the AOT compilation.  The code will never be able run (b/c it will fail instantiation), but the crash happens during compilation.

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

I don't think there are any security implications of this crash other than the denial of service from crashing a tab. I am surprised you can get this far on a release branch, though.

Luke, I'll fix the visitWasmStoreGlobalVar() function which seems to be just an omission. Should we also try to gate SIMD asm.js compilation earlier somehow?

Comment 7

[Luke Wagner [:luke]]

I think it's probably not necessary at this point unless we think there are more of these.

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

Attachment: SIMD globals with 8x16 and 16x8 types. r=bbouvier

Add support for asm.js global variables with the following types:

    Int8x16, Bool8x16, Int16x8, Bool16x8.

We already have the needed code generation support, but tests were
missing for these types, and so their types were omitted from some
critical switches.

MozReview-Commit-ID: B4r7VofjlYL

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Bug 1366096 - SIMD globals with 8x16 and 16x8 types.

Add support for asm.js global variables with the following types:

    Int8x16, Bool8x16, Int16x8, Bool16x8.

We already have the needed code generation support, but tests were
missing for these types, and so their types were omitted from some
critical switches.

Review commit: https://reviewboard.mozilla.org/r/141208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141208/

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8869626 [details]
Bug 1366096 - SIMD globals with 8x16 and 16x8 types.

https://reviewboard.mozilla.org/r/141208/#review144864

Looks good, thank you for the patch!

::: js/src/wasm/AsmJS.cpp:7512
(Diff revision 1)
>  {
>      // Unsigned SIMD types are not allowed in function signatures.
> -    if (IsVectorObject<Int32x4>(v) || IsVectorObject<Float32x4>(v) || IsVectorObject<Bool32x4>(v))
> +    if (IsVectorObject<Int32x4>(v) || IsVectorObject<Int16x8>(v) ||  IsVectorObject<Int8x16>(v) ||
> +        IsVectorObject<Bool32x4>(v) || IsVectorObject<Bool16x8>(v) ||
> +        IsVectorObject<Bool8x16>(v) || IsVectorObject<Float32x4>(v))
>          return true;

nit: needs {} now that the if is on several lines

Comment 11

[Gerry Chang [:gchang]]

Track 54- as the volume of the crash is low and there is no security implication from the crash.

Comment 12

[Julien Cristau [:jcristau]]

Likewise for 55, I guess.

Comment 13

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8869626 [details]
Bug 1366096 - SIMD globals with 8x16 and 16x8 types.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141208/diff/1-2/

Comment 14

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8869626 [details]
Bug 1366096 - SIMD globals with 8x16 and 16x8 types.

https://reviewboard.mozilla.org/r/141208/#review145232

Added braces to multiline conditional.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96d3b13f0835
SIMD globals with 8x16 and 16x8 types. r=bbouvier

Comment 16

[Natalie Silvanovich]

I re-ran the Chakra tests, with this fix they all pass now. Yay!

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/96d3b13f0835

Comment 18

[Ryan VanderMeulen [:RyanVM]]

I don't think we're hitting this crash in the wild at all. Should we just let this ride the trains or did you still want to nominate this for uplift?

Comment 19

[Jakob Stoklund Olesen [:jolesen]]

I don't think we need to uplift this. Just let it ride the trains.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/projects/cedar/rev/96d3b13f0835dfea221d54931a943f519a34bc77
Bug 1366096 - SIMD globals with 8x16 and 16x8 types. r=bbouvier