Bug 20397 - Invalid webkit-border-bottom-left-radius property causes crash
Summary: Invalid webkit-border-bottom-left-radius property causes crash
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Keywords: HasReduction, InRadar
Depends on:
Reported: 2008-08-15 06:20 PDT by Tavis Ormandy
Modified: 2008-09-02 23:12 PDT (History)
4 users (show)

See Also:

Reduced testcase (177 bytes, text/html)
2008-08-19 18:10 PDT, Glenn Wilson
no flags Details
Possible fix for bug 20397 (1.10 KB, patch)
2008-08-20 09:53 PDT, Glenn Wilson
eric: review-
Details | Formatted Diff | Diff
Possible fix for bug 20397 (4.05 KB, patch)
2008-08-21 18:46 PDT, Glenn Wilson
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tavis Ormandy 2008-08-15 06:20:51 PDT
<STYLE TYPE="text/css">
.test {
	-webkit-border-bottom-left-radius: 1 px;
Comment 1 Mark Rowe (bdash) 2008-08-15 06:56:57 PDT
Confirmed with TOT WebKit:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000010
0x022aa842 in WebCore::CSSParser::validUnit (value=0x0, unitflags=WebCore::CSSParser::FLength, strict=false) at WebCore/css/CSSParser.cpp:408
408	    switch(value->unit) {
(gdb) bt
#0  0x022aa842 in WebCore::CSSParser::validUnit (value=0x0, unitflags=WebCore::CSSParser::FLength, strict=false) at WebCore/css/CSSParser.cpp:408
#1  0x022baddb in WebCore::CSSParser::parseValue (this=0xbfffb2ec, propId=1149, important=false) at WebCore/css/CSSParser.cpp:1159
#2  0x022a0fb8 in cssyyparse (parser=0xbfffb2ec) at CSSGrammar.y:1211
#3  0x022bd4cf in WebCore::CSSParser::parseSheet (this=0xbfffb2ec, sheet=0x1b1d7300, string=@0xbfffb4a4) at WebCore/css/CSSParser.cpp:224
#4  0x02309ac2 in WebCore::CSSStyleSheet::parseString (this=0x1b1d7300, string=@0xbfffb4a4, strict=false) at WebCore/css/CSSStyleSheet.cpp:158
Comment 2 Mark Rowe (bdash) 2008-08-15 07:00:48 PDT
Comment 3 Glenn Wilson 2008-08-19 18:10:36 PDT
Created attachment 22885 [details]
Reduced testcase

Just put the testcase reduction into an attachment. If it passes, it will display 'PASS' in green.

Tested on Win XP SP2 w/ Safari 3.1, IE6 and FF2.

By the looks of the CSSParser.cpp, other CSS properties can cause this crash as well:
Comment 4 Glenn Wilson 2008-08-20 09:53:55 PDT
Created attachment 22895 [details]
Possible fix for bug 20397

This small change may fix the problem.

When these CSS properties are being processed by the CSSParser, the parser assumes that m_valueList->current is pointing at the start of the list.  In the case where there is a space between the number and unit ("1 px;"), m_valueList->current is actually pointing at the second element of the list ("px" or whatever unit, assumedly).  So when the parser automatically advances to m_valueList->next, it's over the end of the list, dereferences a null value, and bam!...crash-o-rama.

This fix checks that m_valueList->next exists.  If so, it continues as it did before.  If not, it assumes that the list is already pointing at the end of the list and continues without advancing the list with next().
Comment 5 Eric Seidel (no email) 2008-08-20 23:51:48 PDT
Comment on attachment 22895 [details]
Possible fix for bug 20397

Well, so unfortunately this fix is wrong.

I ran this in the debugger, and it turns out the problem is caused by:

    // In quirks mode, we will look for units that have been incorrectly separated from the number they belong to
    // by a space.  We go ahead and associate the unit with the number even though it is invalid CSS.

int num is initialized from m_valueList->size() before that call is made, and then not updated again after that call is made.  checkForOrphanedUnits can shrink the m_valueList, thus causing "num" to be wrong.  Thus causing (I'm sure) lots of crashes.

Also, unfortunately again, I would have had to r- your fix for lack of ChangeLog and use of tabs instead of spaces.

I'm not sure what the right fix is.  If we can just move checkForOrphanedUnits() above the original "num" initialization, or if we need to move num to below the checkForOrphanedUnits call.  I think we can just move checkForOrphaned units up.
Comment 6 Glenn Wilson 2008-08-21 18:46:32 PDT
Created attachment 22928 [details]
Possible fix for bug 20397

Aha...I see that I was attacking the symptom, and not the problem....and poorly at that :(

Here's a patch that should conform to style guidelines, includes changeLog entries, and has a layout test as well.  Please let me know if I missed anything.

This patch merely moves the call to checkForOrphanedUnits() earlier in the method.  The work that is done between num being set and the old call to checkForOrphanedUnits doesn't seem to conflict with the move.  

(The case that the orphaned unit identifier is somehow paired with the keywords "inherit" or "initial" -- this doesn't seem to be a problem because to do so would mean multiple items in m_valueList, but both cases return when there is more than one item in m_valueList)
Comment 7 Eric Seidel (no email) 2008-08-21 20:11:41 PDT
Comment on attachment 22928 [details]
Possible fix for bug 20397

great!  Congrats on your first patch!
Comment 8 Mark Rowe (bdash) 2008-09-02 23:12:42 PDT
Landed in r36046.