988925 - Assertion failure: producer_ != nullptr, at jit/MIR.h:117

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

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


function x() {
    var i=0;
    var j=0;
    for (i = 3; i<= 100000; i+=00 )
      for (j = 2.e0 ; j < 1000; j+=2)
        var g = newGlobal();
} x();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140320090350" and the hash "61e72c13e60b".
The "bad" changeset has the timestamp "20140320090852" and the hash "3762c3e540cd".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=61e72c13e60b&tochange=3762c3e540cd

Comment 3

[Dan Gohman [:sunfish]]

It appears to be due to 3762c3e540cd. I'll investigate.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: update-valuenumbering.patch

ValueNumberer::simplify replaces one MDefinition with another, the value numbers of all the original MDefinition's users (transitively) become stale. Remove values from the values table when they are marked for a revisit.

Comment 5

[Dan Gohman [:sunfish]]

Conservatively marking sec-critical since in a build without assertions, it's not theoretically impossible that freed memory could be read.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8408686 [details] [diff] [review]
update-valuenumbering.patch

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

This looks fine, forwarding to Marty since he also fixed a GVN bug recently.

Comment 7

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8408686 [details] [diff] [review]
update-valuenumbering.patch

So it looks good, but I'm afraid that this will cause us to take longer to reach a fixed point.  Would it be possible to only do this when a new replacement has happened?  Or does the fact that it is transitive mean that simple checks won't cut it?

Comment 8

[Jan de Mooij [:jandem]]

What's the status of this sec-crit?

Comment 9

[Dan Gohman [:sunfish]]

My plan to fix this it to revert 3762c3e540cd. It's a simple and minor patch.

FWIW, I also plan to fix this on trunk with a much larger series of patches in bug 1004363.

Comment 10

[Dan Gohman [:sunfish]]

Attachment: revert-phi-change.patch

This patch just reverts changeset 3762c3e540cd, which fixes the given testcases.

Because this patch is sec-critical and the bug is present in mozilla-aurora, I'll request sec-approval before committing.

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8418776 [details] [diff] [review]
revert-phi-change.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It depends on how hard it is to manipulate this bug into an actual use-before-free. I suspect it's tricky to do.

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?

mozilla-aurora

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

Bug 981894.

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

I can easily make backports.

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

Very unlikely. It's a revert.

Comment 12

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

Comment on attachment 8418776 [details] [diff] [review]
revert-phi-change.patch

sec-approval+.

Let's get an Aurora patch nominated and approved as well.

Comment 13

[Dan Gohman [:sunfish]]

Comment on attachment 8418776 [details] [diff] [review]
revert-phi-change.patch

The same patch applies on mozilla-aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 981894
User impact if declined: Security bug
Testing completed (on m-c, etc.): It's a revert back to code tested on m-c.
Risk to taking this patch (and alternatives if risky): Low; it's a revert.
String or IDL/UUID changes made by this patch: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

So this only needs to land on Aurora per comment 9? Or should we take this on trunk for now as well?

Comment 15

[Dan Gohman [:sunfish]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> So this only needs to land on Aurora per comment 9? Or should we take this
> on trunk for now as well?

We want this on trunk as well as Aurora.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8418776 [details] [diff] [review]
revert-phi-change.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/d12f02a49c22
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e7bbb297f03

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d12f02a49c22

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.