1557343 - Crash [@ JS::shadow::Realm::compartment] or various GC crashes/use-after-poison with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 155a7e2117e5 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

function loadFile(lfVarx) {
  eval(lfVarx);
}
var g = newGlobal({newCompartment: true});
g.parent = this;
g.hits = 0;
g.eval("new Debugger(parent).onExceptionUnwind = function () { hits++; };");
loadFile(`
  function* g1() {}
  var o = g1();
  function* g3() {
    while (x && 0) {}
  }
  o = g3();
  try { v = o.next(); } catch(exc) {}
`)
loadFile(`
  L: do {    } while (
    class MyArrayBuffer extends ArrayBuffer {}
  );
`);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555605a5a6 in JS::shadow::Realm::compartment (this=<optimized out>) at dist/include/js/Realm.h:50
#1  JS::GetCompartmentForRealm (realm=<optimized out>) at dist/include/js/Realm.h:60
#2  js::ObjectGroup::compartment (this=0xfffe4b4b4b4b4b4b) at js/src/vm/ObjectGroup.h:212
#3  JSObject::compartment (this=<optimized out>) at js/src/vm/JSObject.h:159
#4  JSObject::maybeCompartment (this=<optimized out>) at js/src/vm/JSObject.h:160
#5  _ZZN2js19CrossCompartmentKey11compartmentEvENKUlT_E_clIPPNS_12NativeObjectEEEDaS1_ (__closure=<optimized out>, tp=<optimized out>) at js/src/vm/Compartment.h:226
#6  _ZN2js19CrossCompartmentKey21ApplyToWrappedMatcherIZNS0_11compartmentEvEUlT_E_EclINS_12NativeObjectEEEDaRNS0_8DebuggeeIS2_EE (this=<optimized out>, dbg=...) at js/src/vm/Compartment.h:185
[...]
#18 js::CrossCompartmentKey::compartment (this=<optimized out>) at js/src/vm/Compartment.h:226
#19 js::gc::GCRuntime::markCompartments (this=this@entry=0x7ffff5f1c6a8) at js/src/gc/GC.cpp:4562
#20 0x000055555605b171 in js::gc::GCRuntime::beginMarkPhase (this=0x7ffff5f1c6a8, reason=<optimized out>, session=...) at js/src/gc/GC.cpp:4496
#21 0x000055555605d48b in js::gc::GCRuntime::incrementalSlice (this=this@entry=0x7ffff5f1c6a8, budget=..., reason=reason@entry=JS::GCReason::INCREMENTAL_ALLOC_TRIGGER, session=...) at js/src/gc/GC.cpp:7186
#22 0x000055555605e083 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f1c6a8, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::GCReason::INCREMENTAL_ALLOC_TRIGGER) at js/src/gc/GC.cpp:7628
#23 0x000055555605e72c in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f1c6a8, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::GCReason::INCREMENTAL_ALLOC_TRIGGER) at js/src/gc/GC.cpp:7808
#24 0x000055555605eee2 in js::gc::GCRuntime::startGC (this=0x7ffff5f1c6a8, gckind=GC_NORMAL, reason=JS::GCReason::INCREMENTAL_ALLOC_TRIGGER, millis=0) at js/src/gc/GC.cpp:7907
#25 0x000055555605f158 in js::gc::GCRuntime::gcIfRequested (this=0x7ffff5f1c6a8) at js/src/gc/GC.cpp:8097
#26 0x0000555555c218f6 in HandleInterrupt (cx=<optimized out>, invokeCallback=false) at js/src/vm/Runtime.cpp:413
#27 0x00003f6c1b7edf7d in ?? ()
[...]
#36 0x0000000000000000 in ?? ()
rax	0xfffe4b4b4b4b4b4b	-480163195565237
rbx	0x7fffffffa5a0	140737488332192
rcx	0x7ffff58fe940	140737313237312
rdx	0x8	8
rsi	0x7ffff58fe940	140737313237312
rdi	0x7fffffffa5f8	140737488332280
rbp	0x7fffffffa6a0	140737488332448
rsp	0x7fffffffa520	140737488332064
r8	0x0	0
r9	0x6e	110
r10	0x0	0
r11	0x246	582
r12	0x7fffffffa560	140737488332128
r13	0x7fffffffa590	140737488332176
r14	0x7ffff5f1c6a8	140737319650984
r15	0x7ffff59f7550	140737314256208
rip	0x55555605a5a6 <js::gc::GCRuntime::markCompartments()+1030>
=> 0x55555605a5a6 <js::gc::GCRuntime::markCompartments()+1030>:	mov    0x10(%rax),%rax
   0x55555605a5aa <js::gc::GCRuntime::markCompartments()+1034>:	mov    (%rax),%r15

This is causing issues in fuzzing because it triggers frequently and looks like a real security issue (while it is not, involving Debugger). Marking as fuzzblocker.

Comment 1

[Nicolas B. Pierron [:nbp]]

