Closed Bug 1343261 Opened 3 years ago Closed 3 years ago

Crash [@ JSObject::compartment] or Crash [@ js::gc::GCRuntime::markCompartments] with nukeCCW

Categories

(Core :: JavaScript: GC, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 53+ fixed
thunderbird_esr45 --- fixed
thunderbird_esr52 --- fixed
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: decoder, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(7 keywords, Whiteboard: [jsbugmon:][adv-main53+][adv-esr45.9+][adv-esr52.1+])

Crash Data

Attachments

(3 files, 3 obsolete files)

The following testcase crashes on mozilla-central revision 1bc2ad020aee (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

var wrapper = evaluate("({a: 15, b: {c: 42}})", {
    global: newGlobal({})
});
nukeCCW(wrapper);
gczeal(8, 1);
switch (lfRunTypeId) {}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000998fbd in JSObject::compartment (this=<optimized out>) at js/src/jsobj.h:175
#0  0x0000000000998fbd in JSObject::compartment (this=<optimized out>) at js/src/jsobj.h:175
#1  js::CrossCompartmentKey::compartment()::GetCompartmentFunctor::operator()(JSObject**) const (this=<synthetic pointer>, tp=<optimized out>) at js/src/jscompartment.h:187
#2  decltype ({parm#1}(static_cast<JSObject**>((decltype(nullptr))0))) js::CrossCompartmentKey::applyToWrapped<js::CrossCompartmentKey::compartment()::GetCompartmentFunctor>(js::CrossCompartmentKey::compartment()::GetCompartmentFunctor)::WrappedMatcher::match(JSObject*&) (obj=<optimized out>, this=<synthetic pointer>) at js/src/jscompartment.h:161
#3  mozilla::detail::VariantImplementation<unsigned char, 0ul, JSObject*, JSString*, mozilla::Tuple<js::NativeObject*, JSScript*>, mozilla::Tuple<js::NativeObject*, JSObject*, js::CrossCompartmentKey::DebuggerObjectKind> >::match<decltype ({parm#1}(static_cast<JSObject**>((decltype(nullptr))0))) js::CrossCompartmentKey::applyToWrapped<js::CrossCompartmentKey::compartment()::GetCompartmentFunctor>(js::CrossCompartmentKey::compartment()::GetCompartmentFunctor)::WrappedMatcher&, mozilla::Variant<JSObject*, JSString*, mozilla::Tuple<js::NativeObject*, JSScript*>, mozilla::Tuple<js::NativeObject*, JSObject*, js::CrossCompartmentKey::DebuggerObjectKind> > >(decltype ({parm#1}(static_cast<JSObject**>((decltype(nullptr))0))) js::CrossCompartmentKey::applyToWrapped<js::CrossCompartmentKey::compartment()::GetCompartmentFunctor>(js::CrossCompartmentKey::compartment()::GetCompartmentFunctor)::WrappedMatcher&, mozilla::Variant<JSObject*, JSString*, mozilla::Tuple<js::NativeObject*, JSScript*>, mozilla::Tuple<js::NativeObject*, JSObject*, js::CrossCompartmentKey::DebuggerObjectKind> >&) (aV=..., aMatcher=<synthetic pointer>) at dist/include/mozilla/Variant.h:266
#4  mozilla::Variant<JSObject*, JSString*, mozilla::Tuple<js::NativeObject*, JSScript*>, mozilla::Tuple<js::NativeObject*, JSObject*, js::CrossCompartmentKey::DebuggerObjectKind> >::match<decltype ({parm#1}(static_cast<JSObject**>((decltype(nullptr))0))) js::CrossCompartmentKey::applyToWrapped<js::CrossCompartmentKey::compartment()::GetCompartmentFunctor>(js::CrossCompartmentKey::compartment()::GetCompartmentFunctor)::WrappedMatcher&>(decltype ({parm#1}(static_cast<JSObject**>((decltype(nullptr))0))) js::CrossCompartmentKey::applyToWrapped<js::CrossCompartmentKey::compartment()::GetCompartmentFunctor>(js::CrossCompartmentKey::compartment()::GetCompartmentFunctor)::WrappedMatcher&) (aMatcher=<synthetic pointer>, this=<optimized out>) at dist/include/mozilla/Variant.h:625
#5  js::CrossCompartmentKey::applyToWrapped<js::CrossCompartmentKey::compartment()::GetCompartmentFunctor>(js::CrossCompartmentKey::compartment()::GetCompartmentFunctor) (f=..., this=<optimized out>) at js/src/jscompartment.h:166
#6  js::CrossCompartmentKey::compartment (this=<optimized out>) at js/src/jscompartment.h:193
#7  js::gc::GCRuntime::markCompartments (this=this@entry=0x7ffff695e5f0) at js/src/jsgc.cpp:3996
#8  0x00000000009999f1 in js::gc::GCRuntime::beginMarkPhase (this=this@entry=0x7ffff695e5f0, reason=reason@entry=JS::gcreason::DEBUG_GC, lock=...) at js/src/jsgc.cpp:3939
#9  0x00000000009a38e5 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff695e5f0, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC, lock=...) at js/src/jsgc.cpp:5925
#10 0x00000000009a4e44 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff695e5f0, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6281
#11 0x00000000009a5738 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff695e5f0, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6430
#12 0x00000000009a70fc in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff695e5f0) at js/src/jsgc.cpp:6965
#13 0x0000000000d26be5 in js::gc::GCRuntime::gcIfNeededPerAllocation (this=0x7ffff695e5f0, cx=cx@entry=0x7ffff6948000) at js/src/gc/Allocator.cpp:230
#14 0x0000000000d33f38 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ffff6948000, kind=js::gc::AllocKind::ACCESSOR_SHAPE) at js/src/gc/Allocator.cpp:191
#15 0x0000000000d372ea in js::Allocate<js::AccessorShape, (js::AllowGC)1> (cx=cx@entry=0x7ffff6948000) at js/src/gc/Allocator.cpp:142
#16 0x00000000009e6841 in js::Shape::new_ (nfixed=7, other=..., cx=0x7ffff6948000) at js/src/vm/Shape-inl.h:110
#17 js::PropertyTree::getChild (this=this@entry=0x7ffff44ca9e0, cx=cx@entry=0x7ffff6948000, parentArg=parentArg@entry=0x7ffff46b9c10, child=child@entry=...) at js/src/jspropertytree.cpp:185
#18 0x0000000000bb2bbb in js::NativeObject::getChildProperty (cx=cx@entry=0x7ffff6948000, obj=obj@entry=..., parent=..., parent@entry=..., child=child@entry=...) at js/src/vm/Shape.cpp:451
#19 0x0000000000bc7ed7 in js::NativeObject::addPropertyInternal (cx=cx@entry=0x7ffff6948000, obj=obj@entry=..., id=..., id@entry=..., getter=0x7ffff46c68c0, setter=0x0, slot=16777215, attrs=112, flags=0, entry=0x0, allowDictionary=true, keep=...) at js/src/vm/Shape.cpp:633
#20 0x0000000000bc9440 in js::NativeObject::putProperty (cx=cx@entry=0x7ffff6948000, obj=obj@entry=..., id=id@entry=..., getter=0x7ffff46c68c0, setter=0x0, slot=slot@entry=16777215, attrs=112, flags=0) at js/src/vm/Shape.cpp:791
#21 0x0000000000b502c8 in AddOrChangeProperty (cx=cx@entry=0x7ffff6948000, obj=obj@entry=..., id=id@entry=..., desc=...) at js/src/vm/NativeObject.cpp:1233
#22 0x0000000000b51051 in js::NativeDefineProperty (cx=cx@entry=0x7ffff6948000, obj=..., id=..., id@entry=..., desc_=..., result=...) at js/src/vm/NativeObject.cpp:1458
#23 0x00000000009d878f in js::DefineProperty (cx=cx@entry=0x7ffff6948000, obj=..., id=..., id@entry=..., value=..., getter=<optimized out>, setter=setter@entry=0x0, attrs=80, result=...) at js/src/jsobj.cpp:2729
#24 0x00000000009dc10e in js::DefineProperty (cx=cx@entry=0x7ffff6948000, obj=..., obj@entry=..., id=..., id@entry=..., value=..., value@entry=..., getter=<optimized out>, setter=setter@entry=0x0, attrs=80) at js/src/jsobj.cpp:2760
#25 0x0000000000938b1b in DefinePropertyById (cx=cx@entry=0x7ffff6948000, obj=..., obj@entry=..., id=..., id@entry=..., value=..., get=..., set=..., attrs=80, flags=0) at js/src/jsapi.cpp:2191
#26 0x0000000000939f1f in JS_DefineProperties (cx=cx@entry=0x7ffff6948000, obj=..., obj@entry=..., ps=0x1efec10 <js::SavedFrame::protoAccessors+144>) at js/src/jsapi.cpp:3213
#27 0x0000000000b47b9e in js::GlobalObject::resolveConstructor (cx=cx@entry=0x7ffff6948000, global=..., key=key@entry=JSProto_SavedFrame) at js/src/vm/GlobalObject.cpp:238
#28 0x0000000000b48218 in js::GlobalObject::ensureConstructor (cx=cx@entry=0x7ffff6948000, global=..., global@entry=..., key=key@entry=JSProto_SavedFrame) at js/src/vm/GlobalObject.cpp:122
#29 0x0000000000b842db in js::GlobalObject::getOrCreateSavedFramePrototype (global=..., cx=0x7ffff6948000) at js/src/vm/GlobalObject.h:400
#30 js::SavedFrame::create (cx=cx@entry=0x7ffff6948000) at js/src/vm/SavedStacks.cpp:526
#31 0x0000000000b91c2e in js::SavedStacks::createFrameFromLookup (this=this@entry=0x7ffff692b0b8, cx=cx@entry=0x7ffff6948000, lookup=..., lookup@entry=...) at js/src/vm/SavedStacks.cpp:1515
#32 0x0000000000b91d9e in js::SavedStacks::getOrCreateSavedFrame (this=this@entry=0x7ffff692b0b8, cx=cx@entry=0x7ffff6948000, lookup=lookup@entry=...) at js/src/vm/SavedStacks.cpp:1502
#33 0x0000000000b93525 in js::SavedStacks::insertFrames(JSContext*, js::FrameIter&, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) (this=this@entry=0x7ffff692b0b8, cx=cx@entry=0x7ffff6948000, iter=..., frame=..., frame@entry=..., capture=capture@entry=<unknown type in /mnt/LangFuzz/work/builds/debug64/dist/bin/js, CU 0x4b59418, DIE 0x4d3f74a>) at js/src/vm/SavedStacks.cpp:1408
#34 0x0000000000b939c7 in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) (this=0x7ffff692b0b8, cx=cx@entry=0x7ffff6948000, frame=frame@entry=..., capture=capture@entry=<unknown type in /mnt/LangFuzz/work/builds/debug64/dist/bin/js, CU 0x4b59418, DIE 0x4d3fdf1>) at js/src/vm/SavedStacks.cpp:1175
#35 0x000000000093c408 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) (cx=0x7ffff6948000, stackp=..., capture=<unknown type in /mnt/LangFuzz/work/builds/debug64/dist/bin/js, CU 0x3351910, DIE 0x34f46c5>) at js/src/jsapi.cpp:7030
#36 0x000000000093c4db in CaptureStack (cx=<optimized out>, stack=...) at js/src/jsexn.cpp:361
#37 0x000000000093ff7b in js::ErrorToException (cx=0x7ffff6948000, reportp=0x7fffffffca30, callback=<optimized out>, userRef=<optimized out>) at js/src/jsexn.cpp:679
#38 0x0000000000943034 in js::ReportErrorNumberVA (cx=0x7ffff6948000, flags=flags@entry=0, callback=0x91cfa0 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=<optimized out>, ap=ap@entry=0x7fffffffcac0, argumentsType=js::ArgumentsAreLatin1) at js/src/jscntxt.cpp:888
#39 0x000000000094341d in JS_ReportErrorNumberLatin1VA (cx=<optimized out>, errorCallback=<optimized out>, userRef=<optimized out>, errorNumber=<optimized out>, ap=ap@entry=0x7fffffffcac0) at js/src/jsapi.cpp:5746
#40 0x00000000009434b8 in JS_ReportErrorNumberLatin1 (cx=cx@entry=0x7ffff6948000, errorCallback=errorCallback@entry=0x91cfa0 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=1) at js/src/jsapi.cpp:5735
#41 0x00000000009437b5 in js::ReportIsNotDefined (cx=cx@entry=0x7ffff6948000, id=id@entry=...) at js/src/jscntxt.cpp:949
#42 0x000000000094383e in js::ReportIsNotDefined (cx=0x7ffff6948000, name=..., name@entry=...) at js/src/jscntxt.cpp:958
#43 0x0000000000540300 in js::FetchName<(js::GetNameMode)0> (cx=0x7ffff6948000, receiver=..., holder=..., name=..., prop=..., vp=...) at js/src/vm/Interpreter-inl.h:187
#44 0x00000000005319d9 in js::GetEnvironmentName<(js::GetNameMode)0> (vp=..., name=..., envChain=..., cx=<optimized out>) at js/src/vm/Interpreter-inl.h:257
#45 GetNameOperation (vp=..., pc=<optimized out>, fp=<optimized out>, cx=<optimized out>) at js/src/vm/Interpreter.cpp:217
#46 Interpret (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:3097
#47 0x0000000000535932 in js::RunScript (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:394
#48 0x00000000005384d1 in js::ExecuteKernel (cx=cx@entry=0x7ffff6948000, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:677
#49 0x0000000000538898 in js::Execute (cx=cx@entry=0x7ffff6948000, script=script@entry=..., envChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:710
#50 0x0000000000931fd8 in ExecuteScript (cx=cx@entry=0x7ffff6948000, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x0) at js/src/jsapi.cpp:4473
#51 0x000000000093220d in JS_ExecuteScript (cx=cx@entry=0x7ffff6948000, scriptArg=scriptArg@entry=...) at js/src/jsapi.cpp:4506
#52 0x000000000042c548 in RunFile (compileOnly=false, file=0x7ffff4325400, filename=<optimized out>, cx=0x7ffff6948000) at js/src/shell/js.cpp:680
#53 Process (cx=cx@entry=0x7ffff6948000, filename=<optimized out>, forceTTY=forceTTY@entry=false, kind=kind@entry=FileScript) at js/src/shell/js.cpp:1124
#54 0x0000000000437dc9 in ProcessArgs (op=0x7fffffffda30, cx=0x7ffff6948000) at js/src/shell/js.cpp:7617
#55 Shell (envp=<optimized out>, op=0x7fffffffda30, cx=0x7ffff6948000) at js/src/shell/js.cpp:7977
#56 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8369
rax	0xfffe4b4b4b4b4b4b	-480163195565237
rbx	0x7fffffffa8f0	140737488333040
rcx	0x3	3
rdx	0x0	0
rsi	0x7ffff6982a50	140737330555472
rdi	0x7fffffffa920	140737488333088
rbp	0x7fffffffaa00	140737488333312
rsp	0x7fffffffa860	140737488332896
r8	0x4	4
r9	0x7ffff6a00138	140737331069240
r10	0x7ffff691e040	140737330143296
r11	0x246	582
r12	0x7ffff692b190	140737330196880
r13	0xffffffffffffff	72057594037927935
r14	0x7fffffffa920	140737488333088
r15	0x2	2
rip	0x998fbd <js::gc::GCRuntime::markCompartments()+1661>
=> 0x998fbd <js::gc::GCRuntime::markCompartments()+1661>:	mov    0x10(%rax),%r12
   0x998fc1 <js::gc::GCRuntime::markCompartments()+1665>:	jmpq   0x998d56 <js::gc::GCRuntime::markCompartments()+1046>

I assume this is shell only and has to do with nukeCCW interactions in the shell, but marking s-s until triaged, due to use-after-free.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/25ce82ad3b4b
user:        Brad Lassey
date:        Thu Feb 23 16:02:50 2017 -0500
summary:     bug 1341414 - reverting change to ContentPrefs.cpp to fix orange a11y tests r=orange

This iteration took 267.854 seconds to run.
Crash Signature: [@ JSObject::compartment] → [@ JSObject::compartment] [@ js::gc::GCRuntime::markCompartments]
Summary: Crash [@ JSObject::compartment] with nukeCCW → Crash [@ JSObject::compartment] or Crash [@ js::gc::GCRuntime::markCompartments] with nukeCCW
Probably not the right bisect, test might be intermittent. NI for evilpies for implementing nukeCCW :)
Flags: needinfo?(evilpies)
nukeCCW is just exposing something that exists in the browser. This doesn't immediately seem related to my CCW optimization in CacheIR.
Flags: needinfo?(evilpies) → needinfo?(jcoppeard)
Steve, do you have some spare cycles to look into this?
Flags: needinfo?(jcoppeard) → needinfo?(sphink)
What's happening here is that we have a CCW wrapper -> obj. The crossCompartmentWrappers table in the main compartment contains an entry mapping obj to wrapper. When we nuke the CCW, wrapper no longer points to obj in any way, but we still have the crossCompartmentWrappers entry. We then do a GC slice and sweep the wrapper map, but only in wrapper's zone. We keep the wrapper map entry, because obj is not about to be finalized (it's in the other zone, which is currently marking). 

Next we sweep obj's zone, find obj is dead, and discard it (and its arena). But it's still in wrapper->zone()'s wrapper map.

So when we next sweep the wrapper map, we access obj to see if it's about to be finalized. UAF.
I guess another way of describing what's going wrong is that the nukeCCW call severs the connection that causes the two zones to be in the same zone sweeping group, thus allowing a dependent zone to be swept first. I can think of a couple of possible fixes. I'll have to try them out to see what works best.
(In reply to Steve Fink [:sfink] [:s:] from comment #6)

Oh wow, this sounds bad.

> What's happening here is that we have a CCW wrapper -> obj. The
> crossCompartmentWrappers table in the main compartment contains an entry
> mapping obj to wrapper. When we nuke the CCW, wrapper no longer points to
> obj in any way, but we still have the crossCompartmentWrappers entry. We
> then do a GC slice and sweep the wrapper map, but only in wrapper's zone. We
> keep the wrapper map entry, because obj is not about to be finalized (it's
> in the other zone, which is currently marking).

This is normally fine.  We would sweep the entry if either the wrapper or obj were about to be finalized, and because we trace obj when we trace the wrapper, it doesn't matter that obj is in a different zone group and won't ever appear to be about to die at this point -- we just remove the entry when the wrapper dies.  The problem is nuking cuts that link that traces obj so the wrapper and obj can now die independently.
(In reply to Jon Coppeard (:jonco) from comment #8)
> (In reply to Steve Fink [:sfink] [:s:] from comment #6)
> 
> Oh wow, this sounds bad.
> 
> > What's happening here is that we have a CCW wrapper -> obj. The
> > crossCompartmentWrappers table in the main compartment contains an entry
> > mapping obj to wrapper. When we nuke the CCW, wrapper no longer points to
> > obj in any way, but we still have the crossCompartmentWrappers entry. We
> > then do a GC slice and sweep the wrapper map, but only in wrapper's zone. We
> > keep the wrapper map entry, because obj is not about to be finalized (it's
> > in the other zone, which is currently marking).
> 
> This is normally fine.  We would sweep the entry if either the wrapper or
> obj were about to be finalized, and because we trace obj when we trace the
> wrapper, it doesn't matter that obj is in a different zone group and won't
> ever appear to be about to die at this point -- we just remove the entry
> when the wrapper dies.  The problem is nuking cuts that link that traces obj
> so the wrapper and obj can now die independently.

Right, that was the "wrapper no longer points to obj in any way" part. Things I was thinking of trying, in current order of preference:

 - when nuking, explicitly remove the wrapper from the wrapper map

 - when nuking, leave the old target in place instead of nulling it out. We would still be swapping the handler to a DeadObjectProxy handler. But it would have to trace the target weakly to avoid keeping the target compartment alive.

 - store a zone -> zone edge somewhere else when nuking, so that they end up in the same sweep group.

I'm hoping that first one will work out. :-)
Component: JavaScript Engine → JavaScript: GC
Flags: needinfo?(sphink)
It fixes the problem and didn't seem to immediately introduce any problems, so putting up for review. Semantically, it makes sense to me -- if you're nuking a CCW so that it is no longer associated with the target object at all, then what is it doing in the wrapper map indexed by that target object? In fact, it makes me wonder if it is observable -- if you nuke a CCW, then recreate the wrapper, you would get the old wrapper until a GC and the new wrapper after. (I'm guessing in the browser this wouldn't be observable because when you're nuking things, you won't have any further opportunities to recreate wappers.)

Perhaps https://treeherder.mozilla.org/#/jobs?repo=try&revision=017a83e3ccaa05dceb047fbee1371990722597d0 will reveal something.
Attachment #8843010 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Added the test.

And never mind that last comment; I attempted to write a test for the changing-wrapper thing, but realized there's no way to have an object swept and then create a new wrapper for it. Duh.
Attachment #8843014 - Flags: review?(jcoppeard)
Attachment #8843010 - Attachment is obsolete: true
Attachment #8843010 - Flags: review?(jcoppeard)
(In reply to Steve Fink [:sfink] [:s:] from comment #10)
Don't we want the object to wrap to a dead object wrapper if we try to wrap it again before it dies? 

I think your third idea would be doable by adding an extra edge in JSCompartment::findOutgoingEdges if the wrapper is a dead object proxy (you'd have to add it to gcZoneGroupEdges because it would be going in the opposite direction to what we want here).  This might have the effect of lumping tons of zones together into the same zone group whenever we nuke anything but it should at least fix the problem.
(In reply to Jon Coppeard (:jonco) from comment #12)
> (In reply to Steve Fink [:sfink] [:s:] from comment #10)
> Don't we want the object to wrap to a dead object wrapper if we try to wrap
> it again before it dies? 

Oh... I didn't think of that. My solution breaks wrapper identity in the *normal* case. That's not good.

  g = newGlobal();
  wrapper = g.eval("wrapped = { name: 'Bob' }");
  m = new Map();
  m.set(wrapper, "found it!");
  m.get(g.wrapped); // found it!
  nukeCCW(wrapper);
  m.get(g.wrapped); // this should be "found it!", with my patch it is not

> I think your third idea would be doable by adding an extra edge in
> JSCompartment::findOutgoingEdges if the wrapper is a dead object proxy
> (you'd have to add it to gcZoneGroupEdges because it would be going in the
> opposite direction to what we want here).  This might have the effect of
> lumping tons of zones together into the same zone group whenever we nuke
> anything but it should at least fix the problem.

Yeah, I guess we have all of the information we need right there.
This fixes the bug. I'm not crazy about adding in an extra unconditional pass through all the wrapper maps. Perhaps I should add a bit to the compartment saying whether it contains any dead object proxies? I'm not too sure when to clear it, though.
Attachment #8843114 - Flags: review?(jcoppeard)
Attachment #8843014 - Attachment is obsolete: true
Attachment #8843014 - Flags: review?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #12)
> I think your third idea would be doable by adding an extra edge in
> JSCompartment::findOutgoingEdges if the wrapper is a dead object proxy
> (you'd have to add it to gcZoneGroupEdges because it would be going in the
> opposite direction to what we want here).  This might have the effect of
> lumping tons of zones together into the same zone group whenever we nuke
> anything but it should at least fix the problem.

Oh wait. I thought findOutgoingEdges was too late -- if a zone added an outgoing edge to an already-processed zone, it would get missed. Am I wrong about that?

On second (third) reading, now I'm thinking you're saying the same thing, you're just saying that findOutgoingEdges is the right time *if* it has already been added to gcZoneGroupEdges. I'm just really worried about the perf here...
(In reply to Steve Fink [:sfink] [:s:] from comment #14)
> Perhaps I should add a bit to the
> compartment saying whether it contains any dead object proxies? I'm not too
> sure when to clear it, though.

Even if you never clear, it should still avoid overhead in a lot of cases. Would be good to instrument the browser to check this.
(In reply to Jan de Mooij [:jandem] from comment #16)
> Even if you never clear, it should still avoid overhead in a lot of cases.
> Would be good to instrument the browser to check this.

(Because we nuke chrome -> content wrappers so content compartments shouldn't have dead object proxies.)
(In reply to Steve Fink [:sfink] [:s:] from comment #15)

> Oh wait. I thought findOutgoingEdges was too late -- if a zone added an
> outgoing edge to an already-processed zone, it would get missed. Am I wrong
> about that?

You're right, it is.  I didn't think of that.

>  I'm not crazy about adding in an extra unconditional pass through all the wrapper maps.

Nor me.  What we could do is to add all edges to gcZoneGroup edges and then process that at the end.  If we hit OOM we fall back to using a single zone group already so that's not an issue.

> Perhaps I should add a bit to the compartment saying whether it contains any dead object proxies?

That also sounds like it would work.
(In reply to Jan de Mooij [:jandem] from comment #17)
> (In reply to Jan de Mooij [:jandem] from comment #16)
> > Even if you never clear, it should still avoid overhead in a lot of cases.
> > Would be good to instrument the browser to check this.
> 
> (Because we nuke chrome -> content wrappers so content compartments
> shouldn't have dead object proxies.)

Oh. Never mind, I just wasn't thinking straight.

There's an obvious place to clear the zone flag -- when adding stuff to gcZoneEdges, we're already iterating over all suspect zones, so can easily clear the flag there if no dead proxies are found.

I'll do a new patch.
Attachment #8843114 - Flags: review?(jcoppeard)
Ok, I think this version should fix the perf problem. If you don't have any dead proxies, this will add a single boolean check to each zone. If you do, you only have to scan the zones that contain dead proxies. And I would assume that most of the time, when you nuke CCWs, you'll be collecting the target compartment soon after so the flag will go back to being false.

I can certainly see an argument for making this more granular, at a compartment level so you don't have to scan all the compartments. Let me know if you think it's worth the incremental complexity. (It's no more complex than the zone version, it's just that you'd have two flags for a niche case, since I'd rather not eliminate the zone one -- it keeps us from iterating over the compartments most of the time.)
Attachment #8843337 - Flags: review?(jcoppeard)
Attachment #8843114 - Attachment is obsolete: true
Comment on attachment 8843337 [details] [diff] [review]
dead object proxies must be swept with their former targets

Review of attachment 8843337 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

I think this is fine as it is.

::: js/src/jsgc.cpp
@@ +4450,5 @@
>  
> +bool
> +JSCompartment::findDeadProxyZoneEdges(bool* foundAny)
> +{
> +    *foundAny = false;

This could use a comment to explain why we need to do this.
Attachment #8843337 - Flags: review?(jcoppeard) → review+
Comment on attachment 8843337 [details] [diff] [review]
dead object proxies must be swept with their former targets

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e08cf37f29

Argh. Sorry, I just landed this prematurely.

Retroactive sec-approval:

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

difficult

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The test points pretty clearly to how to trigger the bug (to get a UAF crash). Knowing how to take advantage of it would require first knowing that GC sweeping is a problematic area from a security perspective, and then you'd need a pretty good level of familiarity with our zone sweep groups and things. It doesn't give a very direct handle on how to trigger it. And the test case is of limited help in an actual browser, because it'd be hard to trigger just the right pattern of incremental GC slices. But not impossible.

Which older supported branches are affected by this flaw?

all. It became testable in the shell in bug 1319087, but it has existed since the fabled Hueyfix.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I wouldn't be surprised if there were minor textual differences that mess up the backports, but the code change should be pretty much identical.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely. It only has an effect when you nuke CCWs, and the main regression it can cause is lumping more zones together when sweeping, which could increase max pause times (but only when closing tabs down, so it wouldn't be very common and the change shouldn't be very big anyway.)
Attachment #8843337 - Flags: sec-approval?
The CCW nuking is also only triggered in the browser when you close a tab, so you'd somehow need user interaction timed very precisely with incremental GC. So I doubt this is really exploitable in any reliable way in practice.
https://hg.mozilla.org/mozilla-central/rev/b0e08cf37f29
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
We'll need this on branches affected as well. I really wish we hadn't checked in a test for a UAF bug. Even if sec-approval+ had been given, the test wouldn't have been approved until after we shipped the fix public.
Attachment #8843337 - Flags: sec-approval? → sec-approval+
Hi :sfink,
Since this bug also affects 54, do you think it's worth uplifting to 54 if this patch is not too risky?
Flags: needinfo?(sphink)
Please request uplifts to everything we can (including 52esr), and also put on the list as a possible point-fix or ride-along for 52
I'm discussing with Dan about whether we need to do a point release because of this.
Flags: needinfo?(dveditz)
Andrew thinks this is almost a sec-moderate though.
This requires that the user close a tab in the middle of an incremental GC, so it feels like it would be almost impossible to create an exploit of this.
Well, maybe pages can close windows like this, too. But the timing would still be difficult.
Flags: in-testsuite+
Comment on attachment 8843337 [details] [diff] [review]
dead object proxies must be swept with their former targets

[Approval Request Comment]
User impact if declined: (very) theoretical exploit
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): potential slight increase in GC pause times.
String or UUID changes made by this patch: none

[Feature/Bug causing the regression]: I think it's the Hueyfix where we started nuking cross compartment wrappers
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no. I can't think of a way to reliably trigger this. And I have evidence that it has ever happened in production.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: It's a change to a tricky portion of the GC code, which is fundamentally somewhat risky. But its only effect is to make things slightly safer, and the code is restricted to doing things when tabs are closed.

Personally, I wouldn't do a point release -- no known exploits, no known ways to exploit, and it seems like it's a pretty tough one to exploit. It's a UAF, but the user is the GC sweeping code when it's looking at the closed compartment, so you'd have to run a bunch of JS code in a tab that you've already closed, and getting the timing right for it to happen would be difficult. The usual result would be a crash, and I haven't seen any matching signatures in crash reports (nor can I find any matching bugs when I do a bug search).

I'm not sure about the general tradeoffs with respect to esrs, so I don't know whether it's worth backporting to those or not.
Flags: needinfo?(sphink)
Attachment #8843337 - Flags: approval-mozilla-release?
Attachment #8843337 - Flags: approval-mozilla-esr52?
Attachment #8843337 - Flags: approval-mozilla-esr45?
Attachment #8843337 - Flags: approval-mozilla-beta?
Attachment #8843337 - Flags: approval-mozilla-aurora?
Comment on attachment 8843337 [details] [diff] [review]
dead object proxies must be swept with their former targets

Fix a security issue and crash. Aurora54+.
Attachment #8843337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8843337 [details] [diff] [review]
dead object proxies must be swept with their former targets

Sec-high issue, approving for all branches except release. JCristau will manage the 52 dot release and whether this needs to go as a ride-along.
Attachment #8843337 - Flags: approval-mozilla-esr52?
Attachment #8843337 - Flags: approval-mozilla-esr52+
Attachment #8843337 - Flags: approval-mozilla-esr45?
Attachment #8843337 - Flags: approval-mozilla-esr45+
Attachment #8843337 - Flags: approval-mozilla-beta?
Attachment #8843337 - Flags: approval-mozilla-beta+
Needs a rebased patch for Beta.
Flags: needinfo?(sphink)
Backport for beta. Not sure if you want me to set flags on this.

I haven't checked, but there may be another conflict when backporting to something before cacheir, since then the test will not exist. (It's fine to drop the test in that case.)
And the patch for esr45. Note that this depends on a backport of the nukeCCW test patch from bug 1319087. It requires a simple change (remove the ASCII from a function name), but I'll upload it here too just in case.
Group: javascript-core-security → core-security-release
Setting qe-verify- based on Steve's assessment on manual testing needs (see Comment 33) and the fact that this fix has automated coverage.
Flags: qe-verify-
Clearing the needinfo for sfink: as far as I can see comment 38 answers it.

I'm with Steve in comment 33: this issue doesn't need a point release. I'd really rather not mess with GC without beta testing time unless there's a demonstrable emergency.
Flags: needinfo?(sphink)
Flags: needinfo?(dveditz)
Comment on attachment 8843337 [details] [diff] [review]
dead object proxies must be swept with their former targets

release52- per comments 33 and 43.
Attachment #8843337 - Flags: approval-mozilla-release? → approval-mozilla-release-
Depends on: 1350844
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main53+][adv-esr48.9+][adv-esr52.1+]
Whiteboard: [jsbugmon:][adv-main53+][adv-esr48.9+][adv-esr52.1+] → [jsbugmon:][adv-main53+][adv-esr45.9+][adv-esr52.1+]
Depends on: 1363229
Depends on: 1357022
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.