Closed Bug 830943 Opened 8 years ago Closed 8 years ago

Incorrect behavior on emscripten "bullet" test

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 + fixed
firefox20 + fixed
firefox21 + fixed
firefox-esr17 --- unaffected
b2g18 --- disabled

People

(Reporter: azakai, Assigned: jandem)

Details

(Keywords: sec-critical, Whiteboard: [adv-main19+])

Attachments

(4 files, 1 obsolete file)

Attached file bullet testcase
Another failure on the emscripten test suite on x86_64. Works with --no-ion or in v8, fails with ion.
Assignee: general → jdemooij
Group: core-security
Attached patch Part 1: FixSplinter Review
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.
Attachment #702786 - Flags: review?(dvander)
Attached patch Part 2: Add asserts, test case (obsolete) — Splinter Review
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.
Attachment #702788 - Flags: review?(dvander)
Pretty sure this bug has been here since Ion landed on m-c.
Status: NEW → ASSIGNED
Keywords: sec-critical
Attachment #702788 - Attachment is obsolete: true
Attachment #702788 - Flags: review?(dvander)
Attachment #702794 - Flags: review?(dvander)
Comment on attachment 702786 [details] [diff] [review]
Part 1: Fix

Good catch.
Attachment #702786 - Flags: review?(dvander) → review+
Attachment #702794 - Flags: review?(dvander) → review+
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.
Attachment #702786 - Flags: sec-approval?
Attached file 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.
Attachment #702786 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/9db432271c1b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
Attachment #702786 - Flags: approval-mozilla-beta?
Attachment #702786 - Flags: approval-mozilla-aurora?
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.
Attachment #702794 - Flags: feedback?(gary)
Attachment #702794 - Flags: feedback?(choller)
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+
Attachment #702794 - Flags: feedback?(gary) → feedback+
Comment on attachment 702794 [details] [diff] [review]
Part 2: Add asserts, test case

feedback+, no issues found till now :)
Attachment #702794 - Flags: feedback?(choller) → feedback+
(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.
Attachment #702786 - Flags: approval-mozilla-beta?
Attachment #702786 - Flags: approval-mozilla-beta+
Attachment #702786 - Flags: approval-mozilla-aurora?
Attachment #702786 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main19+]
https://hg.mozilla.org/mozilla-central/rev/64bdaa6a86cd
Flags: in-testsuite+
Target Milestone: --- → mozilla21
Group: core-security
You need to log in before you can comment on or make changes to this bug.