IonMonkey: Assertion failure: ins->type() == MIRType_Double || ins->type() == MIRType_Value, at ion/IonBuilder.cpp:3098

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: h4writer)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

22 Branch
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 unaffected, firefox22 fixed, firefox23 unaffected)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The following testcase asserts on mozilla-aurora revision 4cf75b25cdc3 (run with --ion-eager):


var msPerDay =  86400000;
function UTC(t) {
  return 0;
}
function MakeDay( year, month, date ) {}
function MakeDate( day, time ) {
  return ( day * msPerDay ) + time;
}
function TimeClip( t ) {
  if ( isNaN( t ) ) {
    return ( Number.NaN );
  }
  return ( ToInteger( t ) );
}
function ToInteger( t ) {}
d = new Date(0);
addNewTestCase(
  UTCDateFromTime(SetMonth(0,1,1)),
  LocalDateFromTime(SetMonth(0,1,1)) );
function addNewTestCase( DateString, UTCDate, LocalDate) {}
function LocalDateFromTime(t) {}
function UTCDateFromTime(t) {
  d.value = TimeClip( MakeDate( MakeDay( d.year, d.month, d.date ), d.time ) );
}
function SetMonth( t, mon, date ) {
  return ( TimeClip (UTC(1899)) );
}
(Reporter)

Comment 1

5 years ago
Found during aurora fuzzing as requested in bug 865339. h4writer will take care of it :)
Blocks: 865339, 724444
Version: Trunk → 22 Branch
(Assignee)

Comment 2

5 years ago
Created attachment 742291 [details] [diff] [review]
Possible patch

For now this looks like an over-restrictive assert. This testcase won't do something wrong when removing that assert. I'm gonna investigate a little bit further to be sure it can't be abused otherwise.
Assignee: general → hv1989
(Assignee)

Comment 3

5 years ago
Comment on attachment 742291 [details] [diff] [review]
Possible patch

Yes, an over restrictive assert. Type could be MIRType_Int32. (Actually we could remove the MToInt32 in that case, but opted against, since we are working on mozilla-aurora. Less change is better).
Attachment #742291 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 4

5 years ago
Not security-sensitive based on comment 3. Unhiding :)
Group: core-security
Attachment #742291 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 5

5 years ago
Comment on attachment 742291 [details] [diff] [review]
Possible patch

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

User impact if declined: Nothing on release builds, asserts on debug build

Testing completed (on m-c, etc.): passes jit-tests, can't land on m-c/m-i, since that particular code is gone.

Risk to taking this patch (and alternatives if risky): 
Since this is on aurora, I already went for the easiest fix that touches the least code. This has almost no risk, since the code has been running for weeks in normal builds (where there are no asserts). 

String or IDL/UUID changes made by this patch: /
Attachment #742291 - Flags: approval-mozilla-aurora?
Is this about FF22 code or about aurora tests?  If we don't take this on aurora what happens when m-c (FF23) moves to aurora?
Flags: needinfo?(hv1989)
(Assignee)

Comment 7

5 years ago
This is a change in FF22 only code. On FF23 this code is totally rewritten and doesnt have this problem. If needed I can givethe bugnumber where this has been done. Ergo upon merge from m-c the issue will just dissappear.
Note that this patch also doesn't change behaviour on FF22, but just adjust a signalition that was giving false positives.
Flags: needinfo?(hv1989)

Updated

5 years ago
status-firefox21: --- → unaffected
status-firefox22: --- → affected
status-firefox23: --- → unaffected

Comment 8

5 years ago
Comment on attachment 742291 [details] [diff] [review]
Possible patch

Approving for correctness and based upon low risk evaluation, even given no known user impact.
Attachment #742291 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
status-firefox22: affected → fixed
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.