Closed Bug 1416862 Opened 2 years ago Closed 2 years ago

Crash in mozilla::gfx::StoredPattern::Assign

Categories

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

58 Branch
x86
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 + fixed
firefox59 + fixed

People

(Reporter: philipp, Assigned: bas.schouten)

References

Details

(4 keywords, Whiteboard: [gfx-noted][post-critsmash-triage])

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is
report bp-36a47eef-d55c-412e-8768-23a0d0171112.
=============================================================

Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::gfx::StoredPattern::Assign(mozilla::gfx::Pattern const&) 	gfx/2d/DrawCommand.h:113
1 	xul.dll 	mozilla::gfx::FillRectCommand::FillRectCommand(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&) 	gfx/2d/DrawCommand.h:328
2 	xul.dll 	mozilla::layers::FillRectWithMask(mozilla::gfx::DrawTarget*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::SourceSurface*, mozilla::gfx::SamplingFilter, mozilla::gfx::DrawOptions const&, mozilla::gfx::ExtendMode, mozilla::gfx::SourceSurface*, mozilla::gfx::BaseMatrix<float> const*, mozilla::gfx::BaseMatrix<float> const*) 	gfx/layers/basic/BasicLayersImpl.cpp:148
3 	xul.dll 	mozilla::layers::FillRectWithMask(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::SourceSurface*, mozilla::gfx::SamplingFilter, mozilla::gfx::DrawOptions const&, mozilla::layers::Layer*) 	gfx/layers/basic/BasicLayersImpl.cpp:172
4 	xul.dll 	mozilla::layers::BasicCanvasLayer::Paint(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::layers::Layer*) 	gfx/layers/basic/BasicCanvasLayer.cpp:66
5 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp:711
6 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp:891
7 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp:731
8 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp:891
9 	xul.dll 	mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/basic/BasicLayerManager.cpp:616
10 	xul.dll 	mozilla::PaintInactiveLayer 	layout/painting/FrameLayerBuilder.cpp:3706
11 	xul.dll 	mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) 	layout/painting/FrameLayerBuilder.cpp:6020
12 	xul.dll 	mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) 	layout/painting/FrameLayerBuilder.cpp:6200
13 	xul.dll 	mozilla::layers::ClientPaintedLayer::PaintOffMainThread() 	gfx/layers/client/ClientPaintedLayer.cpp:243
14 	xul.dll 	mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) 	gfx/layers/client/ClientPaintedLayer.cpp:282
15 	xul.dll 	mozilla::layers::ClientContainerLayer::RenderLayer() 	gfx/layers/client/ClientContainerLayer.h:58
16 	xul.dll 	mozilla::layers::ClientLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) 	gfx/layers/client/ClientLayerManager.h:389
17 	xul.dll 	mozilla::layers::ClientContainerLayer::RenderLayer() 	gfx/layers/client/ClientContainerLayer.h:58
18 	xul.dll 	mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/client/ClientLayerManager.cpp:384
19 	xul.dll 	mozilla::FrameLayerBuilder::ComputeGeometryChangeForItem(mozilla::DisplayItemData*) 	layout/painting/FrameLayerBuilder.cpp:4637
20 	xul.dll 	mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/client/ClientLayerManager.cpp:442
21 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, gfxContext*, unsigned int) 	layout/painting/nsDisplayList.cpp:2548
22 	xul.dll 	RepeatOrStretchMirroredSurface 	gfx/thebes/gfxBlur.cpp:824
23 	xul.dll 	mozilla::SVGPathData::BuildPath(mozilla::gfx::PathBuilder*, unsigned char, float) 	dom/svg/SVGPathData.cpp:496
24 		@0x17fe7fff

crash reports with this signature are newly showing up on windows since 58.0a1, the first one was in build 20171101104430. most of the time the crashes are happening in the content process and for 32bit installations. a portion of them are UAF crashes.
Group: core-security → gfx-core-security
I see 14 out of 15 crashes in the past week are UAFs -- quite a portion!
Version: 57 Branch → 58 Branch
milan: you or jet/layout?
Flags: needinfo?(milan)
Graphics.
Ryan, Bas, feels like it's OMTP related?
Flags: needinfo?(rhunt)
Flags: needinfo?(milan)
Flags: needinfo?(bas)
Yup, definitely OMTP related.
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
This is very odd.. I'm guessing based on the stack the surface in this pattern is freed. But I don't understand yet how that could be.. David, have you seen anything like this?
Flags: needinfo?(dvander)
I'm thinking this may be related to bug 1416864. Let's see if that fixes it.
Huh, I'm not sure what's going on here.

I'm not sure if it's related to bug 1416864 because the feature notes has 'D2D? D2D1-'. I'm not sure why the stack frame cuts off at 24. JIT'ed code?

