1584195 - Crash [@ js::DebugScript::decrementGeneratorObserverCount] or Assertion failure: script->hasDebugScript(), at debugger/DebugScript.cpp:44

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision c31591e0b66f (build with --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

try {
  evaluate(`
    var g9 = newGlobal({newCompartment: true});
    g9.parent = this;
    g9.eval(\`
      var dbg = new Debugger(parent);
      dbg.onEnterFrame = frame => {};
    \`);
    function lfPromise(x) {
        return new Promise(resolve => {});
    }
    async function lfAsync() {
        await lfPromise();
    } lfAsync();
  `);
  gczeal(20, 2);
  evaluate(`
    async function lfAsync() {} lfAsync();
  `);
  gczeal(12, 7);
  drainJobQueue();
  evaluate(`
    new { ...  v22 => { 
  `);
} catch (exc) {}
evaluate("");

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::DebugScript::decrementGeneratorObserverCount (fop=0x7fffffffd270, script=0xb85beaa27c0) at js/src/debugger/DebugScript.cpp:264
#1  0x0000555555cba31c in js::DebuggerFrame::clearGenerator (this=this@entry=0xb85beab5100, fop=fop@entry=0x7fffffffd270) at js/src/debugger/Frame.cpp:387
#2  0x0000555555cba430 in js::DebuggerFrame::finalize (fop=0x7fffffffd270, obj=0xb85beab5100) at js/src/debugger/Frame.cpp:1074
#3  0x0000555555e62230 in JSClass::doFinalize (this=<optimized out>, obj=0xb85beab5100, fop=0x7fffffffd270) at dist/include/js/Class.h:835
#4  JSObject::finalize (fop=0x7fffffffd270, this=<optimized out>) at js/src/vm/JSObject-inl.h:69
#5  js::gc::Arena::finalize<JSObject> (thingSize=96, thingKind=js::gc::AllocKind::OBJECT8, fop=<optimized out>, this=0xb85beab5000) at js/src/gc/GC.cpp:470
#6  FinalizeTypedArenas<JSObject> (fop=<optimized out>, src=<optimized out>, dest=..., thingKind=thingKind@entry=js::gc::AllocKind::OBJECT8, budget=...) at js/src/gc/GC.cpp:526
#7  0x0000555555e644bb in FinalizeArenas (fop=0x7fffffffd270, src=src@entry=0x7ffff4ccf4f0, dest=..., thingKind=thingKind@entry=js::gc::AllocKind::OBJECT8, budget=...) at js/src/gc/GC.cpp:554
#8  0x0000555555e6692f in js::gc::ArenaLists::foregroundFinalize (this=0x7ffff4ccf0e0, fop=<optimized out>, thingKind=js::gc::AllocKind::OBJECT8, sliceBudget=..., sweepList=...) at js/src/gc/GC.cpp:5536
#9  0x0000555555e66bca in js::gc::GCRuntime::finalizeAllocKind (this=0x7ffff5f254f0, fop=<optimized out>, budget=...) at js/src/gc/GC.cpp:5834
#10 0x0000555555e96c99 in sweepaction::SweepActionForEach<ContainerIter<mozilla::EnumSet<js::gc::AllocKind, unsigned long> >, mozilla::EnumSet<js::gc::AllocKind, unsigned long> >::run (this=0x7ffff5f3c0b0, args=...) at js/src/gc/GC.cpp:6060
#11 0x0000555555e8caee in sweepaction::SweepActionSequence::run (this=0x7ffff5f1d480, args=...) at js/src/gc/GC.cpp:6025
#12 0x0000555555e96ad6 in sweepaction::SweepActionForEach<js::gc::SweepGroupZonesIter, JSRuntime*>::run (this=0x7ffff5f1d500, args=...) at js/src/gc/GC.cpp:6060
#13 0x0000555555e8caee in sweepaction::SweepActionSequence::run (this=0x7ffff5f1d540, args=...) at js/src/gc/GC.cpp:6025
#14 0x0000555555e96969 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run (this=0x7ffff5f21190, args=...) at js/src/gc/GC.cpp:6060
#15 0x0000555555e6f050 in js::gc::GCRuntime::performSweepActions (this=this@entry=0x7ffff5f254f0, budget=...) at js/src/gc/GC.cpp:6193
#16 0x0000555555e7e315 in js::gc::GCRuntime::incrementalSlice (this=this@entry=0x7ffff5f254f0, budget=..., gckind=..., reason=reason@entry=JS::GCReason::FINISH_GC, session=...) at js/src/gc/GC.cpp:6724
#17 0x0000555555e7eaf6 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f254f0, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., gckind=..., reason=reason@entry=JS::GCReason::FINISH_GC) at js/src/gc/GC.cpp:7135
#18 0x0000555555e7eeb9 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f254f0, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., gckindArg=..., reason=reason@entry=JS::GCReason::FINISH_GC) at js/src/gc/GC.cpp:7321
#19 0x0000555555e7fa80 in js::gc::GCRuntime::finishGC (this=0x7ffff5f254f0, reason=reason@entry=JS::GCReason::FINISH_GC) at js/src/gc/GC.cpp:7436
#20 0x0000555555e80263 in JS::FinishIncrementalGC (cx=cx@entry=0x7ffff5f22000, reason=reason@entry=JS::GCReason::FINISH_GC) at js/src/gc/GC.cpp:8251
#21 0x0000555555e80296 in js::gc::FinishGC (cx=0x7ffff5f22000, reason=JS::GCReason::FINISH_GC) at js/src/gc/GC.cpp:7618
#22 0x0000555555879aa9 in CancelOffThreadJobsForRuntime (cx=0x7ffff5f22000) at js/src/shell/js.cpp:407
#23 <lambda()>::operator() (__closure=<synthetic pointer>) at js/src/shell/js.cpp:11448
#24 mozilla::ScopeExit<main(int, char**, char**)::<lambda()> >::~ScopeExit (this=<synthetic pointer>, __in_chrg=<optimized out>) at dist/include/mozilla/ScopeExit.h:110
#25 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11449
rax	0x0	0
rbx	0xb85beaa27c0	12669057378240
rcx	0x3	3
rdx	0x1d065f7c	486956924
rsi	0x8	8
rdi	0xb85beaa27c0	12669057378240
rbp	0x7fffffffd270	140737488343664
rsp	0x7fffffffce60	140737488342624
r8	0x7ffff4dfd420	140737301697568
r9	0x0	0
r10	0x20	32
r11	0x1d	29
r12	0x7fffffffd270	140737488343664
r13	0xfff9800000000000	-1829587348619264
r14	0xb85beab5000	12669057454080
r15	0x60	96
rip	0x555555cb45b4 <js::DebugScript::decrementGeneratorObserverCount(JSFreeOp*, JSScript*)+20>
=> 0x555555cb45b4 <js::DebugScript::decrementGeneratorObserverCount(JSFreeOp*, JSScript*)+20>:	subl   $0x1,(%rax)
   0x555555cb45b7 <js::DebugScript::decrementGeneratorObserverCount(JSFreeOp*, JSScript*)+23>:	cmpq   $0x0,(%rax)

Comment 1

[Fuzzing Team]

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/8a7c8f8ecd56
user:        Jim Blandy
date:        Mon Jun 10 20:06:34 2019 +0000
summary:     Bug 1539654: Ensure generator scripts observed by Debugger.Frames are marked as debuggees. r=jorendorff

This iteration took 389.842 seconds to run.

Comment 2

[Jim Blandy :jimb]

I can reproduce this.

Comment 3

[Jim Blandy :jimb]

A JSScript is being finalized before the Debugger.Frame that refers to it. Debugger.Frame tries to recognize this by calling IsAboutToBeFinalized, but that test returns false, apparently because the JSScript and Debugger.Frame are finalized in different slices.

Comment 4

[Jim Blandy :jimb]

Confirmed in RR:

• JSScript::finalize is called on a JSScript pointed to by a DebuggerFrame's GeneratorInfo structure. Neither the script nor the frame's mark bits are set.
• The script's mark bit is set by js::gc::GCRuntime::finalizeAllocKind, called from js::gc::GCRuntime::gcIfNeededAtAllocation, called from JSScript::New, which is allocating a nearby arena slot.
• The Debugger.Frame is finalized, calls IsAboutToBeFinalized to see if the JSScript needs its observer count dropped, and incorrectly concludes its JSScript is still alive.

So we have mutator code running between two finalize calls, allocating in an arena pointed to by the second object to be finalized.

Comment 5

[Jim Blandy :jimb]

Flagging as potentially S-S, because although at the moment our only known means for reproduction do involve the debugger, I can't yet see why there can't be other ways to reproduce that do not. Hopefully this is a false alarm, though, because if it were really as simple as that I think we'd be running into the problem more often.

Comment 6

[Jim Blandy :jimb]

Both the JSScript and the DebuggerFrame are in zones for which isGCSweeping returns true, and their gcSweepGroupIndex values are both 1.

Comment 7

[Jim Blandy :jimb]

Hi, Steve. Does the sequence described in comment 4 seem like it should be possible, given the state in comment 6?

Comment 8

[Jim Blandy :jimb]

I don't see anything in the path from JSScript::New to Arena::arenaAllocatedDuringGC that would detect whether finalization is complete.

If it makes a difference, the GC that finalizes the Debugger.Frame is called from js::gc::FinishGC as the shell is shutting down. That's unusual. But it seems to me the bug has already occurred by that point.

Comment 9

[Jim Blandy :jimb]

Okay, jonco and I talked about this on IRC. We're pretty sure it's not S-S, but Searchfox is down at the moment, so it's a bit hard to check, so we're going to leave the S-S marking on it for now and do the evaluation tomorrow.

There's a comment in ArenaLists::foregroundFinalize that suggests that present code ought to work. It says:

  // Empty arenas are not released until all foreground finalized GC things in
  // the current sweep group have been finalized.  This allows finalizers for
  // other cells in the same sweep group to call IsAboutToBeFinalized on cells
  // in this arena.

However, this comment isn't correct. We do not finish all finalization before releasing arenas for new allocation. Rather, each alloc kind's arenas are released as soon as we finish sweeping that alloc kind.

A Debugger.Frame's pointer to its generator object's JSScript is apparently the first case we've had of an edge between two different kinds that is sensitive to finalization order (due to DebuggerFrame::clearGenerator's use of IsAboutToBeFinalized), so the debugger generator support was the first time it has mattered when the arenas get released for reuse. If there are other cases, those could run into the same problem, but we are pretty sure there aren't.

