1195968 - Check how CanvasFilterChainObserver accesses the CanvasRenderingContext2D

Status: RESOLVED FIXED

(Core :: Canvas: 2D, defect)

Description

[Markus Stange [:mstange]]

This needs to be checked:

(In reply to Mats Palmgren (:mats) from comment #25)
> I would also like to know more about CanvasFilterChainObserver.
> It has a raw CanvasRenderingContext2D* and calls UpdateFilter() on it,
> which seems unsafe unless we carefully tear these observers down
> before it goes away.  Where do we do that?

It's not immediately security critical because we're not creating CanvasFilterChainObserver unless canvas filters are enabled (using the pref canvas.filters.enabled), which they aren't currently by default.

Enabling them by default is tracked in bug 1173545; we need to fix this bug before we do that.

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Make sure to destroy CanvasFilterChainObservers

This makes sure we free CanvasFilterChainObservers properly. Should solve security concerns and fixes related memory leaks.

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8738903 [details] [diff] [review]
Make sure to destroy CanvasFilterChainObservers

I'm going to hand this review off to Mats.

Mats, the idea is that the observer is always torn down before the CanvasRenderingContext2D goes away. The only strong reference to the CanvasFilterChainObserver is the RefPtr in CanvasRenderingContext2D::ContextState. There is at least one other weak reference to it, in nsSVGFilterReference::mFilterChainObserver.

I'm not sure if this enough to address your concern, though. Should we make CanvasFilterChainObserver::mContext and nsSVGFilterReference::mFilterChainObserver RefPtrs, instead of raw pointers, and then add cycle collection unlink methods for those fields? I don't really know much about cycle collection so I could use some help here.

Bug 1062832 comment 7 is where I added the initial cycle collection code for these things.

Comment 3

[Markus Stange [:mstange]]

Attachment: graph of objects and references

Comment 4

[Mats Palmgren (:mats)]

Thanks for the object graph, that really helps!

After plowing through the code there's one issue I'm concerned with: nsSVGIDRenderingObserver
(which inherits nsSVGFilterReference; calls our CanvasFilterChainObserver::DoUpdate()).
The nsSVGIDRenderingObserver is a mutation observer, and it calls Add[Remove]MutationObserver
in its ctor/dtor.  So we *depend on* the dtor being called before the mContext object
(CanvasRenderingContext2D*) is being destroyed.  AFAICT, there are no other strong refs
to these objects so deleting the CanvasRenderingContext2D should also destroy these
observer objects.

It's a bit fragile though, and there's a lot of complex code involved.  One small mistake,
like holding a RefPtr on the stack to a ChainObserver or nsSVGFilterReference while
calling a method that might flush, could lead to a use-after-free.  It might be worth
adding some defense / assertion against this.

We could for example add a DetachFromContext() method on CanvasFilterChainObserver
that nulls out mContext, calling it from the CC unlink phase.  Then add a runtime abort
if mContext is null in DoUpdate().  We shouldn't get any observer callbacks beyond
that point, IIUC.

Comment 5

[Mats Palmgren (:mats)]

Comment on attachment 8738903 [details] [diff] [review]
Make sure to destroy CanvasFilterChainObservers

LGTM, but it wouldn't hurt with some extra precautions as suggested in
the last paragraph above.  r=mats

Comment 6

[Tobias Schneider [:tobytailor]]

Attachment: Make sure to destroy CanvasFilterChainObservers (v2)

Addressed review comments. Mats, please have a final look, since I had to move some code around.

Comment 7

[Mats Palmgren (:mats)]

Comment on attachment 8739499 [details] [diff] [review]
Make sure to destroy CanvasFilterChainObservers (v2)

+    if (!mContext)
+      return;

nit: please add {}

Is there any reason to believe this can actually happen?

I don't see any, so I think we can replace the "return" with a MOZ_CRASH,
but we could start with a MOZ_ASSERT_UNREACHABLE before the return if
we're not sure yet.

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: Make sure to destroy CanvasFilterChainObservers (v3)

Thanks for the reviews Mats! I changed the check for mContext to result in a MOZ_CRASH if null, since this should absolutely never happen. One last question here: Does this bug need to be made public before landing the patches?

Comment 9

[Mats Palmgren (:mats)]

sec-moderate bugs doesn't need approval before landing, so feel free to land.
https://wiki.mozilla.org/Security/Bug_Approval_Process

We generally never open security bugs before landing, so you should
leave this one closed.  Someone will eventually open it once the fix
has trickled down to all supported branches.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7045c2c74a1b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7045c2c74a1b