Last Comment Bug 689269 - The conservative GC can use (not just read) uninitialized memory
: The conservative GC can use (not just read) uninitialized memory
Status: RESOLVED FIXED
[qa?]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla10
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks: compartmentGC 669953
  Show dependency treegraph
 
Reported: 2011-09-26 13:32 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-12-07 13:23 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
proposed patch (1.45 KB, patch)
2011-09-26 13:32 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
igor: review-
Details | Diff | Review
New patch I am testing (1.81 KB, patch)
2011-09-27 11:27 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
new patch (1.89 KB, patch)
2011-09-27 12:25 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
wmccloskey: review+
jst: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-26 13:32:43 PDT
Created attachment 562523 [details] [diff] [review]
proposed patch

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.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-26 13:35:46 PDT
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.
Comment 2 Bill McCloskey (:billm) 2011-09-26 14:08:58 PDT
Comment on attachment 562523 [details] [diff] [review]
proposed patch

Is there a test case for this?
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-26 16:30:29 PDT
> 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?
Comment 4 Igor Bukanov 2011-09-27 01:18:52 PDT
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 5 Igor Bukanov 2011-09-27 01:23:55 PDT
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.
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 11:27:04 PDT
Created attachment 562823 [details] [diff] [review]
New patch I am testing

Patch is still building, will update the patch with the results once I have it.
Comment 7 Bill McCloskey (:billm) 2011-09-27 11:56:28 PDT
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.
Comment 8 Bill McCloskey (:billm) 2011-09-27 11:58:38 PDT
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.
Comment 9 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 12:04:04 PDT
(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?
Comment 10 Bill McCloskey (:billm) 2011-09-27 12:07:43 PDT
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.
Comment 11 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 12:25:00 PDT
Created attachment 562833 [details] [diff] [review]
new patch

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 12 Bill McCloskey (:billm) 2011-09-27 12:33:48 PDT
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.
Comment 13 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 12:49:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/29897f5185bb
Comment 14 Marco Bonardo [::mak] 2011-09-28 02:05:43 PDT
https://hg.mozilla.org/mozilla-central/rev/29897f5185bb
Comment 15 Igor Bukanov 2011-09-28 03:55:12 PDT
(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 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-28 06:02:43 PDT
Comment on attachment 562833 [details] [diff] [review]
new patch

Who should be cced to approve this for aurora?
Comment 17 Andrew McCreight [:mccr8] 2011-09-28 06:03:55 PDT
You don't have to CC anybody, the relevant people just search for patches with the approval? flag set.
Comment 18 Alex Keybl [:akeybl] 2011-11-08 11:57:09 PST
[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 19 Bill McCloskey (:billm) 2011-11-08 12:30:05 PST
Comment on attachment 562833 [details] [diff] [review]
new patch

Shoot, this fell through the cracks.
Comment 20 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-09 09:27:57 PST
Really sorry about this. I tried checking on IRC yesterday if I should still do it but got no answer in time.
Comment 21 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-15 14:06:11 PST
https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=0b06a096b180
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-01 14:35:29 PST
I assume based on comment 3 this requires a debug build to verify?

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