[Webkit-unassigned] [Bug 63290] Stack overflow with enormous SVG filter

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63290





--- Comment #25 from Said Abou-Hallawa <sabouhallawa at apple.com>  2014-09-26 16:34:27 PST ---
(In reply to comment #24)
> (From update of attachment 238693 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238693&action=review
> 
> > 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.
>

Why do we have to return 0 in this case?  Should not we return 1 for feFlood and feTurbulence?  And I think this what is going to return according to your suggested solution below.

> 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). 

Why should we count (size of m_namedEffects)? I think we should not. These filters are exactly the same

<filter id="ShiftAndBlur">
  <feOffset dx="10" dy="10" />
  <feGaussianBlur stdDeviation="8.0" />
</filter>

And

<filter id="ShiftAndBlur">
  <feOffset in='SourceGraphic' result='ef1' dx="10" dy="10" />
  <feGaussianBlur in='ef1' result='ef2' stdDeviation="8.0" />
</filter>

But with the first filter (size of m_namedEffects) = 0 and with the second filter (size of m_namedEffects) = 2.

> 
> 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.

Yes appendEffectToEffectReferences is called from one place which is RenderSVGResourceFilter::buildPrimitives() with only a newly created effect.

But I am not sure if we need to keep HashSet<RefPtr<FilterEffect>> and add to it in appendEffectToEffectReferences().  I think this does not give us the information we need starting from the lastEffect.

How about this solution:

unsigned FilterEffect::collectEffects(const FilterEffect*effect, HashSet<const FilterEffect*>& allEffects)
{
    allEffects.add(effect);
    unsigned size = effect->numberOfEffectInputs();
    for (unsigned i = 0; i < size; ++i) {
        FilterEffect* in = effect->inputEffect(i);
        collectEffects(in, allEffects);
    }
    return allEffects.size();
}

unsigned FilterEffect::totalNumberOfEffectInputs() const
{
    HashSet<const FilterEffect*> allEffects;
    return collectEffects(this, allEffects);
}

And the caller will look like this:

if (!lastEffect || lastEffect->totalNumberOfEffectInputs() > maxtotalNumberOfEffectInputs)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.