916761 - Crash on Heap with ParallelArray

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


Array.prototype[2] = '';
function toString(r) {
  var result = "";
  for (var i = 0; i < 100; i++)
    result += r.get(i);
}
toString(new ParallelArray(['x', 'x']));

Comment 1

[Christian Holler (:decoder)]

This looks like a null-deref, but the crash is on the heap without any symbols, so marking as s-s until triaged.

Comment 2

[Christian Holler (:decoder)]

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/d660739f7498
user:        Jan de Mooij
date:        Fri Sep 06 13:52:16 2013 +0200
summary:     Bug 913424 - IonMonkey: Handle JSOP_THIS primitive this case. r=bhackett

This iteration took 0.958 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from Jandem based on comment 2 :)

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Bug 913424 may have exposed this by Ion-compiling more self-hosted PJS code (yay) but the bug itself is a regression from bug 841621.

makeInliningDecision should not return true without calling HeapTypeSet::WatchObjectStateChange (this call ensures the outer script is invalidated when types of the inlined script change).

Comment 5

[Jan de Mooij [:jandem]]

PJS is disabled on aurora and beta.

Comment 6

[Niko Matsakis [:nmatsakis]]

Comment on attachment 810572 [details] [diff] [review]
Patch

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

Ah, good catch. I recall being a bit confused by a big change of structure when merging that pass, I see I missed some crucial code there at the end!

Comment 7

[Andrew McCreight [:mccr8]]

jandem, is this really just a null-deref, or should it be rated more severely, security-wise?  Thanks.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/346ce9e416cb

(In reply to Andrew McCreight [:mccr8] from comment #7)
> jandem, is this really just a null-deref, or should it be rated more
> severely, security-wise?  Thanks.

It's more dangerous than that, marking sec-critical.

Comment 9

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

This is trunk only so it doesn't need sec-approval+ to go in.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/346ce9e416cb

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.