863261 - IonMonkey: Assertion failure: ins->type() == MIRType_Value, at ion/IonBuilder.cpp:3099

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 3ada6a2fd0c6 (run with --ion-eager):


function TestCase(d) {
  toPrinted(d) 
}
function toPrinted(value) {}
function reportCompare (expected, actual, description) {
  if (typeof description == "undefined")
    toPrinted(expected);
  new TestCase(description);
  reportCompare();
}
reportCompare(Math['LN2'], Math['LN2'], 0);

Comment 1

[Christian Holler (:decoder)]

Not sure if this needs s-s but since previous similar asserts we're s-s, let's keep this one hidden until triaged. Requesting a bisect although I probably know already who's the right person for this ;)

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Always box unknown inputs (even when not adding type barrier)

It depends on bug 796114 or bug 862100, i.e. FF22 is or will get affected.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   122585:437c955ff06d
user:        Nicolas B. Pierron
date:        Wed Jan 30 07:41:01 2013 -0800
summary:     Bug 796114 - Inline with type-checked arguments. r=h4writer

This iteration took 12.061 seconds to run.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 739062 [details] [diff] [review]
Always box unknown inputs (even when not adding type barrier)

So I learned I have to ask sec approval.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
A decent amount of knowledge about IM and inlining is needed and prior information on how to achieve this. Also it is harder in normal situation than our test environment.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, I'll land the testcase after landing on aurora. FYI the commit line will be:
Bug 863261: IonMonkey: Box unknown types when inlining, r=nbp

Which older supported branches are affected by this flaw?
aurora only

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

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

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

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 739062 [details] [diff] [review]
Always box unknown inputs (even when not adding type barrier)

sec-approval+. 

Please nominate this patch (or Aurora specific one) for Aurora. Release management may want a security rating on it to judge severity but I expect we'll take it.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 739062 [details] [diff] [review]
Always box unknown inputs (even when not adding type barrier)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 796114

User impact if declined:
Possible crashes during compilation on special crafted scripts. That's the reason it took so long to find this issue.

Testing completed (on m-c, etc.): 
Tested jit-tests. Can't test/land on m-c. Since today that code path is gone. Removed by a big improvement.

Risk to taking this patch (and alternatives if risky): 
Very low. It moves some code a little bit higher, so the reason why this code is added also applies to the code in between.

String or IDL/UUID changes made by this patch:
/

Comment 8

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 8b1a7228674a).

Comment 9

[Alex Keybl [:akeybl]]

We're assuming this is at least sec-high given the fact that this went through sec-approval. We'll approve this low risk regression fix.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 739062 [details] [diff] [review]
Always box unknown inputs (even when not adding type barrier)

Please resolve this as fixed and change the target milestone to 22 once this has landed.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

(In reply to Alex Keybl [:akeybl] from comment #9)
> We're assuming this is at least sec-high given the fact that this went
> through sec-approval. We'll approve this low risk regression fix.

To be clear, I didn't assume that. It is just that people have to ask for sec-high and sec-critical. If they ask for an unknown and it seems safe and appropriate, I'll approve but I do realize we want ratings for decisions on branches as well.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4cf75b25cdc3

Same question as bug 863755. Our normal process it to not uplift to the branches until after the patch reaches m-c. Why was this procedure not followed here?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Explained in bug 863755, thanks!

Comment 14

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 15

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9063e9ebc320

Comment 16

[u279076]

Marking status-firefox23:verified based on comment 14.