1425257 - Crash in mozilla::gfx::DrawTargetD2D1::Flush

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-4161558c-9197-45e7-ba1c-cf2190171214.
=============================================================

Top 6 frames of crashing thread:

0 xul.dll mozilla::gfx::DrawTargetD2D1::Flush gfx/2d/DrawTargetD2D1.cpp:176
1 xul.dll mozilla::gfx::DrawTargetD2D1::MarkChanged gfx/2d/DrawTargetD2D1.cpp:1295
2 xul.dll mozilla::gfx::DrawTargetD2D1::ClearRect gfx/2d/DrawTargetD2D1.cpp:327
3 xul.dll mozilla::dom::CanvasRenderingContext2D::ClearRect dom/canvas/CanvasRenderingContext2D.cpp:3219
4 xul.dll mozilla::dom::CanvasRenderingContext2DBinding::clearRect dom/bindings/CanvasRenderingContext2DBinding.cpp:5307
5  @0x31f93e112c7 

=============================================================

these crashes in the content process on windows are continuing after the patch for bug 1416864 has landed in 58.0b9 targetting the same signature.

https://mozilla.github.io/stab-crashes/supergraph.html?s=https%3A%2F%2Fcrash-stats.mozilla.com%2Fsearch%2F%3Fsignature%3D%253Dmozilla%253A%253Agfx%253A%253ADrawTargetD2D1%253A%253AFlush%26version%3D58.0b

Comment 1

[Peter Chang[:pchang]]

From the crash dump, it crashed at [1] and the address of DrawTargetD2D1 instance was filled with 0xe5e5e5. I think this is a UAF issue. 

mDependingOnTargets is used to store the raw pointer of DrawTargetD2D1. Therefore, it is possible to access a DrawTargetD2D1 which got destroyed.

[1]https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/gfx/2d/DrawTargetD2D1.cpp#176

Comment 2

[Peter Chang[:pchang]]

'mDependentTargets' is also used to store the raw pointer of DrawTargetD2D1. These two structures are used to perform some synchronization. I'm thinking to add a lock to protect 'mDependentTargets' but not mDependingOnTargets to prevent the deadlock.

Comment 3

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

A crash report that hasn't been deleted: https://crash-stats.mozilla.com/report/index/d77584a0-ac45-4ade-975c-8714d0180103 and a matching one on Skia - https://crash-stats.mozilla.com/report/index/91194efe-0672-48c1-9f05-467f60180103

Comment 4

[Frederik Braun [:freddy]]

Lee, this looks pretty inactive while we aim for a ~2 week timeframe for all sec-highs.
Can you bump this in your queue? If not, is there a way we can support you getting there?

Comment 5

[Lee Salzman [:lsalzman]]

Bas, ideas? Looks like something still not going right with the locking added in bug 1416864?

Comment 6

[Bas Schouten (:bas.schouten)]

I feel I know what's going on here. (For the D2D case, I don't think that particular Skia issue has anything to do with this)

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Use a global mutex to guard the DT dependency graph.

This seems like the safest and most upliftable approach.

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8948440 [details] [diff] [review]
Use a global mutex to guard the DT dependency graph.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very difficult, the patch gives little information about where the freed objects are actually located.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? On the bug, yes, on a way to exploit it, no.

Which older supported branches are affected by this flaw? 58+ This patch applies there.

How likely is this patch to cause regressions; how much testing does it need? Extremely unlikely, the patch only introduces an additional lock.

Comment 9

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

sec-approval+ for trunk.
Please request bounty approval on the patch (or another if necessary) as well.

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Al Billings [:abillings] from comment #9)
> sec-approval+ for trunk.
> Please request bounty approval on the patch (or another if necessary) as
> well.

I don't know much about out procedures here.. This was posted from the Socorro interface and not as a security bug, is a bounty appropriate here? (Sorry Philipp :-))

Comment 11

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3400afa45f

Comment 12

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

Backed out for failing R-e10s in slave/test/build/tests/reftest/tests/image/test/reftest/pngsuite-basic-n/basn0g01.png, furthermore webgl failures on Windows:

https://hg.mozilla.org/integration/mozilla-inbound/rev/28d736c28e70c3c092d8b7118511ece0e87da541

Push which ran all the failing tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=05b29220fc4de84e676cf4bfe7a673ae640dd929&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log reftest: https://treeherder.mozilla.org/logviewer.html#?job_id=160540484&repo=mozilla-inbound

Comment 13

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e30390f2f53a

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/e30390f2f53a

Comment 15

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

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> 
> I don't know much about out procedures here.. This was posted from the
> Socorro interface and not as a security bug, is a bounty appropriate here?
> (Sorry Philipp :-))

As something without a test case and which only has data as a top crash, it doesn't really meet the bar.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Given the low frequency with which this is happening on Nightly, are we likely to get any good sense of whether this is fixed before uplifting to Beta?

Comment 17

[Bas Schouten (:bas.schouten)]

Comment on attachment 8948440 [details] [diff] [review]
Use a global mutex to guard the DT dependency graph.

I was trying to determine that, I'd say the answer is no.

Approval Request Comment
[Feature/Bug causing the regression]: OMTP
[User impact if declined]: Rare, but sec-high crash.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]:Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only adds a lock, a tiny perf regression is the only potential risk.
[String changes made/needed]: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8948440 [details] [diff] [review]
Use a global mutex to guard the DT dependency graph.

Let's get this into today's 59b8 so we can see if it helps sooner.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4437812f495f