[webkit-reviews] review denied: [Bug 65858] OOB Read in WebCore::SVGAnimationElement : [Attachment 103357] 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 103357: Patch
https://bugs.webkit.org/attachment.cgi?id=103357&action=review

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