Comment 10

[Jim Blandy :jimb]

Attachment: Bug 1584195: Break Debugger.Frame -> generator JSScript edges before finalization. r?jonco

When a Debugger.Frame refers to a generator or async function call, the script
must be compiled with instrumentation to notify the Debugger API when the call
is resumed. To accomplish this, a generator's JSScript's DebugScript's
generatorObserverCount tracks the number of Debugger.Frames referring to calls
running the script. When the count is non-zero, instrumentation is required.

When a Debugger.Frame for a suspended generator call is garbage collected, its
JSScript's generatorObserverCount must be decremented. However, if the JSScript
is alo being garbage collected, it may have already been finalized, and should
not be touched.

The prior code had js::DebuggerFrame::finalize use IsAboutToBeFinalized to check
whether the JSScript was healthy enough to have its count decremented. Since the
garbage collector always handles debuggers and debuggees in the same GC cycle,
it was believed that the JSScript's mark bit (what IsAboutToBeFinalized checks)
would be accurate.

Unfortunately, it is possible for a JSScript to be finalized in one GC slice,
and then for the mutator to run, and then for the DebuggerFrame to be finalized
in the next slice, with the JSScript's arena available for new allocations in
the middle. When an arena is made available for allocation during an incremental
GC, all its mark bits are set. As a result, the DebuggerFrame finalizer believes
that the JSScript it was referring to is still alive, even thought it has been
finalized. IsAboutToBeFinalized is only safe to use on edges between cells of
the same alloc kind, since we do promise to run all finalizers for a given alloc
kind before putting those arenas up for reuse. It's not safe to use
IsAboutToBeFinalized to inspect edges between cells of differing alloc kinds,
like DebuggerFrame JSObjects and generator JSScripts.

