1094570 - crash in IonScript::unlinkFromRuntime

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Luke Wagner [:luke]]

Attachment: fix-ffi-stubs

This crash is pretty reproducible if you load Aaaaa For the Awesome and switch to another game and wait for GC.  It's a pretty bug and simple fix.  crash-stats shows that a few hundred crashes with this signature in the last several weeks, many mentioning humble bundle in the comments/URL, so it'd definitely be good to uplift.  s-s because it's a use-after-free.

Comment 1

[Luke Wagner [:luke]]

Attachment: test case (to be landed separately from patch)

Comment 2

[Luke Wagner [:luke]]

Looks like only Aurora is affected.  Aaaaaa and the testcase don't reproduce on Beta because the way AsmJSModule and IonScript were linked was different.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8517890 [details] [diff] [review]
fix-ffi-stubs

Approval Request Comment
[Feature/regressing bug #]: 1054538
[User impact if declined]: pretty easy crashes on one humble bundle game
[Describe test coverage new/current, TBPL]: m-c (soon)
[Risks and why]: this is a pretty cause/fix, so the risk is pretty low

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8517890 [details] [diff] [review]
fix-ffi-stubs

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

::: js/src/asmjs/AsmJSModule.h
@@ +1420,5 @@
>          return *(ExitDatum *)(globalData() + exitIndexToGlobalDataOffset(exitIndex));
>      }
> +    bool exitIsOptimized(unsigned exitIndex) const {
> +        MOZ_ASSERT(isFinished());
> +        ExitDatum &exitDatum = exitIndexToGlobalDatum(exitIndex);

Not sure if the assert is needed. Since it also get tested in "exitIndexToGlobalDatum"

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8517890 [details] [diff] [review]
fix-ffi-stubs

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily; patch also contains refactoring.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
Aurora

If not all supported branches, which bug introduced the flaw?
1054538

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes, patch applies cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Not likely

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8517890 [details] [diff] [review]
fix-ffi-stubs

sec-approval+, a=dveditz for aurora

Comment 7

[Luke Wagner [:luke]]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff9dd6530ea1