IonMonkey: Assertion failure: ins->type() == MIRType_Value, at ion/MIR.h:1833

VERIFIED FIXED in Firefox 22

Status

()

--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: decoder, Assigned: h4writer)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla23
x86
Linux
assertion, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 unaffected, firefox22 fixed, firefox23 verified, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [jsbugmon:update][adv-main22-])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
The following testcase asserts on mozilla-central revision 261d6997d1d1 (run with --ion-eager):


function TestCase(n, d, e, a) {}
function reportCompare (expected, actual, description) {
  new TestCase("", description, expected, actual);
}
new TestCase( "", "", 0, Number(new Number()) );
reportCompare(true, true);
evaluate("\
function TestCase(n, d, e, a) {}\
test_negation(-2147483648, 2147483648);\
test_negation(2147483647, -2147483647);\
function test_negation(value, expected)\
    reportCompare(expected, '', '-(' + value + ') == ' + expected);\
");
(Reporter)

Comment 1

6 years ago
S-s because previous asserts like this we're s-s sometimes.
Blocks: 724444
Whiteboard: [jsbugmon:update,bisect]
(Reporter)

Updated

6 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 2

6 years ago
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 0.776 seconds to run.
(Reporter)

Comment 3

6 years ago
Needinfo from Nicolas based on comment 2 :)
(Assignee)

Updated

6 years ago
Assignee: general → hv1989
Blocks: 796114
status-firefox22: --- → affected
status-firefox23: --- → affected
(Assignee)

Comment 4

6 years ago
Created attachment 738254 [details] [diff] [review]
Patch
Attachment #738254 - Flags: review?(nicolas.b.pierron)
Comment on attachment 738254 [details] [diff] [review]
Patch

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

Ok, for this patch, after all this function would be removed with the removal of the analysis types.

::: js/src/ion/IonBuilder.cpp
@@ +3123,5 @@
> +                        MInstruction *toDouble = MUnbox::New(ins, MIRType_Double, MUnbox::Fallible);
> +                        current->add(toDouble);
> +                        ins = toDouble;
> +                    }
> +                    JS_ASSERT(ins->type() == MIRType_Double);

nit: Sounds like this should be added as part of the previous if condition which is checking if:

    callerType == JSVAL_TYPE_DOUBLE || ins->type() == MIRType_Double
Attachment #738254 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/61e661489460
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox23: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 8

6 years ago
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Comment 9

6 years ago
Comment on attachment 738254 [details] [diff] [review]
Patch

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


User impact if declined: 
Crashes around zero with special scripts, not sure if possible to attack in browser

Testing completed (on m-c, etc.):
m-i: 1 day, m-c: 1 hour

Risk to taking this patch (and alternatives if risky): 
Almost no risk

String or IDL/UUID changes made by this patch:
/
Attachment #738254 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

6 years ago
Comment on attachment 738254 [details] [diff] [review]
Patch

When landing this, we probably want to land bug 863261 at the same time. I will request approval again, when other bug lands. (Should happen quickly)
Attachment #738254 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Depends on: 863755
(Assignee)

Comment 11

6 years ago
Created attachment 740920 [details] [diff] [review]
Patch

This is the version that got checked in. Carrying r+ over.

[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.): 
m-c: already a week, I think

Risk to taking this patch (and alternatives if risky): 
Very low. This change make sure we don't unbox already unboxed double. Note that there is a known issue with this patch. I will only push when bug 863755 is approved too.

String or IDL/UUID changes made by this patch:
/
Attachment #738254 - Attachment is obsolete: true
Attachment #740920 - Flags: review+
Attachment #740920 - Flags: approval-mozilla-aurora?
Comment on attachment 740920 [details] [diff] [review]
Patch

Approving along with bug 863755 given the minimal risk here, but would be good to understand what the security rating fort his bug is.
Attachment #740920 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

6 years ago
Keywords: sec-critical
status-b2g18: --- → unaffected
status-firefox21: --- → unaffected
status-firefox-esr17: --- → unaffected
Keywords: regression
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main22-]
Marking status-firefox23:verified based on comment 8.
status-firefox23: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.