Bug 11221 – REGRESSION: iExploder crash due to style="cursor: url()"

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

WebKit crashes when opening the attached page from disk. Bug 11202 is related to this bug, as its testcase was created by reducing this one. However, that reduction unfortunately doesn't show the same behaviour as this original page, so I create this other bug.

Created attachment 10983 [details]
iExploder crasher

Download and open from disk to crash WebKit. It does here quite reliably on nightly r16878.

Created attachment 10984 [details]
Crash log

Created attachment 10985 [details]
Reduction (will crash)

<br style="cursor: url()">

[Rob Buis]

Created attachment 11021 [details]
Potential fix

This patch changes the logic slightly so any cursor value with a cursor list (even if
it amounts to an empty list like in the reduction) gets processed inside the if (list) {}.
Should fix the bug and still passes the webkits tests.
Cheers,

Rob.

[Rob Buis]

Created attachment 11031 [details]
Now with testcase

Added a testcase as suggested by Mitz.
Cheers,

Rob.

Comment on attachment 11031 [details]
Now with testcase

+            list = new CSSValueList; 

This will allocate a new CSSValueList on every iteration through the loop, throwing away the previously allocated one.

I noticed that the current code leaks a CSSValueList in a couple of places. One of them this patch fixes, another one is here:

            if ((strict && !value) || (value && !(value->unit == Value::Operator && value->iValue == ',')))
                return false;

(So for example, "cursor: url(cursor.png) ex" will leak a CSSValueList).

             if (strict || coords.size() == 0) {

This code after this 'if' is insufficiently indented, please clean it up.

In WebCore/ChangeLog, please add a line noting the test that goes with your patch. The usual format is "Test: fast/css/invalid-cursor-property-crash.html", right after the bug summary.

[Rob Buis]

Hi Mitz,

(In reply to comment #6)
> (From update of attachment 11031 [details] [edit])
> +            list = new CSSValueList; 

I clearly wasnt thinking :}
 
> This will allocate a new CSSValueList on every iteration through the loop,
> throwing away the previously allocated one.

See above :}
 
> I noticed that the current code leaks a CSSValueList in a couple of places. One
> of them this patch fixes, another one is here:
> 
>             if ((strict && !value) || (value && !(value->unit ==
> Value::Operator && value->iValue == ',')))
>                 return false;

Yep, the state after bug 6002 shows that there were some issues left in the code. I guess the review should have been better, and I should have also studied this (tricky) code better.

> (So for example, "cursor: url(cursor.png) ex" will leak a CSSValueList).
> 
>              if (strict || coords.size() == 0) {
> 
> This code after this 'if' is insufficiently indented, please clean it up.

Ok.

> In WebCore/ChangeLog, please add a line noting the test that goes with your
> patch. The usual format is "Test: fast/css/invalid-cursor-property-crash.html",
> right after the bug summary.

Ok. Will try to make a new patch asap.
Cheers,

Rob.

[Rob Buis]

Created attachment 11032 [details]
Improved patch

This one should address Mitz' issues with the patch before this one.
Cheers,

Rob.

Comment on attachment 11032 [details]
Improved patch

r=me

[Rob Buis]

Landed in r16991.