1644985 - Crash [@ js::gc::GCRuntime::unregisterWeakRef] with use-after-free or Assertion failure: flags() == 0, at gc/Cell.h:741 with Weak References

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200610-590d76562067 (opt build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --enable-weak-refs --more-compartments):

wr4 = new WeakRef(evalcx('({})'));
function simulateEndOfJob(fn) {
  clearKeptObjects();
  gc();
}
nukeAllCCWs();
simulateEndOfJob()

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555555ab7cf0 in js::gc::GCRuntime::unregisterWeakRef(JSContext*, JSObject*, js::WeakRefObject*) ()
#1  0x0000555555d06631 in unsigned long js::gc::Arena::finalize<JSObject>(JSFreeOp*, js::gc::AllocKind, unsigned long) ()
#2  0x0000555555ce988e in FinalizeArenas(JSFreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&) ()
#3  0x0000555555cf95b0 in js::gc::ArenaLists::foregroundFinalize(JSFreeOp*, js::gc::AllocKind, js::SliceBudget&, js::gc::SortedArenaList&) ()
#4  0x0000555555cfab1f in js::gc::GCRuntime::finalizeAllocKind(JSFreeOp*, js::SliceBudget&) ()
#5  0x0000555555d09ff1 in sweepaction::SweepActionForEach<ContainerIter<mozilla::EnumSet<js::gc::AllocKind, unsigned long> >, mozilla::EnumSet<js::gc::AllocKind, unsigned long> >::run(js::gc::SweepAction::Args&) ()
#6  0x0000555555d0dcfc in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#7  0x0000555555d09d9c in sweepaction::SweepActionForEach<js::gc::SweepGroupZonesIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#8  0x0000555555d0dcfc in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#9  0x0000555555d09b39 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#10 0x0000555555cfaf77 in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) ()
#11 0x0000555555cfce60 in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason, js::gc::AutoGCSession&) ()
#12 0x0000555555cfe0f1 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#13 0x0000555555cfeeb8 in js::gc::GCRuntime::collect(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#14 0x0000555555cd787f in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::GCReason) ()
#15 0x00005555559d3018 in JSRuntime::destroyRuntime() ()
#16 0x0000555555946d3c in js::DestroyContext(JSContext*) ()
#17 0x000055555568a312 in main ()
rax	0x4b4b4b4b4b4b4000	5425512962855747584
rbx	0x3b356c500000	65100636487680
rcx	0x3b356c5ba040	65100637249600
rdx	0x3b356c598c20	65100637113376
rsi	0x3b356c5ba040	65100637249600
rdi	0x7fffffffd490	140737488344208
rbp	0x7fffffffd220	140737488343584
rsp	0x7fffffffd1c0	140737488343488
r8	0xfd00070	265289840
r9	0x30	48
r10	0xfd0	4048
r11	0x7fffffffd27c	140737488343676
r12	0x40	64
r13	0x3b356c5ba040	65100637249600
r14	0x3b356c5ba040	65100637249600
r15	0x40	64
rip	0x555555ab7cf0 <js::gc::GCRuntime::unregisterWeakRef(JSContext*, JSObject*, js::WeakRefObject*)+48>
=> 0x555555ab7cf0 <_ZN2js2gc9GCRuntime17unregisterWeakRefEP9JSContextP8JSObjectPNS_13WeakRefObjectE+48>:	mov    0x8(%rax),%rax
   0x555555ab7cf4 <_ZN2js2gc9GCRuntime17unregisterWeakRefEP9JSContextP8JSObjectPNS_13WeakRefObjectE+52>:	cmpl   $0x0,0xa30(%rax)

Marking s-s until triaged due to potential use-after-free.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200611093454-10ad7868f3ca.
The bug appears to have been introduced in the following build range:
> Start: a78b1f43fcab841c3d93bbed383853602467fbd7 (20191213054920)
> End: d1ac49b9eb3efcc46210bb7ad810c80ba74f7dd7 (20191213060919)
> Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a78b1f43fcab841c3d93bbed383853602467fbd7&tochange=d1ac49b9eb3efcc46210bb7ad810c80ba74f7dd7

Comment 3

[Jon Coppeard (:jonco)]

This looks like an interaction with wrapper nuking.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1644985 - Clean up WeakRef and FinalizationRegistry data when cross compartment wrappers are nuked r?sfink

This crash happens because we try and clean up the map from target to WeakRef in the WeakRef finalizer, and the target can be dead by this point if it's a nuked CCW (before it is nuked the CCW ensures this sweep order does not happen).

The fix is to fix up the map when CCWs to WeakRefs are nuked. Fortunately there's already a hook where the GC is told about this.

The same issue applies to FinalizationRecordObjects. This fix is slightly different because they don't have a target pointer so we can't find the map entry. Instead we clear the record and cleanup happens later when it gets swept.

Comment 5

[Jon Coppeard (:jonco)]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

WeakRefs are only present on nightly at the moment.

Comment 6

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

https://hg.mozilla.org/integration/autoland/rev/75b4198a731db7dcbfc6280bf25639b083e36cca
https://hg.mozilla.org/mozilla-central/rev/75b4198a731d

Comment 8

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200615092624-f05a0084c5f2.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 10

[Andrew McCreight [:mccr8]]

The release management bot is leaking information about what releases are affected by sec bugs into non-sec bugs, which I think is not good: https://bugzilla.mozilla.org/show_bug.cgi?id=1655917#c2

I'm not sure if there's a bugzilla component I can file a hidden bug in or not, so I'm needinfo'ing you all here.

Comment 11

[Andrew McCreight [:mccr8]]

This also leaks the information that there is a sec bug that caused the regression, as you can't see the regressed-by field if the bug there is a sec bug you can't access.

Comment 12

[Calixte Denizet (:calixte)]

I disabled the autonag tool which is responsible of that in waiting for a fix.

Comment 13

[Daniel Veditz [:dveditz]]

This leak doesn't sound that bad. Isn't it better for people working on the regression to have a handle (bug number) they can use to find the right people involved? Otherwise they will have to go looking for the regressor themselves, and either find the sec bug patch anyway or not find it and have a harder time fixing the regression.

Is this any worse than the leaking we do when we check in security bug fixes with just a bug number comment? You've got a number, you know it's inaccessible, and you can find the code change.

Well, if we are concerned it's probably not too hard to have the nagbot file those as private comments if current bug is public and the regressing bug is private. I do find the nagbot's status-setting--and importantly the reasons for the setting--to be useful.

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Daniel Veditz [:dveditz] from comment #13)

Is this any worse than the leaking we do when we check in security bug fixes with just a bug number comment? You've got a number, you know it's inaccessible, and you can find the code change.

In this particular case, the patch has landed so I don't think it matters (as you said), but in general people could mark a regressing bug for sec bugs where a patch has not landed. It could also make a little bit of difference in a case where we land a patch on Nightly before beta and it would show that beta is also affected. The patch would already give more information, but analyzing regression bot traffic would be an easier way of automatically finding bugs that affect release than analyzing patches.

As a side note, it looks like the regression information marked in the other bug is different from the information marked in this bug, which seems weird.

Comment 15

[Andrew McCreight [:mccr8]]

Conceptually, it feels like the bug bot that marks public bugs should not have sec bug access (so it would be impossible for it to leak information), and you could have a separate instance that does have sec bug access that could mark sec bugs. Maybe that's too much of a pain.