Bug 99243 – FEImage::m_document is never cleared. Why not?

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Adam Barth]

As far as I can tell, there's no mechanism for clearing FEImage::m_document after the Document has been destroyed.  Given that FEImage is RefCounted, it seems very likely that FEImage can be kept alive after the Document is destroyed.  I tried to chase down how we might be able to do that, but I got lost in SVG filter land.

[Stephen Chenney]

I'm in that space now. It's a complete disaster area. I'll try to figure this one out too.

[Stephen Chenney]

I don't think this is a problem. The document is only used when the filter is actually applied, and as far as I know you cannot apply a filter unless it is in the document. Furthermore, if the filter leaves the document the FilterEffect objects will be destroyed or marked dirty (forcing regeneration).

The RefCounted on such objects does not indicate that they are held outside of FilterData objects, rather it seems to be to simplify management of pointers in SVGFilterBuilder. The FEImage filter effect is SVG-only and is only held inside SVGFilterBuilder.

[Adam Barth]

Thanks for investigating this issue!  Would you be willing to add a comment explaining this to the m_document declaration so that we won't get scared the next time we audit this code?

[Stephen Chenney]

Reopened to comment appropriately.

[Stephen Chenney]

Created attachment 169239 [details]
Patch

Is this what you had in mind?

[Adam Barth]

Comment on attachment 169239 [details]
Patch

Yes, great.  I'll leave this patch for someone who can verify the comment to review.  Thanks!

[Stephen Chenney]

I added an assert to the destructor for the FEImage filter effect and verified that it is destructed when it's target is removed from the document. These tests demonstrate the behavior. The assert stacks show the FEImage being destructed whenever its target (the thing that will try to use it) is detached.

To summarize: Only a target renderer ever tries to use the m_document in the filter effect. When the target is added to the document the filter is created. When the target is removed from the document the filters are destroyed. JS cannot hold references to this data - it is entirely internal. Hence, there is no point at which a filter can exist without a valid document.

svg/filters/feImage-remove-target.svg
svg/filters/feImage-target-reappend-to-document.svg
svg/filters/feImage-target-remove-from-document.svg
svg/filters/feImage-target-add-to-document.svg
svg/filters/feImage-target-attribute-change-with-use-indirection.svg

Is that sufficient verification to R+ the comment?

[Dirk Schulze]

If it is wrong, why not fixing it? The comment is not really helpful. Make this bug no security bug anymore and fix it.

[Maciej Stachowiak]

If this pointer is not actually a dangling reference hazard, then there is nothing to fix. I can see the value of adding a comment explaining that, so no one goes on a wild goose chase, but it seems a bit verbose for WebKit style. Is there a way to get the point across more concisely? I think the comment can just state that the pointer can't be used to access free memory despite never being cleared, without giving the full detailed explanation.

[Stephen Chenney]

Created attachment 171475 [details]
Patch

[Stephen Chenney]

Maciej is right that this is never dangling and there's no point in fixing it. The only possible code change would be to explicitly set m_document to null before destroying the object, but there's no point at all in that.

I've come up with a one line comment (a bit longer). Actually, it just occurred to me that I should reference this bug, so I'll give it another pass.

Also, there is no need for this to be a security issue.

[Stephen Chenney]

Created attachment 171478 [details]
Patch

Much more concise, yet still informative.

[Eric Seidel (no email)]

Comment on attachment 171478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171478&action=review

> Source/WebCore/svg/graphics/filters/SVGFEImage.h:58
> +    // m_document will never be a dangling reference. See https://bugs.webkit.org/show_bug.cgi?id=99243

Better justification would be to explain why here as well.  This is not an Element, so it doesn't keep the document alive.  I assume this is held off of a renderer?  Can this also be held off of a CSSValue?

[Stephen Chenney]

Comment on attachment 171478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171478&action=review

>> Source/WebCore/svg/graphics/filters/SVGFEImage.h:58
>> +    // m_document will never be a dangling reference. See https://bugs.webkit.org/show_bug.cgi?id=99243
> 
> Better justification would be to explain why here as well.  This is not an Element, so it doesn't keep the document alive.  I assume this is held off of a renderer?  Can this also be held off of a CSSValue?

This object is only held by a SVGFilterBuilder which in turn is held by a RenderSVGResourceFilter::FilterData object, which in turn is held by a RenderSVGResourceFilter object which is a renderer. Nothing else at all holds references to FEImage. Any changes to CSS properties (including SVG attributes) that affect the filter cause the FilterData and hence FEImage object to be destroyed and recreated. CSS Filters cannot use SVGFEImage.

The only way that FEImage::m_document is used is to look up the renderer for the image it is drawing. It will only do that during applyResource, which is only called when the target for the filter (the thing that will draw it) is painted, which only happens if the target is in the document. If the target is in the document then FEImage::m_document is valid. If the target is not in the document then it both cannot be painted and there is no FEImage object to have a dangling reference.

[Dirk Schulze]

Comment on attachment 171478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171478&action=review

> Source/WebCore/ChangeLog:8
> +        Adding a comment to explain why the failure to clear m_document is not a problem.

Please say that this will be refactored with another patch.

