830943 - Incorrect behavior on emscripten "bullet" test

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

Attachment: bullet testcase

Another failure on the emscripten test suite on x86_64. Works with --no-ion or in v8, fails with ion.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1: Fix

Tricky IonMonkey bug.

Lowering allocates argument slots for calls (allocateArguments and freeArguments). This only works if MPrepareCall and MCall are property "nested" though, and due to the way tableswitch blocks are ordered it's possible to break this invariant (with inlining + nested calls).

The attached patch fixes the tableswitch block ordering and makes sure the argument allocation code works properly. The next patch will add asserts to catch similar bugs.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2: Add asserts, test case

This patch adds a (debug-only) stack of MPrepareCall instructions to Lowering, so that we can ensure allocateArguments and freeArguments properly match. The testcase asserts without the previous patch.

Comment 3

[Jan de Mooij [:jandem]]

Pretty sure this bug has been here since Ion landed on m-c.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2: Add asserts, test case

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 702786 [details] [diff] [review]
Part 1: Fix

Good catch.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 702786 [details] [diff] [review]
Part 1: Fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Extremely hard.

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?

Firefox 18, 19, 20

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

IonMonkey.

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

Backporting is easy, patch moves 2 lines of code.

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

Regressions are unlikely, usual nightly testing + fuzzing should be enough.

Comment 7

[Alon Zakai (:azakai)]

Attachment: another testcase, zlib

I found another failing testcase, same symptoms as before (fails with ion, works without it), and this patch fixes it as well. So I guess the testcase is not needed, but attaching anyhow if it's useful in some way.

Comment 8

[Jan de Mooij [:jandem]]

Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9db432271c1b

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/mozilla-central/rev/9db432271c1b

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 702786 [details] [diff] [review]
Part 1: Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): IonMonkey
User impact if declined: security bugs, crashes, correctness issues
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: None

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 702794 [details] [diff] [review]
Part 2: Add asserts, test case

decoder, gary: can you guys give this patch (part 2) some fuzzing? It should be applied to m-c or m-i tip. I will land it when part 1 has made its way to mozilla-release, but it would be good to give this patch some fuzzing to find similar bugs before that happens.

Comment 12

[Christian Holler (:decoder)]

Fuzzing on langfuzz1 now :)

Comment 13

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

Comment on attachment 702794 [details] [diff] [review]
Part 2: Add asserts, test case

(In reply to Jan de Mooij [:jandem] from comment #11)
> Comment on attachment 702794 [details] [diff] [review]
> Part 2: Add asserts, test case

I didn't hit anything particularly bad after some overnight fuzzing -> feedback+

Comment 14

[Christian Holler (:decoder)]

Comment on attachment 702794 [details] [diff] [review]
Part 2: Add asserts, test case

feedback+, no issues found till now :)

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] from comment #13)
> I didn't hit anything particularly bad after some overnight fuzzing ->
> feedback+

(In reply to Christian Holler (:decoder) from comment #14)
> feedback+, no issues found till now :)

Thanks! Then I will just push this patch next month :)

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 702786 [details] [diff] [review]
Part 1: Fix

Very low risk fix for a tracked sg:crit issue. Let's land on branches.

Comment 17

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6624fe215f6d
https://hg.mozilla.org/releases/mozilla-beta/rev/608e239e0e14

Comment 18

[Jan de Mooij [:jandem]]

Pushed part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/64bdaa6a86cd

(Linux Try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=bdd7c595daf2)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64bdaa6a86cd