1647747 - Assertion failure: WeakMapBase::checkMarkingForZone(zone), at js/src/gc/GC.cpp:4907 with Debugger

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200623-b1146cce5053 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --more-compartments):

var g27 = newGlobal();
g27.debuggeeGlobal = this;
g27.eval(`
  dbg = new Debugger(debuggeeGlobal);
  dbg.onExceptionUnwind = function (frame, exc) {};
`);
s45 = newGlobal();
try {
  evalcx(`
      function h(h) {}
      h.valueOf=g;
  `, s45);
} catch (x) {}
try {
  evalcx("throw h", s45)
} catch (x) {}
gcslice(100000);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555621a870 in js::gc::GCRuntime::endMarkingSweepGroup(JSFreeOp*, js::SliceBudget&) ()
#1  0x0000555556260561 in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#2  0x000055555624f557 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#3  0x0000555556222b19 in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) ()
#4  0x0000555556227c09 in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason, js::gc::AutoGCSession&) ()
#5  0x000055555622aada in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#6  0x000055555622c6d0 in js::gc::GCRuntime::collect(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#7  0x000055555622dbe8 in js::gc::GCRuntime::startDebugGC(JSGCInvocationKind, js::SliceBudget&) ()
#8  0x0000555555ee278b in GCSlice(JSContext*, unsigned int, JS::Value*) ()
#9  0x000055555593e2e2 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#21 0x00005555557b5c48 in main ()
rax	0x55555707e3f1	93825020716017
rbx	0x7fffffffb4a0	140737488336032
rcx	0x555558358840	93825040484416
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb500	140737488336128
rsp	0x7fffffffb4a0	140737488336032
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9bd40	140737353727296
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0x7ffff602a3b8	140737320756152
r13	0x7ffff6029728	140737320752936
r14	0xffffb402	4294947842
r15	0x7ffff60297b0	140737320753072
rip	0x55555621a870 <js::gc::GCRuntime::endMarkingSweepGroup(JSFreeOp*, js::SliceBudget&)+896>
=> 0x55555621a870 <_ZN2js2gc9GCRuntime20endMarkingSweepGroupEP8JSFreeOpRNS_11SliceBudgetE+896>:	movl   $0x132b,0x0
   0x55555621a87b <_ZN2js2gc9GCRuntime20endMarkingSweepGroupEP8JSFreeOpRNS_11SliceBudgetE+907>:	callq  0x55555584464e <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jon Coppeard (:jonco)]

Steve, can you have a look at this when you get a chance?

Comment 3

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200624093107-e858eb7ffeba.
The bug appears to have been introduced in the following build range:
> Start: 063d499e409ffb2862a2fc49f20835dab96d94f7 (20200425151933)
> End: 436fde13601e1558959b083556f44b7367c0b1a5 (20200425153542)
> Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=063d499e409ffb2862a2fc49f20835dab96d94f7&tochange=436fde13601e1558959b083556f44b7367c0b1a5

Comment 4

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

I love fuzzbugs like this.

I've looked at this some. First, it has a case where the weakmap, key, and value are all in different zones, which I hadn't thought possible. Though it probably doesn't matter much where the value is.

