842424 - IonMonkey: Assertion failure: addr % CellSize == 0, at ../../gc/Heap.h:831

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 0acbd06d48a9 (run with --ion-eager):


gczeal(4);
function Employee ( name, dept ) {
  for each(b in [0, -0]) {}    
  this.dept = dept || "general";
}
function Manager () {}
Manager.prototype = new Employee();

Comment 1

[Christian Holler (:decoder)]

Debug backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0804c117 in js::gc::Cell::address (this=0x42) at ../../gc/Heap.h:831
831         JS_ASSERT(addr % CellSize == 0);
(gdb) bt
#0  0x0804c117 in js::gc::Cell::address (this=0x42) at ../../gc/Heap.h:831
#1  0x0804c267 in js::gc::Cell::arenaHeader (this=0x42) at ../../gc/Heap.h:947
#2  0x0807fc17 in js::gc::Cell::getAllocKind (this=0x42) at ../gc/Heap.h:970
#3  0x081a57fd in js::gc::GetGCThingTraceKind (thing=0x42) at ../jsgcinlines.h:59
#4  0x085e4bc2 in js::gc::MarkKind (trc=0x8cc5f34, thingp=0xffff8a00, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:353
#5  0x085e59b7 in MarkValueInternal (trc=0x8cc5f34, v=0xf7433050) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:473
#6  0x085e79c1 in js::gc::MarkValueUnbarriered (trc=0x8cc5f34, v=0xf7433050, name=0x8a93499 "write barrier") at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:697
#7  0x087632c0 in js::ion::MarkValueFromIon (rt=0x8cc5d50, vp=0xf7433050) at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:2164
#8  0xf7fc8349 in ?? ()


S-s due to GC-related assertion.

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:   120953:53a09e22d034
user:        Terrence Cole
date:        Mon Jan 28 11:01:54 2013 -0800
summary:     Bug 706885 - Do not store singletons in the Nursery; r=bhackett

This iteration took 99.199 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from Terrence based on comment 2.

Comment 4

[Terrence Cole [:terrence]]

The thing that's being marked is a busted pointer. I have no idea how the implicated change could have caused this, but then I'm not terribly familiar with exactly how Ion uses the singleton property. I'm going to forward the needinfo to Brian, since he does actually know this and will probably have a much better idea what might be going wrong here.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

There was a missing use of newKind that caused 'new' objects which are supposed to have singleton type to not have singleton type.  Their properties were then cleared, causing them to go out of sync with their type information --- they were marked as definitely having properties when they actually didn't have any, and Ion invoked a pre barrier on a slot of the object which did not correspond to any property, which in debug builds is a poisoned ObjectValue(42).

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 716040 [details] [diff] [review]
patch

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

Nice assertion!

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 716040 [details] [diff] [review]
patch

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

not at all

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?

only aurora is affected, no release branches

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

tbpl is fine

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0002d4b427

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 716040 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 706885
User impact if declined: potential exploit
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none

Comment 10

[Brian Hackett [Laid off!]]

Followup to remove the nice assertion.  It didn't build on any compiler other than the one I tested on, and I don't think it adds that much.

https://hg.mozilla.org/integration/mozilla-inbound/rev/48c067a87ba2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48c067a87ba2
https://hg.mozilla.org/mozilla-central/rev/5b0002d4b427

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a93c1d7a3d5e

FWIW, I qfolded the two patches together for the uplift.

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Verified the fix using a built js from mozilla-beta-af3011f1cccb (latest mozilla beta source atm) and the testcase from bug description: no assesrtions.