This could be something odd with AsyncCanvasRenderer because it looks like theres some interesting threading stuff in there, but I'm not familiar with that code.
Flags: needinfo?(rhunt)
I don't think it's bug 1416864 either. Keeping ni, I think we need to look at some crash dumps.
I agree, I noticed the D2D1- as well after I posted that comment.
I agree, fwiw, this probably has something to do with CopyableCanvasRenderer::ReadbackSurface()? But I wasn't involved with this code, so I'm still working on understanding how this is supposed to work.
So, I'd like to know on these systems, what codepath in CopyableCanvasRenderer::ReadbackSurface we are supposed to take, and if anyone has ideas on how that might lead us to get a race condition on a SourceSurface, that would be even better! :)
Flags: needinfo?(mtseng)
Flags: needinfo?(jmuizelaar)
Whiteboard: [gfx-noted]
Looking at the crash dump, surfPat->mSurface points to 0x150ecc0, this seems like a reasonable address based on the stack to contain the surface. When it loads the vtable ptr from that address the result it got was 0xe5e5e5e5, confirming the problem is a destroyed surface being passed in.
Based on what I'm seeing in the crash report this does not appear to be using the canvasRenderer codepath, but rather the PersistentBufferProvider codepath.
Flags: needinfo?(mtseng)
Flags: needinfo?(jmuizelaar)
I believe I've figured out the problem. Patch upcoming.
I strongly suspect this is yet another race condition surrounding mSnapshot.

I believe we're getting the Snapshot off a DrawTarget just after the paint thread is destroying that same Snapshot, since we've already transferred the pointer into a local before the Snapshot destructor cleared out mSnapshot, we'll then return the destroyed snapshot from DrawTargetSkia::Snapshot().
Flags: needinfo?(dvander)
Attachment #8931471 - Flags: review?(matt.woodrow)
Comment on attachment 8931471 [details] [diff] [review]
Ensure mSnapshot access is always protected by mSnapshotLock

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very difficult, this is a very rare race condition, one could attempt to trigger it by having a 1000 canvasses and drawing to them at once or something, but that still wouldn't really put you in a great place to actually exploit it. 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The patch itself does. Even without a comment it would be pretty obvious what's going on here.

Which older supported branches are affected by this flaw? None

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should apply to Beta as is.

How likely is this patch to cause regressions; how much testing does it need? Extremely unlikely, at the worst this would cause a performance regression but even that seems farfetched for a single, rarely contended lock.
Attachment #8931471 - Flags: sec-approval?
Attachment #8931471 - Flags: review?(matt.woodrow) → review+
sec-approval+. Please nominate for beta.
Attachment #8931471 - Flags: sec-approval? → sec-approval+
Comment on attachment 8931471 [details] [diff] [review]
Ensure mSnapshot access is always protected by mSnapshotLock

Approval Request Comment
[Feature/Bug causing the regression]: 1403935
[User impact if declined]: UAF
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No, in progress.
[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?]: Single line added grabbing a lock that is rarely contended with no deadlock risk.
[String changes made/needed]: None
Attachment #8931471 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/da1a37d96966
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: gfx-core-security → core-security-release
Comment on attachment 8931471 [details] [diff] [review]
Ensure mSnapshot access is always protected by mSnapshotLock

Fix a sec-high. Beta58+.
Attachment #8931471 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Bad news, although this may have helped, this crash does -not- appear to be fixed.
Status: RESOLVED → REOPENED
Flags: needinfo?(dvander)
Resolution: FIXED → ---
I'm an idiot. There's still an obvious race condition here, if Snapshot() grabs the snapshot lock when the destructor has already been started. When the snapshot lock is grabbed by the SourceSurface the DT will still get out the source surface that's in the process of being destroyed. I'm thinking about how we can address this.
So, I can only see two ways forward here, either we need to make SourceSurfaces use a virtual release method, that can be overloaded and hold the lock while release is called. This may be a simpler patch, but it seems like the less desirable solution.

The other option is to reverse the ownership model to the model I used originally in D2D. I don't know if there was ever a good reason it was done the other way around in Skia. The patch for this will be a little more complicated, it will need to bake on nightly for at least 2-3 days before we can uplift it.

I'm tempted to say the second solution is the best, and I will construct a patch for it.
For what it's worth, I think the Skia ownership model is kind of weird. I can see why it might make sense, but inverting it is a lot clearer.
Flags: needinfo?(dvander)
So the race is that: SourceSurfaceSkia's refcount drops to 0, but DrawTargetSkia wins the race to get the lock, and this brings the refcount back up to 1. On a dead object. That does seem likely, really the only option is that it should own a ref to the snapshot.

