689269 - The conservative GC can use (not just read) uninitialized memory

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: 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)]

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 [inactive unless it's an emergency] (:billm)]

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

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

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]

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

Attachment: New patch I am testing

Patch is still building, will update the patch with the results once I have it.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 [inactive unless it's an emergency] (:billm)]

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

(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 [inactive unless it's an emergency] (:billm)]

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

Attachment: 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 [inactive unless it's an emergency] (:billm)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/29897f5185bb

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/29897f5185bb

Comment 15

[Igor Bukanov]

(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)]

Comment on attachment 562833 [details] [diff] [review]
new patch

Who should be cced to approve this for aurora?

Comment 17

[Andrew McCreight [:mccr8]]

You don't have to CC anybody, the relevant people just search for patches with the approval? flag set.

Comment 18

[Alex Keybl [:akeybl]]

[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 [inactive unless it's an emergency] (:billm)]

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

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

https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=0b06a096b180

Comment 22

[u279076]

I assume based on comment 3 this requires a debug build to verify?