Bug 32171 - SVG patterns and masks should not be able to reference themselves
Summary: SVG patterns and masks should not be able to reference themselves
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 37751 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-12-04 13:58 PST by Dirk Schulze
Modified: 2010-07-15 01:25 PDT (History)
4 users (show)

See Also:


Attachments
Pattern calls itself (329 bytes, image/svg+xml)
2009-12-04 13:58 PST, Dirk Schulze
no flags Details
Initial patch (90.12 KB, patch)
2010-07-14 01:30 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (89.92 KB, patch)
2010-07-14 01:55 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch v2 (89.91 KB, patch)
2010-07-14 02:25 PDT, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff
New patch (23.73 KB, patch)
2010-07-15 01:04 PDT, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2009-12-04 13:58:17 PST
Created attachment 44333 [details]
Pattern calls itself

We need to make sure, that the content of a pattern/mask doesn't call the pattern/mask itself. This would cause infinity recursion, and webkit crashes. The <use> element already takes care about that.
Comment 1 Dirk Schulze 2010-04-19 03:26:23 PDT
*** Bug 37751 has been marked as a duplicate of this bug. ***
Comment 2 Nikolas Zimmermann 2010-07-14 01:30:46 PDT
Created attachment 61484 [details]
Initial patch
Comment 3 Eric Seidel 2010-07-14 01:39:41 PDT
Comment on attachment 61484 [details]
Initial patch

WebCore/rendering/RenderSVGResourceContainer.h:38
 +          , m_id(node->hasID() ? node->getIdAttribute() : nullAtom)
Is this testable?  Do we need a test for this change?

WebCore/rendering/RenderSVGResourceContainer.h:59
 +          m_id = static_cast<Element*>(node())->getIdAttribute();
Don't we have an asElement() call or similar?  Is this static_cast always safe?

WebCore/svg/SVGPaint.cpp:114
 +  bool SVGPaint::referencedResourceEqualsToID(SVGPaint* paint, const String& referenceId)
This isn't really english.  I think you might mean:

isURIWithTarget(target)

Why should this be a static method?  Seems it should be an instance method.

Otherwise looks good.
Comment 4 Nikolas Zimmermann 2010-07-14 01:46:39 PDT
(In reply to comment #3)
> (From update of attachment 61484 [details])
> WebCore/rendering/RenderSVGResourceContainer.h:38
>  +          , m_id(node->hasID() ? node->getIdAttribute() : nullAtom)
> Is this testable?  Do we need a test for this change?
No, this is not testable in SVG documents. The only difference is that the id would have been lowercased before, if the document would be in "compatibility mode", which is not the case for SVG.
 
> WebCore/rendering/RenderSVGResourceContainer.h:59
>  +          m_id = static_cast<Element*>(node())->getIdAttribute();
> Don't we have an asElement() call or similar?  Is this static_cast always safe?
Yes, it's always safe, RenderSVGResourceContains are only constructed for SVGStyledElements. See:
constructor "RenderSVGResourceContainer(SVGStyledElement* node)".

> 
> WebCore/svg/SVGPaint.cpp:114
>  +  bool SVGPaint::referencedResourceEqualsToID(SVGPaint* paint, const String& referenceId)
> This isn't really english.  I think you might mean:
> 
> isURIWithTarget(target)
> 
> Why should this be a static method?  Seems it should be an instance method.
Going to fix that, and name it "machesTargetURI".
Comment 5 Nikolas Zimmermann 2010-07-14 01:55:58 PDT
Created attachment 61489 [details]
Updated patch

Not yet tested build with the new changes, EWS is faster than my machine.
Comment 6 Nikolas Zimmermann 2010-07-14 02:25:32 PDT
Created attachment 61493 [details]
Updated patch v2

This one actually builds and works, tested locally. Turns out EWS was not faster today :-)
Comment 7 Eric Seidel 2010-07-14 02:26:57 PDT
Comment on attachment 61493 [details]
Updated patch v2

LGTM.  Do we need to rip out any of the old recursion prevention code?
Comment 8 Nikolas Zimmermann 2010-07-14 02:31:26 PDT
(In reply to comment #7)
> (From update of attachment 61493 [details])
> LGTM.  Do we need to rip out any of the old recursion prevention code?

We didn't have any recursion prevention code for masks/patterns until now. Only markers have special code, that can be modified in a follow-up patch (to integrate within this new logic)
Comment 9 Nikolas Zimmermann 2010-07-14 02:41:59 PDT
Landed in r63300.
Comment 10 Nikolas Zimmermann 2010-07-15 00:57:45 PDT
Pattern still have an issue with self-referencing, going to upload a fix soon, found by Dirk.
Comment 11 Nikolas Zimmermann 2010-07-15 01:04:25 PDT
Created attachment 61617 [details]
New patch

Forgot that patterns can be chained using xlink:href -> also catch recursion in those cases. Masks are not affected. Problem should really be fixed now.
Comment 12 Eric Seidel 2010-07-15 01:23:10 PDT
Comment on attachment 61617 [details]
New patch

OK.
Comment 13 Nikolas Zimmermann 2010-07-15 01:25:44 PDT
Landed in r63415.