Paul, do you have any way to find the right person who might look at this issue?

Comment 2

[Jason Orendorff [:jorendorff]]

Jim, this fuzzblocker is probably related to the recent Debugger changes around generator frames.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/b65e48539b71
user: Jim Blandy
date: Wed Jun 05 03:33:18 2019 +0000
summary: Bug 1551176: Add GENERATOR_INFO_SLOT to js::DebuggerFrame. r=jorendorff

Jim, is bug 1551176 a likely regressor?

Comment 4

[Jim Blandy :jimb]

Yes. I'm able to reproduce; taking.

Comment 5

[Paul Bone [:pbone]]

Sorry I didn't respond, I started looking for a regressing change but didn't find it before a public holiday.

Comment 6

[Jim Blandy :jimb]

Okay, we have an analysis for this.

Debugger creates a Debugger.Frame for a generator frame. This establishes various relations:

• Debugger::generatorFrames maps the generator call's AbstractGeneratorObject to its DebuggerFrame.
• The DebuggerFrame's GeneratorInfo is set to point to the AbstractGeneratorObject.
• Compartment::crossCompartmentWrappers includes a DebuggeeFrameGenerator mapping the Debugger and the AbstractGeneratorObject to the DebuggerFrame.

Then, the generator frame throws. This removes the first two relations above: the entry in generatorFrames is removed, and the DebuggerFrame's GeneratorInfo is removed. But the DebuggeeFrameGenerator key is left in crossCompartmentWrappers. I thought this was okay, since I couldn't find any other code in Debugger.cpp that bothers to clean up the crossCompartmentWrappers entries we create for all our other Debugger.Foo objects.

But it's not okay: if the AbstractGeneratorObject becomes garbage and the GC collects the debuggee zone but not the debugger zone, then the absence of an entry in generatorFrames means that Debugger::traceCrossCompartmentEdges isn't able to alert the GC that the debuggee's compartment's wrapper map may need to be cleaned up. Eventually GCRuntime::markCompartments tries to consult the wrapper map entry for the compartment of the wrapper's referent (the AbstractGeneratorObject), but the referent is gone.

Comment 7

[Jim Blandy :jimb]

I have a fix, but really, since those three relations should always be in sync, the code should reflect that requirement, so I need to rearrange things a little bit. Otherwise we're just kicking the can down the road until OOM fuzzing finds some way to make the three come out of sync again.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Jim Blandy :jimb from comment #6)
If I understand correctly there is still an entry in the wrapper map for the AbstractGeneratorObject, yet it is collected and the entry ends up containing a dangling pointer. I think this is wrong. It doesn't work this way for non-debugger wrappers. In that case the GC conservatively marks the referents of wrappers in zones that are not being collected.

Comment 9

[Jim Blandy :jimb]

Here's a slightly simpler test case; run with --no-ggc:

var g = newGlobal({ newCompartment: true });
g.eval(`
  function* g3() {
    debugger;
  }

  function next() {
    g3().next();
  }

  function ggg() {
    gc(gc);
  }
`);

var dbg = new Debugger(g);
var frame;
dbg.onDebuggerStatement = f => {
  frame = f;
};

g.next();
g.ggg();

Comment 10

[Jim Blandy :jimb]

The test case in comment 0 takes about thirty seconds to run, whereas the above has no loops. So I think we should not land the original test case.

Comment 11

[Jim Blandy :jimb]

Attachment: Bug 1557343: Move AutoRealm outside addGeneratorFrame. r=jorendorff

Without this patch, addGeneratorFrame may be called from any realm, and enters
the debugger's realm to call DebuggerFrame::setGenerator. However, we would like
to merge addGeneratorFrame and setGenerator, and call the combined from various
points which are already in the debugger's realm, so it would be a little nicer
to simply make the function assume it is called from the debugger's realm.

Comment 12

[Jim Blandy :jimb]

Attachment: Bug 1557343: Replace the sole use of DebuggerWeakMap::removeIf with its definition. r=jorendorff

Later in this patch series, we will be gathering up all the code to manage the
association between DebuggerFrame and AbstractGeneratorFrame objects into a pair
of functions, one to establish a relation and the other to tear it down. The
removeif method combines iteration and entry removal, but we would rather have
entry removal live next to the code that tears down the rest of the association.

In preparation for that, this changeset replaces the sole use of removeIf with
its (not very large) definition, so that the entry removal can be more readily
moved into another function.

Depends on D35605

Comment 13

[Jim Blandy :jimb]

Attachment: Bug 1557343: Gather the code to manage the association between DebuggerFrames and AbstractGeneratorObjects. r?jorendorff

Merge Debugger::addGeneratorFrame into DebuggerFrame::setGenerator, and expand
the role of clearGenerator to fully undo the effect of setGenerator.

