Bug 45896 – CSS: Fix crash in getTimingFunctionValue()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

Since TimingFunctions are refcounted since http://trac.webkit.org/changeset/67032 we need to store them using RefPtr rather than TimingFunction*.

[Andreas Kling]

Created attachment 67803 [details]
Proposed patch

[Simon Fraser (smfr)]

Comment on attachment 67803 [details]
Proposed patch

Needs a testcase.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 67803 [details])
> Needs a testcase.

Right, sorry. This is already covered by existing tests, for example transitions/inherit-other-props.html. You'll only get an actual crash on picky platforms (or valgrind ;-))

[Simon Fraser (smfr)]

Comment on attachment 67803 [details]
Proposed patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 13da8c8..7277379 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,15 @@
> [phone]  Andreas Kling  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        CSS: Fix crash in getTimingFunctionValue()
> +        https://bugs.webkit.org/show_bug.cgi?id=45896
> +
> +        Use RefPtrs to avoid deleting the TimingFunctions prematurely.

You should say here why you didn't add any tests.

> diff --git a/WebCore/css/CSSComputedStyleDeclaration.cpp b/WebCore/css/CSSComputedStyleDeclaration.cpp
> index ce96e1c..f351cd7 100644
> --- a/WebCore/css/CSSComputedStyleDeclaration.cpp
> +++ b/WebCore/css/CSSComputedStyleDeclaration.cpp
> @@ -514,12 +514,12 @@ static PassRefPtr<CSSValue> getTimingFunctionValue(const AnimationList* animList
>      RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
>      if (animList) {
>          for (size_t i = 0; i < animList->size(); ++i) {
> -            const TimingFunction* tf = animList->animation(i)->timingFunction().get();
> +            RefPtr<TimingFunction> tf = animList->animation(i)->timingFunction();

I don't see why this RefPtr is needed. How can animList->animation(i)->timingFunction() go bad here?

[Andreas Kling]

Created attachment 67815 [details]
Proposed patch v2

Updated ChangeLog with information about test coverage.
Removed the unnecessary guard for Animation::timingFunction().

[Andreas Kling]

Comment on attachment 67815 [details]
Proposed patch v2

Clearing flags on attachment: 67815

Committed r67634: <http://trac.webkit.org/changeset/67634>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/8730620>

[Vincent Danen]

This was assigned CVE-2011-0113.