Fortunately, there is another approach we can take. The garbage collector calls
DebugAPI::sweepAll after all marking is done, but before any finalization
takes place. At this point, all cells' mark bits are accurate (assuming they're
in a zone being GC'd), but nothing has been finalized. We can disconnect
unmarked DebuggerFrames from their scripts now, knowing that both parties are
still fully initialized.

Comment 11

[Steve Fink [:sfink] [:s:]]

jonco: This seems pretty surprising to me. I assumed the purpose of the ordering constraints between different finalization phases was to allow access from one phase's finalizers to cells belonging to later phases. If this is not the case, then can we assert this somehow? I am imagining tracking the current phase, then in IsAboutToBeFinalized (and other accessors?), mapping the cell you're checking to a finalization phase and checking that it is the same as the current phase. That is assuming that we don't want to allow cross-phase accesses.

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #11)

jonco: This seems pretty surprising to me. I assumed the purpose of the ordering constraints between different finalization phases was to allow access from one phase's finalizers to cells belonging to later phases.

Right. The problem here is we have a finalizer for a later phase (objects) examining cells that have already been finalized (scripts).

I originally though this was OK because of the fact that we keep empty arenas around, but I forget about the non-empty arenas that are returned to zone and can be allocated from during incremental sweeping.

From searchfox I can see IATBF is used directly from the JSScript finalizer and indirectly from the DebuggerFrame one (via clearGenerator). The former is for LazyScript which is finalized later that JSScript. So I think we're fine as far as other instances of this problem go.

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #12)

