Closed Bug 1414448 Opened 2 years ago Closed 2 years ago

Crash in SkImage::peekPixels

Categories

(Core :: Canvas: 2D, defect, critical)

58 Branch
Unspecified
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: calixte, Assigned: bas.schouten)

References

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(1 file)

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 ?
Flags: needinfo?
Adding another signature to the mix.
Crash Signature: [@ SkImage::peekPixels] → [@ SkImage::peekPixels] [@ mozilla::gfx::ReadSkImage]
Flags: needinfo?
Flags: needinfo?(lsalzman)
Duplicate of this bug: 1414877
Flags: needinfo?(lsalzman)
Duplicate of this bug: 1415148
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.
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.
Noting the duplicate bug is marked as sec-high so it seems likely this should also be rated sec-high.
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.
Assignee: nobody → bas
Attachment #8926431 - Flags: review?(dvander)
Attachment #8926431 - Attachment is patch: true
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.
Attachment #8926431 - Flags: review?(dvander) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/44e87c9281a4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Duplicate of this bug: 1415884
This signature is gone as per the 9th. Confirming our theory.
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.