Last Comment Bug 757431 - Assertion failure: v->toGCThing(), at gc/Marking.cpp:327 or Crash [@ js::gc::ArenaHeader::allocated]
: Assertion failure: v->toGCThing(), at gc/Marking.cpp:327 or Crash [@ js::gc::...
: assertion, crash, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
-- critical (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: langfuzz
  Show dependency treegraph
Reported: 2012-05-22 07:26 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 07:56 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Test case for shell (run with -n -m -a) (1.38 KB, application/javascript)
2012-05-22 07:26 PDT, Christian Holler (:decoder)
no flags Details
fix (2.85 KB, patch)
2012-06-11 17:46 PDT, Bill McCloskey (:billm)
bhackett1024: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2012-05-22 07:26:51 PDT
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.
Comment 1 User image Christian Holler (:decoder) 2012-05-25 07:48:35 PDT
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 :)
Comment 2 User image Bill McCloskey (:billm) 2012-06-11 15:35:49 PDT
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 User image Bill McCloskey (:billm) 2012-06-11 17:46:18 PDT
Created attachment 632088 [details] [diff] [review]

Another VM stack scanning bug. We can GC inside of GetElements before all the elements have been initialized.
Comment 4 User image Christian Holler (:decoder) 2012-06-12 06:51:11 PDT
No problem, we'll just ignore this bug for the tool so it does not get accidentally verified the wrong way.
Comment 6 User image Ed Morley [:emorley] 2012-06-19 01:24:30 PDT
Comment 7 User image Daniel Veditz [:dveditz] 2012-06-22 10:27:38 PDT
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 User image Bill McCloskey (:billm) 2012-06-22 11:07:12 PDT
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.
Comment 9 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 15:32:43 PDT
Comment on attachment 632088 [details] [diff] [review]

low risk, sec critical - approved.
Comment 10 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 15:33:14 PDT
adjusting the esr tracking/status flags according to comment 8
Comment 12 User image Christian Holler (:decoder) 2012-06-28 14:06:55 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 13 User image Christian Holler (:decoder) 2013-01-14 07:56:13 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug757431.js.

Note You need to log in before you can comment on or make changes to this bug.