The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla10
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox9 fixed)

Details

(Whiteboard: [qa?])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Attachment #562523 - Flags: review?
Attachment #562523 - Attachment is patch: true
Attachment #562523 - Flags: review? → review?(cdleary)
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.
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?

Comment 4

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

Comment 5

6 years ago
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-
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.
Attachment #562523 - Attachment is obsolete: true
Attachment #562823 - Flags: review?(igor)
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.
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.
Assignee: general → respindola
Attachment #562823 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #562823 - Flags: review?(igor)
Attachment #562833 - Flags: review?(wmccloskey)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/29897f5185bb
https://hg.mozilla.org/mozilla-central/rev/29897f5185bb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Comment 15

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

Updated

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

Updated

5 years ago
Attachment #562833 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=0b06a096b180

Updated

5 years ago
status-firefox9: --- → fixed
I assume based on comment 3 this requires a debug build to verify?
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.