[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 #103357|review?                     |review-
               Flag|                            |




--- Comment #5 from Nikolas Zimmermann <zimmermann at kde.org>  2011-08-09 11:11:32 PST ---
(From update of attachment 103357)
View in context: https://bugs.webkit.org/attachment.cgi?id=103357&action=review

Thanks that you're working on this Ken, first round of comments:

> LayoutTests/ChangeLog:5
> +        Merge branch 'master' into SVGbug

What's this?

> LayoutTests/ChangeLog:7
> +        Added some additional checking on on spline animation vector sizes, for potential mismatches with other vectors.

This doesn't say what's actually wrong in trunk. Can you rephrase that?

> LayoutTests/ChangeLog:11
> +

The bug title should come first. Like this:

OOB Read
https:://

Reviewed by NOBODY...

Added testcase covering keySpline array length problem.

> LayoutTests/svg/animations/animate-calcMode-spline-crash-bad-array-length.xhtml:23
> +                    document.body.innerHTML = "PASS";

PASS, if no assertion is triggered in a debug build.
Sounds better, eh?

> Source/WebCore/ChangeLog:5
> +        Merge branch 'master' into SVGbug

See the comments about LayoutTests/ChangeLog, please fix the WebCore ChangeLog as well.

> Source/WebCore/svg/SVGAnimationElement.cpp:459
> +    if (calcMode() == CalcModeSpline && index < m_keySplines.size()) {
>          ASSERT(m_keySplines.size() == m_keyPoints.size() - 1);

What's the condition for index >= m_keySplines.size(), when does that happen?

> Source/WebCore/svg/SVGAnimationElement.cpp:535
> -    if (calcMode == CalcModeSpline) {
> +    if (calcMode == CalcModeSpline && index < m_keySplines.size()) {

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:608
> +    else if (m_keyPoints.isEmpty() && mode == CalcModeSpline && m_keyTimes.size() > 1 && m_keySplines.size() == m_keyTimes.size() - 1)

Shouldn't we rather guarantee that if m_keyTimes.size() > 1, that m_keySplines.size() is always m_keyTimes.size() - 1 ?
If there's such a mismatch we could clear m_keySplines/keyTimes instead of keeping "invalid states" around and checking for them in various places (calculatePercentFromKeyPoints/currentValuesForValuesAnimation/updateAnimation). Seems too fragile for me.

What do you think?

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