Closed Bug 689269 Opened 8 years ago Closed 8 years ago
The conservative GC can use (not just read) uninitialized memory
I was seeing crashes like this #0 0x002663f5 in CrashInJS () at /Users/cltbld/esp/mozilla-inbound/js/src/jsutil.cpp:92 #1 0x0026645f in JS_Assert (s=0x482678 "addr % Cell::CellSize == 0", file=0x482648 "/Users/cltbld/esp/mozilla-inbound/js/src/jsgc.h", ln=669) at /Users/cltbld/esp/mozilla-inbound/js/src/jsutil.cpp:103 #2 0x000e0011 in js::gc::Cell::address (this=0xdadadada) at jsgc.h:669 #3 0x000e0299 in js::gc::Cell::arenaHeader (this=0xdadadada) at jsgc.h:677 #4 0x000efb9a in js::gc::Cell::isAligned (this=0xdadadada) at jsgc.h:701 #5 0x000ebe42 in js::gc::CheckMarkedThing<JSObject> (trc=0xbfffdc90, thing=0xdadadada) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgcmark.cpp:118 #6 0x000f00b7 in js::gc::Mark<JSObject> (trc=0xbfffdc90, thing=0xdadadada) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgcmark.cpp:128 #7 0x000ed5be in js::gc::MarkObject (trc=0xbfffdc90, obj=@0xdadadada, name=0x4950b2 "type_singleton") at /Users/cltbld/esp/mozilla-inbound/js/src/jsgcmark.cpp:174 #8 0x000ed84a in js::gc::MarkTypeObject (trc=0xbfffdc90, type=0x1804260, name=0x49513f "type_stack") at /Users/cltbld/esp/mozilla-inbound/js/src/jsgcmark.cpp:233 #9 0x000ee2bc in js::gc::MarkKind (trc=0xbfffdc90, thing=0x1804260, kind=JSTRACE_TYPE_OBJECT) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgcmark.cpp:411 #10 0x000e7f04 in js::MarkIfGCThingWord (trc=0xbfffdc90, w=25182816) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:827 #11 0x000d97ea in js::MarkWordConservatively (trc=0xbfffdc90, w=25182816) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:854 #12 0x000d98bd in js::MarkRangeConservatively (trc=0xbfffdc90, begin=0xbfffdd40, end=0xc0000000) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:862 #13 0x000d9972 in js::MarkThreadDataConservatively (trc=0xbfffdc90, td=0x9b7014) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:879 #14 0x000d9a3b in js::MarkConservativeStackRoots (trc=0xbfffdc90) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:911 #15 0x000d9ae5 in js::MarkRuntime (trc=0xbfffdc90) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:1877 #16 0x000d9f01 in BeginMarkPhase (cx=0xa07330, gcmarker=0xbfffdc90, gckind=GC_NORMAL, gcTimer=@0xbfffddbc) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:2257 #17 0x000dc6e6 in MarkAndSweep (cx=0xa07330, gckind=GC_NORMAL, gcTimer=@0xbfffddbc) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:2418 #18 0x000dc9fd in GCCycle (cx=0xa07330, comp=0x1015400, gckind=GC_NORMAL, gcTimer=@0xbfffddbc) at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:2664 #19 0x000dcd04 in js_GC () at /Users/cltbld/esp/mozilla-inbound/js/src/jsgc.cpp:2750 #20 0x00040a10 in JS_CompartmentGC (cx=0xa07330, comp=0x1015400) at /Users/cltbld/esp/mozilla-inbound/js/src/jsapi.cpp:2616 #21 0x0000b01c in GC (cx=0xa07330, argc=1, vp=0xb00058) at /Users/cltbld/esp/mozilla-inbound/js/src/shell/js.cpp:1207 #22 0x003e704c in CallJSNative [inlined] () at jscntxt.h:296 #23 CallCompiler::generateNativeStub (this=0xbfffe620) at jscntxtinlines.h:937 #24 0x003dbf9b in js::mjit::ic::NativeCall (f=@0xbfffe640, ic=0x10177ac) at /Users/cltbld/esp/mozilla-inbound/js/src/methodjit/MonoIC.cpp:1162 #25 0x009c9c48 in ?? () #26 0x0034233b in js::mjit::EnterMethodJIT (cx=0xbad, fp=0x1800040, code=0x0, stackLimit=0x340aa7, partial=false) at /Users/cltbld/esp/mozilla-inbound/js/src/methodjit/MethodJIT.cpp:866 Previous frame inner to this frame (gdb could not unwind past this frame) Looking into it a bit in gdb showed that * We were creating a new arena when allocating a type. * allocateFromNewArena gets used and the arena stays marked as fully used * the compiler leaves a pointer into the arena in the stack. * a gc happens and it starts looking at the stack (MarkConservativeStackRoots) * having found the pointer in the stack, the gc proceeds to check if it is live in MarkIfGCThingWord. * the pointer does point inside an arena. The last line of defense is InFreeList. * InFreeList fails with if (!aheader->hasFreeThings()) return false; * The GC starts walking uninitialized (0xdadadada) memory.
Attachment #562523 - Flags: review?
8 years ago
Comment on attachment 562523 [details] [diff] [review] proposed patch Note that I am not familiar with this code, so I am not sure when is the right moment to use copyFreeListsToArenas.
8 years ago
Attachment #562523 - Flags: review?(cdleary) → review?(anygregor)
Comment on attachment 562523 [details] [diff] [review] proposed patch Is there a test case for this?
Attachment #562523 - Flags: review?(anygregor) → review?(igor)
> Is there a test case for this? Not a small one. I noticed this doing a debug + opt build on OS X which would fail in the jit_test. I have created a try run in https://tbpl.mozilla.org/?tree=Try&rev=4d204f9f33a1 with this patch and optimizations enabled. The OS X debug build went green already, so it is better than it was. I can try to copy the build directory from the bot where I was able to reproduce it and see if it would reproduce in a 10.6 or 10.7 install. Would that help?
As far as I can see this is a regression from the bug 605662. The conservative stack scanner during per-compartment GC should skip GC things from arenas that does not belong to the current compartment.
Comment on attachment 562523 [details] [diff] [review] proposed patch The patch should just teach MarkIfGCThingWord to check right after the aheader->allocated() for the compartment match and to ignore the ptr that does not belong to the current compartment. That requires to extend ConservativeGCTest with CGCT_OTHERCOMPARTMENT and patch the conservative roots dumping code accordingly.
Attachment #562523 - Flags: review?(igor) → review-
Patch is still building, will update the patch with the results once I have it.
Actually, I think this regressed when TI landed. Normally, the conservative scanner calls MarkKind, which calls Mark<T>, and that does a compartment check. However, for type objects we do some extra work that isn't guarded by a compartment check (the extra stuff in MarkTypeObject). Rafael's patch seems like the safest fix. But it would be really nice to make MarkTypeObject work the same way as other mark paths.
Although I just realized that Rafael's patch doesn't handle the case where gcCurrentCompartment is NULL, which means we're collecting all compartments. In that case, we should never return CGCT_OTHERCOMPARTMENT.
(In reply to Bill McCloskey (:billm) from comment #8) > Although I just realized that Rafael's patch doesn't handle the case where > gcCurrentCompartment is NULL, which means we're collecting all compartments. > In that case, we should never return CGCT_OTHERCOMPARTMENT. I just found that in my testing :-) So, should keep the check in the position it is in the current patch or move it to MarkTypeObject?
I think it's clearest to keep the check where it is. Just change it like so: if (rt->gcCurrentCompartment && rt->gcCurrentCompartment != aheader->compartment) return CGCT_OTHERCOMPARTMENT; We can deal with the MarkTypeObject thing later.
BTW, I tried to use JS_DUMP_CONSERVATIVE_GC_ROOTS=stdout to double check this, but it looks to be broken at the moment.
Comment on attachment 562833 [details] [diff] [review] new patch Please call the variable curComp instead of CurCompartiment. Also, JS style is for 100 character lines, so I don't think you need two lines for the assignment. Thanks for the patch.
Attachment #562833 - Flags: review?(wmccloskey) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(In reply to Bill McCloskey (:billm) from comment #7) > Actually, I think this regressed when TI landed. Normally, the conservative > scanner calls MarkKind, which calls Mark<T>, and that does a compartment > check. However, for type objects we do some extra work that isn't guarded by > a compartment check (the extra stuff in MarkTypeObject). The issue was never considered or commented for the bug 605662 and just by luck we did not get a bug before TI landing. In any case, we need to nominate this bug for aurora to make sure that TI would be released with this fix.
Comment on attachment 562833 [details] [diff] [review] new patch Who should be cced to approve this for aurora?
Attachment #562833 - Flags: approval-mozilla-aurora?
You don't have to CC anybody, the relevant people just search for patches with the approval? flag set.
Attachment #562833 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[Triage Comment] This bug was approved for Aurora while Firefox 9 was on that channel. If there's reason to now take this for Firefox 9 on Beta, please set approval-mozilla-beta?
Comment on attachment 562833 [details] [diff] [review] new patch Shoot, this fell through the cracks.
Attachment #562833 - Flags: approval-mozilla-beta?
Really sorry about this. I tried checking on IRC yesterday if I should still do it but got no answer in time.
Attachment #562833 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I assume based on comment 3 this requires a debug build to verify?
You need to log in before you can comment on or make changes to this bug.