>>> Source/WebCore/svg/graphics/filters/SVGFEImage.h:58
>>> +    // m_document will never be a dangling reference. See https://bugs.webkit.org/show_bug.cgi?id=99243
>> 
>> Better justification would be to explain why here as well.  This is not an Element, so it doesn't keep the document alive.  I assume this is held off of a renderer?  Can this also be held off of a CSSValue?
> 
> This object is only held by a SVGFilterBuilder which in turn is held by a RenderSVGResourceFilter::FilterData object, which in turn is held by a RenderSVGResourceFilter object which is a renderer. Nothing else at all holds references to FEImage. Any changes to CSS properties (including SVG attributes) that affect the filter cause the FilterData and hence FEImage object to be destroyed and recreated. CSS Filters cannot use SVGFEImage.
> 
> The only way that FEImage::m_document is used is to look up the renderer for the image it is drawing. It will only do that during applyResource, which is only called when the target for the filter (the thing that will draw it) is painted, which only happens if the target is in the document. If the target is in the document then FEImage::m_document is valid. If the target is not in the document then it both cannot be painted and there is no FEImage object to have a dangling reference.

Please add a link to this bug report, and open the bug report after landing again. Can be dependent on your refactoring patch later. After refactoring you just close it.

[Stephen Chenney]

Comment on attachment 171478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171478&action=review

>> Source/WebCore/ChangeLog:8
>> +        Adding a comment to explain why the failure to clear m_document is not a problem.
> 
> Please say that this will be refactored with another patch.

But it won't be refactored, at least not to my knowledge. The comment exists so that _if_ there is ever a patch that changes the behavior the patch author will know the situation. Or if anyone, like Adam, is doing a security review they will have context in which to understand why this is not a security problem.

>>>> Source/WebCore/svg/graphics/filters/SVGFEImage.h:58
>>>> +    // m_document will never be a dangling reference. See https://bugs.webkit.org/show_bug.cgi?id=99243
>>> 
>>> Better justification would be to explain why here as well.  This is not an Element, so it doesn't keep the document alive.  I assume this is held off of a renderer?  Can this also be held off of a CSSValue?
>> 
>> This object is only held by a SVGFilterBuilder which in turn is held by a RenderSVGResourceFilter::FilterData object, which in turn is held by a RenderSVGResourceFilter object which is a renderer. Nothing else at all holds references to FEImage. Any changes to CSS properties (including SVG attributes) that affect the filter cause the FilterData and hence FEImage object to be destroyed and recreated. CSS Filters cannot use SVGFEImage.
>> 
>> The only way that FEImage::m_document is used is to look up the renderer for the image it is drawing. It will only do that during applyResource, which is only called when the target for the filter (the thing that will draw it) is painted, which only happens if the target is in the document. If the target is in the document then FEImage::m_document is valid. If the target is not in the document then it both cannot be painted and there is no FEImage object to have a dangling reference.
> 
> Please add a link to this bug report, and open the bug report after landing again. Can be dependent on your refactoring patch later. After refactoring you just close it.

There is a link to this bug report. Did you mean something else?

Again, there is no plan for a future patch at this time.

[Dirk Schulze]

(In reply to comment #16)
> (From update of attachment 171478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171478&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +        Adding a comment to explain why the failure to clear m_document is not a problem.
> > 
> > Please say that this will be refactored with another patch.
> 
> But it won't be refactored, at least not to my knowledge. The comment exists so that _if_ there is ever a patch that changes the behavior the patch author will know the situation. Or if anyone, like Adam, is doing a security review they will have context in which to understand why this is not a security problem.
> 
> >>>> Source/WebCore/svg/graphics/filters/SVGFEImage.h:58
> >>>> +    // m_document will never be a dangling reference. See https://bugs.webkit.org/show_bug.cgi?id=99243
> >>> 
> >>> Better justification would be to explain why here as well.  This is not an Element, so it doesn't keep the document alive.  I assume this is held off of a renderer?  Can this also be held off of a CSSValue?
> >> 
> >> This object is only held by a SVGFilterBuilder which in turn is held by a RenderSVGResourceFilter::FilterData object, which in turn is held by a RenderSVGResourceFilter object which is a renderer. Nothing else at all holds references to FEImage. Any changes to CSS properties (including SVG attributes) that affect the filter cause the FilterData and hence FEImage object to be destroyed and recreated. CSS Filters cannot use SVGFEImage.
> >> 
> >> The only way that FEImage::m_document is used is to look up the renderer for the image it is drawing. It will only do that during applyResource, which is only called when the target for the filter (the thing that will draw it) is painted, which only happens if the target is in the document. If the target is in the document then FEImage::m_document is valid. If the target is not in the document then it both cannot be painted and there is no FEImage object to have a dangling reference.
> > 
> > Please add a link to this bug report, and open the bug report after landing again. Can be dependent on your refactoring patch later. After refactoring you just close it.
> 
> There is a link to this bug report. Did you mean something else?
> 
> Again, there is no plan for a future patch at this time.

Hm, ok. Then land it :P

[Stephen Chenney]

Committed r133158: <http://trac.webkit.org/changeset/133158>