812380 - Possible use-after-CC-Unlink due to incorrect cycle collector WeakMap optimizations

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

In pseudocode, the GC's WeakMap::markIteratively works like this:
  if (key is marked) {
    mark value
  } else (delegate is marked) {
    mark key
    mark value
  }

This means that even if the value is marked when we start, the key will get marked if the delegate is marked.

On the other hand, the cycle collector will bail out at a number of points (TraceWeakMapping, GCGraphBuilder::NoteWeakMapping, maybe other places) if the value is marked black, so we never hit the equivalent code in ScanWeakMaps, which I think means that the key can end up being marked as dead in the CC when it is alive in the GC, which means we'll Unlink a live object, which is not awesome.

I don't really want to eliminate the optimization entirely, as I imagine the bulk of things in weak maps are black. Maybe changing the bailouts to check if the key AND value are black would work. I don't really want to model full logic from markIteratively in the optimization that is in the plumbing.

Comment 1

[Andrew McCreight [:mccr8]]

This is not dangerous by itself, but use-after-CC-Unlink errors can be combined with errors in Unlink functions to cause badness.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: WIP

Comment 3

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=8f1db6f51e03

Comment 4

[Andrew McCreight [:mccr8]]

This patch does a number of things.

1. At the start of TraceWeakMapping, we only bail out early if both the key and value are things we aren't going to care about in the CC.

2. The child tracer, which handles the weird fanout case where the value isn't representable in the CC so we instead add all of its children (maybe not a problem post E4X removal, because I think the only cases for this are XML and shapes), could encounter only values that we optimize out of the CC, and thus not report any entries to the CC, which would prevent the delegate->key edge from being communicated to the CC. To handle this case, we ensure that at least one entry is reported, with an empty NULL value.

3. Don't bother trying to optimize away weak map entries in GCGraphBuilder::NoteWeakMapping any more: this should be handled in XPC, and I don't want to duplicate the weird logic.

4. The rest of the changes in the CC are accounting for the fact that we now sometimes pass NULL as the value pointer, to handle (2).

The good news that 3 and 4 make the code a little simpler and more uniform...

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 682510 [details] [diff] [review]
WIP

This isn't quite right for the !AddToCCKind(vkind) case.

Comment 6

[Andrew McCreight [:mccr8]]

Hmm.  I think this may not actually be a problem after all, because the only keys this can apply to are Proxies, and the only outgoing edge of the proxy is to the wrappee, so if the wrappee is really alive, then tricking the CC into thinking the wrapper isn't alive doesn't help, because the CC already thinks the wrappee is alive.

You can also confuse the CC about whether the entry is alive or not... but the only case where this applies is when the value is either marked black, and thus definitely alive, or something that the CC doesn't care about, so I think that's irrelevant.

Marking moderate for now, but I want to think about this for a bit. And we may want to fix this anyways, because it is unsettling.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: don't optimize as much

Comment 8

[Andrew McCreight [:mccr8]]

Okay, never mind, after talking to billm it sounds like the wrapper could have point to other things besides its wrappee.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: optimize less

jorendorff said watchpoint closures can be null, so I went through this function and added a bunch of null checks for keys and values.  Gross.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: optimize less, null check more

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

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

::: xpcom/base/nsCycleCollector.cpp
@@ +2268,5 @@
>                  anyChanged = true;
>              }
>  
> +            if (mColor == black && kColor == black && vColor != black) {
> +                GraphWalker<ScanBlackVisitor>(ScanBlackVisitor(mWhiteNodeCount)).Walk(wm->mVal);

Do you need a null check on wm->mVal here? Or will Walk handle it?

Comment 12

[Andrew McCreight [:mccr8]]

> Do you need a null check on wm->mVal here? Or will Walk handle it?

(vColor != black) implies (v != null), because (v == null) implies (vColor == black).

Comment 13

[Andrew McCreight [:mccr8]]

(from the definition of vColor)

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Security approval request comment]
How easily can the security issue be deduced from the patch? Probably not very easily.

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? 17+

If not all supported branches, which bug introduced the flaw? already marked

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Shouldn't be any problem, as I think this code hasn't changed much.

How likely is this patch to cause regressions; how much testing does it need? Probably not too bad, but this code is a little bit tricky.

Comment 15

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62d807d96900

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62d807d96900

Comment 17

[Alex Keybl [:akeybl]]

Internally discovered, a bit tricky, Let's get this landed on Aurora, and then fixed on the ESR17 in the 19+ timeframe.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 798678
User impact if declined: possible crashes, security problems
Testing completed (on m-c, etc.): it has been on m-c for a few days
Risk to taking this patch (and alternatives if risky): shouldn't be too bad. most of the changes in this patch are fairly simple. but of course, all of this code is a little unpleasant.
String or UUID changes made by this patch: none

Comment 19

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3e93b76c6bc

Comment 20

[Andrew McCreight [:mccr8]]

I'm assuming that the 19+ on the ESR-17 thing means this shouldn't land there yet.

Comment 21

[Alex Keybl [:akeybl]]

Yep - please prepare a patch for both ESR17 and B2G18. Thanks!

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 19, 20
Risk to taking this patch (and alternatives if risky): should be low
String or UUID changes made by this patch: none

This patch also applies to b2g18 cleanly. I'm assuming I shouldn't flag it for approval yet.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

Approving this low risk fix for esr17. You can nominate for b2g18 but we won't be approving those uplifts for tracking-b2g18=19+ until after 1/25

Comment 24

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/e9cbc3f091c6

Comment 25

[Andrew McCreight [:mccr8]]

Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Approval Request Comment]
Oops, I guess this never got nominated. See comment 2.

Comment 26

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9d285ca1bf1a

Comment 27

[Alex Keybl [:akeybl]]

Marking as tef+ - we should either uplift to v1.0.1 or mark that flag as fixed.

Comment 28

[Andrew McCreight [:mccr8]]

Looks fixed to me in the repo.