The association between a Debugger.Frame referring to a generator or async call
and the underlying generator object must be recorded in five separate places, as
a transaction: either all or present, or none are present. To ensure this is
true, this patch places sole responsibility for emplacing all those relations in
a single function (setGenerator), with another function to tear down those
relations (clearGenerator) as its inverse/antidote/complement/antagonist (in the
anatomical sense)/what-have-you.

Actually, when a Debugger.Frame is GC'd, we cannot reliably undo some of the
connections, and in fact can let the GC take care of those for us, so the
tear-down function clearGenerator is split into two overloads, one which is
suitable for use from a finalizer and the other which takes care of the entire
task.

Depends on D35606

Comment 14

[Jim Blandy :jimb]

Another way to fix this bug would be to have Compartment::traceOutgoingCrossCompartmentWrappers also treat debugger keys' referents as incoming edges, instead of considering only JSObject* keys as it does now. This would cause the AbstractGeneratorObject and JSScript to be held live, avoiding the crash. However, since the bug only occurs when the generator call has completed, and the Debugger.Frame has been cleared, there isn't actually any need for the AGO and script to be held alive.

Comment 15

[Christian Holler (:decoder)]

This bug is creating a huge amount of GC crashes in current fuzzing that are very hard to filter. Can we accelerate review/landing for this?

Comment 16

[Jason Orendorff [:jorendorff]]

Yep, I'll review it today.

Comment 17

[Jason Orendorff [:jorendorff]]

Really truly reviewing this today.

Comment 19

[Jim Blandy :jimb]

Landing queued.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1663fcb7bfd7
Move AutoRealm outside addGeneratorFrame. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/a9de28d699a7
Replace the sole use of DebuggerWeakMap::removeIf with its definition. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/70040a80a4d7
Gather the code to manage the association between DebuggerFrames and AbstractGeneratorObjects. r=jorendorff

Comment 21

[Natalia Csoregi [:nataliaCs]]

Backed out for bustages on Debugger.h

backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3c04e55d4acf82c328da941f0f4dc17592fcd45c

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=70040a80a4d720f327d1f2d6fa18e525f4bb6921&selectedJob=253566871

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253566851&repo=autoland&lineNumber=5387

[task 2019-06-26T18:50:30.697Z] 18:50:30 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/js/src/gc/Unified_cpp_js_src_gc1.cpp:11:
[task 2019-06-26T18:50:30.697Z] 18:50:30 INFO - In file included from /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:25:
[task 2019-06-26T18:50:30.698Z] 18:50:30 ERROR - /builds/worker/workspace/build/src/js/src/vm/Debugger.h:169:5: error: bad implicit conversion constructor for 'Enum'
[task 2019-06-26T18:50:30.698Z] 18:50:30 INFO - Enum(DebuggerWeakMap& map) : Base::Enum(map) {}
[task 2019-06-26T18:50:30.698Z] 18:50:30 INFO - ^
[task 2019-06-26T18:50:30.699Z] 18:50:30 INFO - /builds/worker/workspace/build/src/js/src/vm/Debugger.h:169:5: note: consider adding the explicit keyword to the constructor
[task 2019-06-26T18:50:30.699Z] 18:50:30 INFO - Enum(DebuggerWeakMap& map) : Base::Enum(map) {}
[task 2019-06-26T18:50:30.699Z] 18:50:30 INFO - ^
[task 2019-06-26T18:50:30.699Z] 18:50:30 INFO - explicit
[task 2019-06-26T18:50:30.699Z] 18:50:30 INFO - 1 error generated.
[task 2019-06-26T18:50:30.700Z] 18:50:30 INFO - /builds/worker/workspace/build/src/config/rules.mk:801: recipe for target 'Unified_cpp_js_src_gc1.o' failed
[task 2019-06-26T18:50:30.700Z] 18:50:30 ERROR - make[4]: *** [Unified_cpp_js_src_gc1.o] Error 1
[task 2019-06-26T18:50:30.701Z] 18:50:30 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/js/src/gc'
[task 2019-06-26T18:50:30.702Z] 18:50:30 INFO - make[4]: *** Waiting for unfinished jobs....

other failures: error: unused variable 'p' [-Werror,-Wunused-variable] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253566830&repo=autoland&lineNumber=1331

Comment 22

[Jim Blandy :jimb]

Haste makes waste. I'm sorry, fuzzer folk. I've got a revision for this, but I'm going to get a clean try push before I land it again.

Comment 23

[Jim Blandy :jimb]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0acc8d34f4d5f9e5461505d3788dbf47ff0068b

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b9bb81548d1
Move AutoRealm outside addGeneratorFrame. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/2b89c2511a1f
Replace the sole use of DebuggerWeakMap::removeIf with its definition. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/9cc95aad8b36
Gather the code to manage the association between DebuggerFrames and AbstractGeneratorObjects. r=jorendorff

Comment 25

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/9b9bb81548d1
https://hg.mozilla.org/mozilla-central/rev/2b89c2511a1f
https://hg.mozilla.org/mozilla-central/rev/9cc95aad8b36