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

VERIFIED FIXED in Firefox 14



7 years ago
6 years ago


(Reporter: decoder, Assigned: billm)


(Blocks: 1 bug, 4 keywords)

assertion, crash, sec-critical, testcase
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 unaffected)


(Whiteboard: [jsbugmon:ignore][advisory-tracking+], crash signature)


(2 attachments)



7 years ago
Created attachment 625998 [details]
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.
Assignee: general → wmccloskey

Comment 1

7 years ago
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 
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.
Created attachment 632088 [details] [diff] [review]

Another VM stack scanning bug. We can GC inside of GetElements before all the elements have been initialized.
Attachment #632088 - Flags: review?(bhackett1024)

Comment 4

7 years ago
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+
Last Resolved: 7 years ago
status-firefox16: --- → fixed
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?
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox-esr10: --- → ?
Keywords: sec-critical
Comment on attachment 632088 [details] [diff] [review]

[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]

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
status-firefox-esr10: affected → unaffected
tracking-firefox-esr10: ? → ---

Comment 12

7 years ago
JSBugMon: This bug has been automatically verified fixed.


7 years ago
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][advisory-tracking+]
Group: core-security

Comment 13

6 years ago
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.