1414389 - Crash in mozilla::gfx::DrawTargetSkia::FillRect

Status: RESOLVED WORKSFORME

(Core :: Graphics: Layers, defect, P3)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-ac95df4c-5c1d-4f78-a9a6-767ce0171102.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2zuw0q7

Crashes started in 58 using 20171031220132 (17 crashes/11 installs) and affect Win 7, Win 8 and Win 10. One crash also present in the later 57 betas, and 2 in Firefox 56.

marking as sec sensitive due to the possible UAF addresses.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

This may very well be the same root cause as bug 1413857, just some kind of a race condition.  Even though it's in 56, possibly tickled by OMTP.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Also, bug 1412850

Comment 3

[Bas Schouten (:bas.schouten)]

Agreed.

Comment 4

[Marcia Knous [:marcia]]

I see a few other signatures that look related:

*mozilla::gfx::GetSkImageForSurface - http://bit.ly/2zypvls 
*mozilla::gfx::StoredPattern::Assign - http://bit.ly/2zypvls

Comment 6

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

59 also looks to be affected.

Comment 7

[[:philipp]]

this is one of the top content crashes in 58.0b so far (nearly 2% of content crashes in 58.0b6).
can we increase the priority based on this?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Lee, Ryan, this is a DrawTargetSkia::FillRect fast path (bug 1294337) with OMTP.

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Lee, Ryan, this is a DrawTargetSkia::FillRect fast path (bug 1294337) with
> OMTP.

The fact that this is inside DrawTargetSkia::FillRect is merely incidental, and not causative. I don't think the problem is FillRect directly. It's accessing a SourceSurface inside a SurfacePattern on a DrawTargetCapture playback. There are thus two possibilities here given the line it is crashing at: https://hg.mozilla.org/mozilla-central/annotate/e8ce4865e421/gfx/2d/DrawTargetSkia.cpp#l765

Either 1) the pat.mSurface is crashing, meaning pat is already freed, which means the FillRectCommand containing this StoredSurface/SurfacePattern is freed, which means the DrawTargetCapture around it is likely thus bogus.

or 2) pat.mSurface->GetFormat() is crashing, implying that the mSurface is already freed, even though pat is still valid. Superficial inspection of the StoredPattern code looks like it is more or less currently keeping the reference to the SourceSurface that is pointed at, though.

I don't have access to bug 1413857, so I can offer no insight into a relationship there.

Someone more familiar with the lifetimes of SourceSurfaces within OMTP would be better to follow up.

Comment 10

[Bas Schouten (:bas.schouten)]

I suspect this is bug 1416862 with subtly different timings, let's see how the patch there affects this.

Comment 11

[Bas Schouten (:bas.schouten)]

Crash numbers are low on nightly so it's hard to say if bug 1416862 made a difference here, but I'm going to keep an eye on this.

Comment 12

[Bas Schouten (:bas.schouten)]