The problem here is we have a finalizer for a later phase (objects) examining cells that have already been finalized (scripts).

This came up in the GC meeting and we realised that this is wrong. Objects are finalized before scripts in the same zone. The reason is because they are in different zones.

Comment 14

[Steve Fink [:sfink] [:s:]]

We talked about this at our GC meeting today.

The previous comment is backwards -- objects are finalized first, so they are allowed to see scripts. And there's a sweep group edge, so the object and the script here are going to be in the same sweep group. The problem is that that isn't enough for cross-zone edges, because zones are finalized independently. (Most cross-zone edges are ok because they go through the wrapper map, and are not direct pointers.)

From looking at the callgraph, it appears that this case is the only instance of IsAboutToBeFinalized being called from within a finalizer, so we could disallow that.

Comment 15

[Jim Blandy :jimb]

It seems like bug 1565895 is a duplicate of this, or is at least fixed by it.

Comment 16

[Jim Blandy :jimb]

I wonder how we should treat this, in terms of security sensitivity.

• This is not a present danger. One cannot cause IsAboutToBeFinalized to be applied to an inter-alloc-kind, inter-zone edge without using the Debugger API.

• But, there's nothing stopping people from introducing such calls in content-accessible code, which would be a genuine security problem. There's no assertion in IsAboutToBeFinalized that would fire.

So even if this bug doesn't provide information on how to exploit Firefox immediately, it does give people something to look out for, something that we won't catch if we introduce.

If it's possible, we should make IsAboutToBeFinalized safer to use, either by making it give accurate answers, or asserting if it can't. But since IsAboutToBeFinalized has no idea where the edge it's being asked about originated, it's not clear how it could assert.

Comment 17

[Daniel Veditz [:dveditz]]

Thanks for the explanation in comment 16. sec-moderate sounds right then, and we don't need to be concerned about revealing too much. Your second point, that someone might accidentally introduce this problem somewhere else, might even argue that we should unhide the bug. But unhiding the bug won't ensure the right people see it. Land away!

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/ea0524a93cf3d94bf808c41ce879d14cdd12e04e
https://hg.mozilla.org/mozilla-central/rev/ea0524a93cf3