[webkit-reviews] review denied: [Bug 65858] OOB Read in WebCore::SVGAnimationElement : [Attachment 103397] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Ken Buchanan
<kenrb at chromium.org>'s request for review:
Bug 65858: OOB Read in WebCore::SVGAnimationElement
https://bugs.webkit.org/show_bug.cgi?id=65858

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.