[Webkit-unassigned] [Bug 65858] OOB Read in WebCore::SVGAnimationElement

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #103397|review?                     |review-
               Flag|                            |




--- Comment #8 from Nikolas Zimmermann <zimmermann at kde.org>  2011-08-12 23:26:13 PST ---
(From update of attachment 103397)
View in context: https://bugs.webkit.org/attachment.cgi?id=103397&action=review

Almost there, still some tweaks needed.

> Source/WebCore/svg/SVGAnimationElement.cpp:182
> +        if (calcMode() == CalcModeSpline && !m_keySplines.isEmpty() && m_keySplines.size() != m_keyTimes.size() - 1) {
> +            // There is an array size mismatch between keySplines and keyTimes
> +            m_keyTimes.clear();
> +        }

In general, this looks fine - it would be nice to avoid the duplication though, by adding a validateKeyTimes() method that could be called from here.
I still wonder whether parseMappedAttribute is the right place though.

Looking through the code I see stuff like

float SVGAnimationElement::calculatePercentFromKeyPoints(float percent) const
{
    ASSERT(!m_keyPoints.isEmpty());
    ASSERT(calcMode() != CalcModePaced);
    ASSERT(m_keyTimes.size() > 1);
...

and or
    } else if (!m_keyPoints.isEmpty() && mode != CalcModePaced)
        effectivePercent = calculatePercentFromKeyPoints(percent);
    else if (m_keyPoints.isEmpty() && mode == CalcModeSpline && m_keyTimes.size() > 1)
...

It should be easy to trigger an assertion now, when combinining keyTimes/keySplines (with length mismatch) and keyPoints, as this code assumes keyTimes is never empty.
Do you agree that eg. non-paced mode now needs additional logic like (if m_keyTimes.size() > 1) before calling this method.

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