Do we still have the same problem if ownership is reversed? In that scenario, DrawTargetSkia owns a ref to the snapshot, and the snapshot does not own a ref to the DrawTarget. As long as the snapshot doesn't try to revive the DrawTarget, I think it's okay - it just has to acquire the lock before trying to use it, since potentially DrawTargetWillChange/MarkIndependent have been called while the lock is held.
Also, potentially a workaround for uplift (not a long-term solution) would be for DrawTargetSkia::Snapshot() to check if |mSnapshot && mSnapshot->refcount == 0|. That should be okay since, either we lost the race and mSnapshot is null, or we won the race and the object is not destroyed yet. The important thing is we just can't revive it.
(In reply to David Anderson [:dvander] from comment #27)
> Also, potentially a workaround for uplift (not a long-term solution) would
> be for DrawTargetSkia::Snapshot() to check if |mSnapshot &&
> mSnapshot->refcount == 0|. That should be okay since, either we lost the
> race and mSnapshot is null, or we won the race and the object is not
> destroyed yet. The important thing is we just can't revive it.

Nope, sadly, that's not true, suppose this scenario:

Thread 1:
EnterLock()
if (mSnapshot && mSnapshot->refCnt != 0) {

Thread 2:
if (--mRefCnt == 0) {
  delete this;

Thread 1:
RefPtr<SourceSurfaceSkia> snapshot = mSnapshot;

Thread 2:
SourceSurfaceSkia::~SourceSurfaceSkia()

You really have to be holding the lock while you do --mRefCnt == 0.
Flags: needinfo?(dvander)
Attached patch Reverse ownership model in Skia. (obsolete) — Splinter Review
Attachment #8934204 - Flags: review?(dvander)
(In reply to Bas Schouten (:bas.schouten) from comment #28)
> (In reply to David Anderson [:dvander] from comment #27)
> > Also, potentially a workaround for uplift (not a long-term solution) would
> > be for DrawTargetSkia::Snapshot() to check if |mSnapshot &&
> > mSnapshot->refcount == 0|. That should be okay since, either we lost the
> > race and mSnapshot is null, or we won the race and the object is not
> > destroyed yet. The important thing is we just can't revive it.
> 
> Nope, sadly, that's not true, suppose this scenario:
> 
> Thread 1:
> EnterLock()
> if (mSnapshot && mSnapshot->refCnt != 0) {
> 
> Thread 2:
> if (--mRefCnt == 0) {
>   delete this;
> 
> Thread 1:
> RefPtr<SourceSurfaceSkia> snapshot = mSnapshot;
> 
> Thread 2:
> SourceSurfaceSkia::~SourceSurfaceSkia()
> 
> You really have to be holding the lock while you do --mRefCnt == 0.

Oh, of course - you're right. Looping a compare-and-swap to increment the refcount could address that, I guess, where we'd stop trying if the refcount drops to 0.
Flags: needinfo?(dvander)
Comment on attachment 8934204 [details] [diff] [review]
Reverse ownership model in Skia.

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +295,5 @@
> +    MutexAutoLock lock(mSnapshotLock);
> +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> +    // keep the snapshot object alive so it doesn't get destroyed while
> +    // DrawTargetWillChange is running.
> +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;

I think we want to do something like DrawTargetCairo does instead. Call a function called MarkSnapshotIndependent or something, and then only call DrawTargetWillChange if the snapshot has > 1 ref. We'd want to do this in MarkChanged() as well.

Otherwise I think we're potentially going to copy a lot of surfaces.

@@ +2240,5 @@
>  {
> +  // I'm not entirely certain whether this lock is needed, as multiple threads
> +  // should never modify the DrawTarget at the same time anyway, but this seems
> +  // like the safest.
> +  MutexAutoLock lock(mSnapshotLock);

I agree - probably not needed, but harmless to have it.
Attachment #8934204 - Flags: review?(dvander)
(In reply to David Anderson [:dvander] from comment #31)
> Comment on attachment 8934204 [details] [diff] [review]
> Reverse ownership model in Skia.
> 
> Review of attachment 8934204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +295,5 @@
> > +    MutexAutoLock lock(mSnapshotLock);
> > +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> > +    // keep the snapshot object alive so it doesn't get destroyed while
> > +    // DrawTargetWillChange is running.
> > +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;
> 
> I think we want to do something like DrawTargetCairo does instead. Call a
> function called MarkSnapshotIndependent or something, and then only call
> DrawTargetWillChange if the snapshot has > 1 ref. We'd want to do this in
> MarkChanged() as well.
> 
> Otherwise I think we're potentially going to copy a lot of surfaces.
> I agree - probably not needed, but harmless to have it.

Oh, this is silly, I meant to just hand off the skSurface, not do a full MarkIndependent. I will correct this.
Attachment #8934204 - Attachment is obsolete: true
Attachment #8934629 - Flags: review?(dvander)
Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

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

Nice - this is so much simpler than the inverted model!

::: gfx/2d/DrawTargetSkia.cpp
@@ +295,5 @@
> +    MutexAutoLock lock(mSnapshotLock);
> +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> +    // keep the snapshot object alive so it doesn't get destroyed while
> +    // DrawTargetWillChange is running.
> +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;

I *think* this comment and the RefPtr are not necessary. GiveSurface doesn't call back into the DrawTarget, so there's no way the snapshot's refcount could drop to 0.

If you were calling MarkChanged or something then the RefPtr/comment would be necessary. Maybe it's worth keeping the RefPtr or leaving some kind of comment just in case someone does that in the future.

::: gfx/2d/SourceSurfaceSkia.h
@@ +61,5 @@
>    void DrawTargetWillChange();
>  
>    sk_sp<SkImage> mImage;
> +  // This keeps a surface alive if needed because its DrawTarget has gone away.
> +  sk_sp<SkSurface> mSurface;

Just so I understand - the SkImage is effectively a pointer to the SkSurface, so all we need to do is keep a reference to it alive?
Attachment #8934629 - Flags: review?(dvander) → review+
Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +291,5 @@
>  
>  DrawTargetSkia::~DrawTargetSkia()
>  {
> +  if (mSnapshot) {
> +    MutexAutoLock lock(mSnapshotLock);

Also, is the lock necessary here? I think not, *but* it might be necessary to lock mChangeMutex in DrawTargetSkia. Probably in GiveSurface().
err, mChangeMutex in SourceSurfaceSkia*
(In reply to David Anderson [:dvander] from comment #34)
> Comment on attachment 8934629 [details] [diff] [review]
> Reverse ownership model in Skia. v2
> 
> Review of attachment 8934629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice - this is so much simpler than the inverted model!
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +295,5 @@
> > +    MutexAutoLock lock(mSnapshotLock);
> > +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> > +    // keep the snapshot object alive so it doesn't get destroyed while
> > +    // DrawTargetWillChange is running.
> > +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;
> 
> I *think* this comment and the RefPtr are not necessary. GiveSurface doesn't
> call back into the DrawTarget, so there's no way the snapshot's refcount
> could drop to 0.
> 
> If you were calling MarkChanged or something then the RefPtr/comment would
> be necessary. Maybe it's worth keeping the RefPtr or leaving some kind of
> comment just in case someone does that in the future.

That was actually my reasoning. But you're right, I should improve the comment.

> ::: gfx/2d/SourceSurfaceSkia.h
> @@ +61,5 @@
> >    void DrawTargetWillChange();
> >  
> >    sk_sp<SkImage> mImage;
> > +  // This keeps a surface alive if needed because its DrawTarget has gone away.
> > +  sk_sp<SkSurface> mSurface;
> 
> Just so I understand - the SkImage is effectively a pointer to the
> SkSurface, so all we need to do is keep a reference to it alive?

The SkImage is a pointer to the -data- pointed to by the SkSurface, however the SkSurface is the object that -owns- that data in the 'normal' case.

(In reply to David Anderson [:dvander] from comment #35)
> Comment on attachment 8934629 [details] [diff] [review]
> Reverse ownership model in Skia. v2
> 
> Review of attachment 8934629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +291,5 @@
> >  
> >  DrawTargetSkia::~DrawTargetSkia()
> >  {
> > +  if (mSnapshot) {
> > +    MutexAutoLock lock(mSnapshotLock);
> 
> Also, is the lock necessary here? I think not, *but* it might be necessary
> to lock mChangeMutex in DrawTargetSkia. Probably in GiveSurface().

I don't think we do, as the surface should be threadsafe refcounted and this won't actually affect the content of the surface. Nor will any other party ever access mSurface on SourceSurfaceSkia.
https://hg.mozilla.org/mozilla-central/rev/286203c73311
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Please request approval on the new patch too once you're happy with how it's looking on m-c.
Flags: needinfo?(bas)
Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

Approval Request Comment
[Feature/Bug causing the regression]: OMTP
[User impact if declined]: UAF crash
[Is this code covered by automated tests?]: Extensively
[Has the fix been verified in Nightly?]: 2 days
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1423281
[Is the change risky?]: Slightly, but the code is very widely used so nightly testing is fairly solid.
[Why is the change risky/not risky?]: Changes an ownership pattern
[String changes made/needed]: None
Flags: needinfo?(bas)
Attachment #8934629 - Flags: approval-mozilla-beta?
Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

sec fix for omtp, beta58+
Attachment #8934629 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.