1414448 - Crash in SkImage::peekPixels

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-a324b3c6-f0f8-4c71-a354-437300171103.
=============================================================

There are 19 crashes in nightly 58 starting with buildid 20171101104430.
13 reports are showing that crash address is 0xffffffffe5e5e5e5 so probably an UAF.
:lsalzman, could you investigate please ?

Comment 1

[Marcia Knous [:marcia]]

Adding another signature to the mix.

Comment 4

[Lee Salzman [:lsalzman]]

The evidence seems to point to these reports starting with the Oct 31 nightly. I can't pinpoint any particular causative patch at the moment, though others did some big patches that landed around that time in the gfx area.

Comment 5

[Bas Schouten (:bas.schouten)]

So with information gathered by Lee, I believe I know what the cause of this bug is. The fix is slightly tricky, but I'll have one by tomorrow.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Noting the duplicate bug is marked as sec-high so it seems likely this should also be rated sec-high.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Protect SourceSurfaceSkia destructor from racing with DrawTargetWillChange

This is the simplest solution I can think of, the thing is due to a lack of lifetime guarantees here we have to keep the mutex alive for the duration of the SourceSurfaceSkia destructor. At the same time the DrawTarget needs to lock the mutex at the very beginning of MarkChanged, and can't get it off the SourceSurface or it could still race with the destructor. Therefore a threadsafe object with ownership shared by both the DrawTarget and the SourceSurface seems like the only solution, I couldn't find an existing helper class for sharedlocks in Gecko, so this seems like the safest approach.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8926431 [details] [diff] [review]
Protect SourceSurfaceSkia destructor from racing with DrawTargetWillChange

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

Yeah, unfortunately the way gecko uses Moz2D makes lifetimes very unclear.

When Snapshots are given to a DrawTargetCapture, we SHOULD guarantee they are detached before being replayed on the paint thread. SourceSurfaceCapture already asserts this. Maybe we should assert it for the other DrawTargets too. There would be edge cases, for example nsDisplayCanvasBackgroundColor creates a draw target and caches it, and then keeps returning the same snapshot. For that case we need some kind of synchronization. But for all non-caching cases it should be okay to destroy or detach DrawTargets on the main thread before the paint thread starts replaying.

That said - I think the mutex solution covers all the bases, so let's see if our theory is right.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

If our theory is right, we probably need this for D2D as well. D2D inverts the ownership, so the DrawTarget owns the snapshot, but the same problem could happen.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44e87c9281a4

Comment 12

[Bas Schouten (:bas.schouten)]

This signature is gone as per the 9th. Confirming our theory.