A bigger problem is that the map and key zones are being collected together, but the delegate zone is not. Either it should be in the same group, or there should be logic that says that the key should get marked because the delegate is assumed to be black. (The key is a CCW around the delegate, so ordinarly the key marks the delegate but not the other way around. But in this case, the delegate should preserve the key because it's in a WeakMap and re-wrapping needs to find the same entry.)

I don't think I enforced the first, so the logic for the second must be faulty.

Comment 5

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

Ok, I understand it now. What's going on is:

1. When you mark a map, you scan all unmarked keys and enter them into a weakKeys list.
2. But for each unmarked key, you actually enter its delegate (if it has one) into the list. (Thus if the delegate gets marked later, it will be found in the list. If the key gets marked later, it will mark its delegate since it's a CCW wrapper around the delegate.)
3. When entering weak marking mode for a zone, you scan through the Zone's weakKeys list and check each previously unmarked key to see if the weakmap entry now needs to be marked.

So two factors led to this regression: (1) the key is no longer being added to the weakKeys list, and (2) the delegate's zone is not being collected with the key and map zones. So the key zone's enterWeakMarkingMode does not find the marked delegate, and the delegate zone's enterWeakMarkingMode never happens (well, it would happen later, but then it would be too late.)

I can change either of these to fix the problem. Neither is really something I would prefer to change. I'll think about it.

Comment 6

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

After sleeping on it, I can think of three options:

1. Add both key and delegate to their respective Zones' gcWeakKeys (perhaps only if they are different Zones.)
2. Add a zone edge from the delegate to the key zone
3. When doing markKey for the key, treat a delegate as black if it is going to be marked in a later sweep group

I really don't like #1. It burns up lots of memory for an edge case. #3 is ok, but kind of unfortunate -- we would end up not collecting stuff that could be found to be dead within the set of zones that we are collecting.

#2 is interesting. Initially, I was thinking it was awful because now whenever you have a CCW key, you have to collect both zones together. But then I realized that if you're not collecting the delegate zone at all, then everything works -- when an unmarked key is found during map marking, and its delegate in a noncollecting zone will already be treated as black. So I just need an ordering edge -- if you're collecting these two zones, then they must be collected together not serially. But you don't have to collect them during the same GC at all, if you don't need to.

Comment 7

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

I'm an idiot. #2 is what's already implemented, but only for ObjectValueWeakMap, and DebuggerWeakMap doesn't implement that part. Implementing that part fixes this bug.

Comment 8

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

Attachment: Bug 1647747 - Add in zone edges for delegates to DebuggerWeakMap r=jonco,jorendorff

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Based on the severity of the bug, I assume this fix can ride the trains and won't require uplift. Feel free to scream if that's incorrect though.

Comment 10

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Based on the severity of the bug, I assume this fix can ride the trains and won't require uplift. Feel free to scream if that's incorrect though.

Yes, that seems ok. It looks scarily close to a UAF (as marking bugs generally are), but actually this would just result in a key being dropped from a weakmap so that it wouldn't be there if you looked it up again. Correctness bug, but not a security problem. In this case, it's a DebuggerWeakMap, and I'm not sure exactly what effect that would have. Perhaps just the loss of expando-like properties on the DebuggerObject?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c0f3e0e7df9
Add in zone edges for delegates to DebuggerWeakMap r=jonco,jorendorff

Comment 12

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest failures on /FindSCCs.h

Backout link: https://hg.mozilla.org/integration/autoland/rev/a1edd40624aabc5bb12b9d061b848d5ee4f8a099
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308402630&repo=autoland&lineNumber=2430

Comment 13

[Narcis Beleuzu [:NarcisB]]

Please also check:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308405734&repo=autoland&lineNumber=21845
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308402446&repo=autoland&lineNumber=2495
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308403162&repo=autoland&lineNumber=2355

Comment 14

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

Attachment: Bug 1647747 - Followups to DebuggerWeakMap changes.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/032cdefd3d8c
Add in zone edges for delegates to DebuggerWeakMap r=jonco,jorendorff

Comment 16

[Natalia Csoregi [:nataliaCs]]

Backed out for assertion failures on Cell.h

backout: https://hg.mozilla.org/integration/autoland/rev/d6746477f75fdd127da58185eaeb49179d154e66

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=032cdefd3d8c49559717b6aad7b4a9275db8875a&searchStr=plain%2Cmochitests&selectedTaskRun=YMKWtF4WRjmITIhaMCNjAg.0

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

[task 2020-07-15T04:01:19.913Z] 04:01:19 INFO - GECKO(2667) | JavaScript error: resource:///actors/DOMFullscreenParent.jsm, line 113: Error: TelemetryStopwatch: key "FULLSCREEN_CHANGE_MS" was already initialized
[task 2020-07-15T04:01:19.997Z] 04:01:19 INFO - GECKO(2667) | Assertion failure: isTenured(), at /builds/worker/checkouts/gecko/js/src/gc/Cell.h:301
[task 2020-07-15T04:01:20.188Z] 04:01:20 INFO - GECKO(2667) | [Parent 2667, Breakpad Server] WARNING: Resource acquired is being released in non-LIFO order; why?
[task 2020-07-15T04:01:20.188Z] 04:01:20 INFO - GECKO(2667) | : file /builds/worker/checkouts/gecko/xpcom/threads/BlockingResourceBase.cpp, line 292
[task 2020-07-15T04:01:20.191Z] 04:01:20 INFO - GECKO(2667) | --- Mutex : dumpSafetyLock (currently acquired)
[task 2020-07-15T04:01:20.191Z] 04:01:20 INFO - GECKO(2667) | calling context
[task 2020-07-15T04:01:20.191Z] 04:01:20 INFO - GECKO(2667) | [stack trace unavailable]

Comment 17

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

I somehow managed to push the wrong version, without the latest round of fixes merged in.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8190007924dd
Add in zone edges for delegates to DebuggerWeakMap r=jonco,jorendorff

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8190007924dd

Comment 20

[Jason Kratzer [:jkratzer]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR78 approval ASAP.

Comment 23

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

Comment on attachment 9159849 [details]
Bug 1647747 - Add in zone edges for delegates to DebuggerWeakMap r=jonco,jorendorff

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very difficult. It's reading a value from the nursery as if it were the tenured heap, and then using that to make a marking decision, so in theory you could get a UAF out of it. It also requires the use of the Debugger API.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 79
• If not all supported branches, which bug introduced the flaw?: Bug 1470369
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I think it should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: It should be pretty safe.

Beta/Release Uplift Approval Request

• User impact if declined: Theoretically a UAF, though unlikely. More likely, it's probably possible for this to be a stability issue when using Debugger.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's calling code that is already used for other purposes, and should also have been called in this case.
• String changes made/needed: none

Comment 24

[Andrew McCreight (out of office until 8/21) [:mccr8]]

You nominated it for sec-approval and not esr78.

Comment 25

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

(In reply to Andrew McCreight [:mccr8] from comment #24)

You nominated it for sec-approval and not esr78.

Should I even be requesting sec-approval given that it's already landed? (I landed it before I thought it was relevant to security.)

I don't need it on 78. The regression was in 79.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9159849 [details]
Bug 1647747 - Add in zone edges for delegates to DebuggerWeakMap r=jonco,jorendorff

Approved for 79.0rc1.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ffdee1f07e14

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 9159849 [details]
Bug 1647747 - Add in zone edges for delegates to DebuggerWeakMap r=jonco,jorendorff

post-facto sec-approval+