Closed Bug 757431 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla16
Tracking Status
firefox14 --- fixed
firefox15 --- fixed
firefox16 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: decoder, Assigned: billm)

Details

(4 keywords, Whiteboard: [jsbugmon:ignore][advisory-tracking+])

Crash Data

Attachments

(2 files)

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.
Assignee: general → wmccloskey
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 :)
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.
Attached patch fixSplinter Review
Another VM stack scanning bug. We can GC inside of GetElements before all the elements have been initialized.
Attachment #632088 - Flags: review?(bhackett1024)
No problem, we'll just ignore this bug for the tool so it does not get accidentally verified the wrong way.
Whiteboard: js-triage-needed → [jsbugmon:ignore]
Attachment #632088 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/a79fe8932e3f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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 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.
Attachment #632088 - Flags: approval-mozilla-beta?
Attachment #632088 - Flags: approval-mozilla-aurora?
Comment on attachment 632088 [details] [diff] [review]
fix

low risk, sec critical - approved.
Attachment #632088 - Flags: approval-mozilla-beta?
Attachment #632088 - Flags: approval-mozilla-beta+
Attachment #632088 - Flags: approval-mozilla-aurora?
Attachment #632088 - Flags: approval-mozilla-aurora+
adjusting the esr tracking/status flags according to comment 8
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][advisory-tracking+]
Group: core-security
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug757431.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.