757431 - Assertion failure: v->toGCThing(), at gc/Marking.cpp:327 or Crash [@ js::gc::ArenaHeader::allocated]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Test case for shell (run with -n -m -a)

The attached test asserts/crashes on mozilla-central revision 5586efaf687d (options -m -n -a).

Crash trace (debug build):

Program received signal SIGSEGV, Segmentation fault.
0x0804c753 in js::gc::ArenaHeader::allocated (this=0x0) at ../../gc/Heap.h:467
467             JS_ASSERT(allocKind <= size_t(FINALIZE_LIMIT));
(gdb) bt 8
#0  0x0804c753 in js::gc::ArenaHeader::allocated (this=0x0) at ../../gc/Heap.h:467
#1  0x0804c7b7 in js::gc::ArenaHeader::getAllocKind (this=0x0) at ../../gc/Heap.h:497
#2  0x0806b1f5 in js::gc::Cell::getAllocKind (this=0x0) at ../gc/Heap.h:941
#3  0x080f29e0 in js::gc::GetGCThingTraceKind (thing=0x0) at /srv/repos/mozilla-central/js/src/jsgcinlines.h:30
#4  0x0832568e in js::gc::MarkKind (trc=0x86495b8, thingp=0xffffb48c, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:231
#5  0x08325c00 in js::gc::MarkValueInternal (trc=0x86495b8, v=0xf77021a0) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:330
#6  0x08325d2c in js::gc::MarkValueRoot (trc=0x86495b8, v=0xf77021a0, name=0x848f00a "vm_stack") at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:352
#7  0x082b20cf in js::StackSpace::markFrameSlots (this=0x8616b30, trc=0x86495b8, fp=0xf7702160, slotsEnd=0xf77021b0, pc=0x8647dfe "W") at /srv/repos/mozilla-central/js/src/vm/Stack.cpp:501
(More stack frames follow...)
(gdb) x /i $pc
=> 0x804c753 <js::gc::ArenaHeader::allocated() const+21>:       movzbl 0xc(%eax),%eax
(gdb) info reg eax
eax            0x0      0


S-s due to GC-related assertion with crash, until the crash is confirmed to be harmless and always a null-deref.

Comment 1

[Christian Holler (:decoder)]

Fuzzer came up with an even simpler test that shows the same assertion and crash:


function setterFunction(v) { called = true; }
function getterFunction(v) { return "getter"; }
Object.defineProperty(Array.prototype, 1,{ 
  get: getterFunction, 
  set: setterFunction 
});
gczeal(4);
var N = 350;
var source = "".concat(
  repeat_str("try { f(); } finally {\n", N),
  repeat_str("}", N));
function repeat_str(str, repeat_count) {
  var arr = new Array(--repeat_count);
  while (repeat_count != 0)
    arr[--repeat_count] = str;
  return str.concat.apply(str, arr);
}


If you think this is not the same bug though, let me know and I'll clone :)

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The first bug is a dupe of bug 753283, which I'm having trouble landing. I'm working on it, though.

The second one is a separate bug, as far as I can tell. Let's make this bug be about the second test case and ignore the first one. Hopefully that won't screw up your tools too much.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

Another VM stack scanning bug. We can GC inside of GetElements before all the elements have been initialized.

Comment 4

[Christian Holler (:decoder)]

No problem, we'll just ignore this bug for the tool so it does not get accidentally verified the wrong way.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a79fe8932e3f

Comment 6

[Ed Morley [:emorley]]

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

Comment 7

[Daniel Veditz [:dveditz]]

Guessing sec-critical since the fix is "MakeRangeGCSafe(f.regs.sp, delta);", and we appear to have this code back to at least ESR. Is there any reason we shouldn't take this fix on old branches?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 632088 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 723313
User impact if declined: Low likelihood of GC crashes, exploits.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low. It just overwrites unused memory with NULLs.
String or UUID changes made by this patch: None.

No need to take on ESR. We used to handle the undefined memory more gracefully before bug 723313.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 632088 [details] [diff] [review]
fix

low risk, sec critical - approved.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

adjusting the esr tracking/status flags according to comment 8

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/733e912f7e05
https://hg.mozilla.org/releases/mozilla-beta/rev/0ad2987252b2

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug757431.js.