Analysis of a couple of crash dumps shows that for the address of the mSurface vtable we find 0xe5e5e5e5 or 0x1 in most of these cases. This suggests bug 1416862 will either fix this or there may be another race condition we don't know about yet. So far I haven't been able to figure out if the surface is a snapshot here or not.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Bug 1416862 landed around 12/8, and was uplifted to beta 12/10 (so isn't in 58b10, but would be in b11).  59 crash rate is too low to use for verification.  Beta should show quickly if that bug fixed this.

Comment 14

[Frederik Braun [:freddy] (OOO - Back April 8th)]

Time has passed long enough for us to get new crashes.
We still get some reports after 58b11 with addresses like 0xffffffffe5e5e5f1, 0xffffffffe0201b64, 0x7fed3ffc5e0, 0xffffffffffffffff still implies a use-after-free. Can you take another look, Bas?

Comment 15

[Bas Schouten (:bas.schouten)]

So, in the recent builds (58.0.2), there appear to be three crashes, one of them is with 0xffffffffffffffff, and one with 0x0, those are completely different crashes than the original bug here, it is actually accessing aPattern here rather than aSurface, so accessing arguments to the functions, these are very mysterious, but I don't think at a crash rate of 2 over all of 58.0.2, they're a concern, nor do I think there's enough information at least in these bugs to figure out the cause, it may just be some random form of corruption at this crash rate.

The third is a surface which it can't find at the address it's expecting, but that doesn't have the typical UAF poisoning pattern, it may be something else going on, but from a single crash it's really hard to figure out what it could be. That crash has no painting activity on any other threads at least.

There's a single one that looks like a real UAF (with the memory poisoning pattern in there), which is https://crash-stats.mozilla.com/report/index/ae44615c-0070-413f-a291-a5f860180213. This one is the sole crash that looks similar to the original bug filed here, I can't really explain that one.

There's also a non-OMTP one in OS X here: https://crash-stats.mozilla.com/report/index/4f3bf002-426e-4a81-bf8e-c52c10180207

This is an extremely hot function, these crash reports are so rare, and show so little correlation, I think we can consider the original bug fixed. Unless we find something more common indicative of a race condition, if there is one, I don't think at this point this is actionable, or a real threat to our users from a security perspective.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Exec wildptr crash in 59b6: https://crash-stats.mozilla.com/report/index/309ac918-089b-43c3-878e-5fbd90180207 and other wildptr READ violations

Lots of clear UAF signatures in 59a1 and 58bN: https://crash-stats.mozilla.com/report/index/1262307a-d9f3-4ad0-b14c-685140180217 (that seems to have gone away - temporary bug?)

25-30 58.0.N crashes, many wildptr, one clear UAF: https://crash-stats.mozilla.com/report/index/ae44615c-0070-413f-a291-a5f860180213

Search: https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Agfx%3A%3ADrawTargetSkia%3A%3AFillRect&date=%3E%3D2018-01-28T08%3A48%3A56.000Z&date=%3C2018-02-28T08%3A48%3A56.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-version&_sort=-date&page=1

Further thoughts, Bas?

Comment 17

[Bas Schouten (:bas.schouten)]

(In reply to Randell Jesup [:jesup] from comment #16)
> Exec wildptr crash in 59b6:
> https://crash-stats.mozilla.com/report/index/309ac918-089b-43c3-878e-
> 5fbd90180207 and other wildptr READ violations

I'll have a look at these, not sure what's up with these, they don't look like the original bug.

> Lots of clear UAF signatures in 59a1 and 58bN:
> https://crash-stats.mozilla.com/report/index/1262307a-d9f3-4ad0-b14c-
> 685140180217 (that seems to have gone away - temporary bug?)

It looks like these were actually the original bug? Indeed this report is from before the patch landed (11/22)

> 25-30 58.0.N crashes, many wildptr, one clear UAF:
> https://crash-stats.mozilla.com/report/index/ae44615c-0070-413f-a291-
> a5f860180213

I think this is the one I listed in my comment, that's the one crash that looks like the original bug, I can't explain it, but it's pretty lonely in there.

Comment 18

[Frederik Braun [:freddy] (OOO - Back April 8th)]

Any news?

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://crash-stats.mozilla.com/report/index/06c1020a-3735-4725-988f-0ea500180602 is an e5e5 crash in 60.0.1

About 30 or 40 crashes in 60 and newer in the last month. Mostly wildptr or fff's, one e5

Comment 20

[Ryan Hunt [:rhunt]]

From a discussion with Lee last week, these crashes could be caused by us iterating over the command list at the same time we are mutating it (causing a resize and free of the backing store). This should not be happening and it's not clear how we could be doing that as we lock around this.

So I think it may be worth adding a diagnostic assert to catch improper uses of CaptureCommandList to give us some more useful call stacks.

I've prototyped a patch and will upload it soon.

Comment 21

[Ryan Hunt [:rhunt]]

Attachment: capture-command-list-thread-safety.patch

This patch adds some threadsafe tracking information to assert that a capture command list is either only being read (by possibly multiple readers), or only being written to by one writer.

The tracking flags should detect any case of calling Append, ReuseOrAppend, operator= while iterating over the display list. It will also assert if we destroy the CaptureCommandList while an iterator still active.

The one thing this doesn't protect against is multiple threads calling Append but somehow always never at the same time, but stomping over the pointer returned by Append. This is a limitation of the CaptureCommandList API, but I don't think it's worth working around because it's very unlikely.

Comment 22

[Ryan Hunt [:rhunt]]

Does this patch need sec-review? It's mostly just a diagnostic crash to try and discover the real issue.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 8989447 [details] [diff] [review]
capture-command-list-thread-safety.patch

It needs sec-approval+, not sec-review. Even though it's not a fix we need to double-check that such diagnostic changes don't point a finger at an obvious problem area.

sec-approval=dveditz

Make sure you mark the bug "leave-open" so the sheriffs don't close the bug when they merge the diagnostic patch.

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e672ba38c0a4750468a2927be9ffa88bac4b002d
https://hg.mozilla.org/mozilla-central/rev/e672ba38c0a4

Comment 25

[Ryan Hunt [:rhunt]]

I did some searching on crash stats and have been unable to find any reports hitting this assert yet.

Unsure if either there is a processing delay, I'm not searching optimally, or no users have hit this assert.

I've searched for all combinations of `RwAssert` and `CaptureCommandList` in `proto signature` and `signature`.

Comment 26

[Ryan Hunt [:rhunt]]

From triaging my open bugs I noticed bug 1445481, which is a release assert being hit in SourceSurfaceCapture.

Now that I know this code better here, this assert was added to detect when we have a SourceSurfaceCapture on the paint thread which has an underlying CaptureCommandList that could be mutated by an attached DrawTargetCapture.

This seems like it could be the same situation, and getting a better crash report here could help there.

Comment 27

[Ryan Hunt [:rhunt]]

Also according to Bas, the locking and tracking flags introduced here are showing up in his profiles and are causing some real performance regressions.

I'd like to leave this code in until we get some actionable crash reports and then remove it before it goes to beta. Or alternatively compile it conditionally on nightly.

Comment 28

[Ryan Hunt [:rhunt]]

(In reply to Ryan Hunt [:rhunt] from comment #25)
> I did some searching on crash stats and have been unable to find any reports
> hitting this assert yet.
> 
> Unsure if either there is a processing delay, I'm not searching optimally,
> or no users have hit this assert.
> 
> I've searched for all combinations of `RwAssert` and `CaptureCommandList` in
> `proto signature` and `signature`.

Update. Still haven't seen any diagnostics crashes in crash-stats. I haven't
seen any of the original FillRect crashes in the 63 nightly either [1].

[1] https://crash-stats.mozilla.com/signature/?product=Firefox&version=63.0a1&signature=mozilla::gfx::DrawTargetSkia::FillRect&date=%3E%3D2017-10-19T20:14:37.000Z&date=%3C2018-07-17T20:14:00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

Comment 29

[Daniel Veditz [:dveditz]]

The crashes now are definitely different. I don't see the original UAF signatures after 58 (in recent crashes; see comment 19), and a bunch of the recent crashes are near-null.

In the 62/63 crashes it looks like aPattern is bogus and is simply detected at various points depending on how bogus (usually at aPattern.GetType() or pat.mSurface->GetFormat() a couple lines later). But it's not marked with the freed-memory poison value.

Comment 30

[Ryan Hunt [:rhunt]]

Attachment: rw-assert.patch

Still haven't seen any diagnostic crashes since it landed. According to lsalzman and Bas, this assertion has real performance impacts and shows up in profiles so we should remove it.

Comment 31

[Ryan Hunt [:rhunt]]

Comment on attachment 9002962 [details] [diff] [review]
rw-assert.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily, it's removing a diagnostic crash.

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? Unsure

If not all supported branches, which bug introduced the flaw? Enabling of OMTP

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Comment 32

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for landing on trunk since it is just removing the crash.

Comment 33

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/62bf59351409430561fb8f8fcef08bdb1a7208cd
https://hg.mozilla.org/mozilla-central/rev/62bf59351409

Comment 34

[Pascal Chevrel:pascalc]

Ryan, do you think that your patch can be uplifted to 63 beta? Thanks

Comment 35

[Ryan Hunt [:rhunt]]

The diagnostic patch was added and removed in the 63 nightly cycle so no uplifting should be necessary for removing the diagnostic from any of our branches.

Comment 36

[Pascal Chevrel:pascalc]

(In reply to Ryan Hunt [:rhunt] from comment #35)
> The diagnostic patch was added and removed in the 63 nightly cycle so no
> uplifting should be necessary for removing the diagnostic from any of our
> branches.

Thanks, adjusting flags accordingly then.

Comment 37

[Maire Reavy [:mreavy]]

Adding Jessie (new graphics engineering manager) to all sec-crit and sec-high graphics bugs

Comment 38

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

The leave-open keyword is there and there is no activity for 6 months.
:rhunt, maybe it's time to close this bug?

Comment 39

[Ryan Hunt [:rhunt]]

Looking at the crash table, it seems like this is very low frequency but is still probably not fixed.

Comment 40

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

The leave-open keyword is there and there is no activity for 6 months.
:rhunt, maybe it's time to close this bug?

Comment 42

[Daniel Veditz [:dveditz]]

Haven't seen this in current builds

Comment 43

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit BugBot documentation.