Closed Bug 912264 Opened 9 years ago Closed 8 years ago

Assertion failure: false (MOZ_ASSUME_UNREACHABLE(Modified registers between VM call and OsiPoint)), at jit/shared/CodeGenerator-shared.cpp:521

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- unaffected
firefox26 - affected
firefox27 + fixed

People

(Reporter: decoder, Assigned: nbp)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update,ignore])

Attachments

(2 files)

The following testcase asserts on mozilla-central revision d54e0cce6c17 (run with --fuzzing-safe --ion-eager):


enableOsiPointRegisterChecks();
gczeal(4);
with({});
eval('for (var x in this) {}');
evaluate("\
var sText = 's';\
for (var i = 0; i < 250000; -i)\
    sText += 'a\\n';\
", { noScriptRval : true });
decoder, can we run autoBisect on this testcase?
We can try it :)
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/4be6c333853a
user:        Nicolas Pierron
date:        Thu Aug 29 10:20:28 2013 -0700
summary:     Bug 909544 - IonMonkey: Check VM Call sanity in visit functions. r=jandem

This iteration took 342.132 seconds to run.
needinfo? per comment 4 :)
Flags: needinfo?(nicolas.b.pierron)
Is this a crash that could happen in web content, or is this a crash in the extra checks added by enableOsiPointRegisterChecks?
Blocks: 909544
Keywords: regression
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 676322e0166c).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore,bisectfix]
I suspect this isn't fixed. I also vaguely remember that the test was sometimes not reproducing properly... so let's just try this.
Whiteboard: [jsbugmon:update,ignore,bisectfix] → [jsbugmon:update,bisectfix]
Whiteboard: [jsbugmon:update,bisectfix] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision e42374f83509).
JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c8dffd55eeef
user:        Nicholas D. Matsakis
date:        Fri Aug 30 07:14:30 2013 -0400
summary:     Bug 910777 - Add JSCLASS_IMPLEMENTS_BARRIERS to the Binary Data classes with custom trace r=sfink

This iteration took 407.602 seconds to run.
(In reply to Christian Holler (:decoder) from comment #9)
> The first good revision is:
> changeset:   http://hg.mozilla.org/mozilla-central/rev/c8dffd55eeef

This sounds unrelated.
Flags: needinfo?(nicolas.b.pierron)
Based on the comment 4, I we should consider this bug as sec-critical until proven safe.

What surprised me is that I did not spot any invalid use of callVM last time I audited the CodeGen for the try&catch patches.
Keywords: sec-critical
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Any luck?
Flags: needinfo?(nicolas.b.pierron)
(In reply to David Bolter [:davidb] from comment #12)
> Any luck?

Not yet, I did not investigated more since the last comment.
I will keep the need-info as a reminder. Thanks for the ping. :)
Ok, this was hard to follow and I am still missing a few pieces of the puzzle, but it seems that:
  1/ We do a concatenation with the callVM path.
  2? Raise the interrupt check.
  3/ Jump to the OutOfLine path of the interrupt check of the loop, without doing the reset of the checkRegs flags.
  4/ Fail on the OSI Point of the Interrupt check.

The assertion seems to be raised because we did not executed the reset of the checkRegs flags which is done before the execution of the instruction, as we jump directly to the out-of-line path.

I will try to understand from where we jump to the oolInterruptCheck and if there is no risk involved in this process.  The goal would be to determine if the OSI Point of the concatenation is executed before we jump to the oolInterruptCheck (which is likely), in which case there is no risk and this is just a really tricky false alarm.

(Keep the needinfo on my-self until I get a precise idea of what is going on)
Flags: needinfo?(bhackett1024)
… why does bugzilla reset flags that I did not changed …
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(bhackett1024) → needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> I will try to understand from where we jump to the oolInterruptCheck and if
> there is no risk involved in this process.  The goal would be to determine
> if the OSI Point of the concatenation is executed before we jump to the
> oolInterruptCheck (which is likely), in which case there is no risk and this
> is just a really tricky false alarm.

Ok, this is what is happening here. :)
The backedge of the loop jumps directly into the OOL path of the implicit interrupt check, and thus skip the reset added by generateBody.
Attachment #811362 - Flags: review?(jdemooij)
Flags: needinfo?(nicolas.b.pierron)
I remove the sec-critical rating based on comment 16.
Keywords: sec-critical
Is this no longer a security issue and we can unhide it then?
(In reply to Al Billings [:abillings] from comment #18)
> Is this no longer a security issue and we can unhide it then?

I confident that this is no longer a security issue, but I prefer to wait for the review in case I missed something.
Comment on attachment 811362 [details] [diff] [review]
Reset osi-regs flags before the implicit interrupt check.

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

Good catch.
Attachment #811362 - Flags: review?(jdemooij) → review+
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/22e91a399223
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
please nominate for Aurora uplift.
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #23)
> please nominate for Aurora uplift.

I am happy to uplift it but I do not how this would be useful except for fuzzing.
Is this the goal of this tracking flag?
Flags: needinfo?(lsblakk)
(In reply to Nicolas B. Pierron [:nbp] from comment #24)
> (In reply to lsblakk@mozilla.com [:lsblakk] from comment #23)
> > please nominate for Aurora uplift.
> 
> I am happy to uplift it but I do not how this would be useful except for
> fuzzing.
> Is this the goal of this tracking flag?

Yes, if it's a regression (and it's marked as such) then we will usually track for the channels affected.  If this is low risk to uplift, we might as well get this onto FF26 branch (which is about to be Beta today).
Flags: needinfo?(lsblakk)
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #25)
> (In reply to Nicolas B. Pierron [:nbp] from comment #24)
> > (In reply to lsblakk@mozilla.com [:lsblakk] from comment #23)
> > > please nominate for Aurora uplift.
> > 
> > I am happy to uplift it but I do not how this would be useful except for
> > fuzzing.

s/how/think/

> > Is this the goal of this tracking flag?
> 
> Yes, if it's a regression (and it's marked as such) then we will usually
> track for the channels affected.  If this is low risk to uplift, we might as
> well get this onto FF26 branch (which is about to be Beta today).

This "regression" is not visible in any way from users in any way. This patch just sanitize the assertion.
removing tracking as per comment 26, if this isn't visible to users and there's no other reason to uplift to beta let's just let it ride the trains.
You need to log in before you can comment on or make changes to this bug.