[webkit-reviews] review denied: [Bug 63290] Stack overflow with enormous SVG filter : [Attachment 238693] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 63290: Stack overflow with enormous SVG filter
https://bugs.webkit.org/show_bug.cgi?id=63290

Attachment 238693: Patch
https://bugs.webkit.org/attachment.cgi?id=238693&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238693&action=review


> Source/WebCore/ChangeLog:20
> +	   -- Do not build a filter if its has more than 200 FilterEffect's in
its map.

Typo: its -> it
Typo: FilterEffects

> Source/WebCore/ChangeLog:22
> +	   -- Discard a filter after it has more than 100 FilterEffect's in its
tree.

Typo: FilterEffects

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:116
> +unsigned FilterEffect::numberOfEffectInputsRecursive() const
> +{
> +    unsigned size = m_inputEffects.size();
> +    unsigned count = 0;
> +    for (unsigned i = 0; i < size; ++i) {
> +	   FilterEffect* in = m_inputEffects.at(i).get();
> +	   count += in->numberOfEffectInputsRecursive();
> +    }
> +    return count + 1;
> +}

This isn't quite right.  e.g. feFlood and feTurbulence don't have any input
effects, so we need to return 0 if the m_inputEffects.size() is 0.

But also, this could count the same effect multiple times - it could be the
input to more than one later step. Doing this properly is a bit tricky :( I'll
make another comment down where we might be able to count properly (in
RenderSVGResourceFilter.cpp).

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:181
> -    if (!lastEffect)
> +    if (!lastEffect || lastEffect->numberOfEffectInputsRecursive() >
maxNumberOfEffectInputsRecursive)

OK, so we have to recursively count the input effects without duplicates.
Luckily the builder has a map of all the named inputs and effect references, so
it already has this information, but we just need to get it to expose it.

I think you should add this to SVGFilterBuilder. You'd keep a
HashSet<RefPtr<FilterEffect>> that is added to in
appendEffectToEffectReferences, and then a method to expose
totalNumberOfInputs() or something like that. It would be the size of that set,
plus the named inputs (size of m_namedEffects), plus the built-in effects
(m_builtInEffects). 

You should double check that appendEffectToEffectReferences is not called for
the named or built-in effects though. If it is, then we just need the size of
our new set.