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

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.

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.

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

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

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.

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.

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

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 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.

Fuzzing on langfuzz1 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 on attachment 702794 [details] [diff] [review] Part 2: Add asserts, test case feedback+, no issues found till now :)

(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 on attachment 702786 [details] [diff] [review] Part 1: Fix Very low risk fix for a tracked sg:crit issue. Let's land on branches.

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

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)