Bug 20397

Summary:

Invalid webkit-border-bottom-left-radius property causes crash

Product:

WebKit

Reporter:

Tavis Ormandy <taviso>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, gwilson, hyatt, mrowe

Priority:

Keywords:

HasReduction, InRadar

Version:

525.x (Safari 3.1)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Reduced testcase	none
Possible fix for bug 20397	eric: review-
Possible fix for bug 20397	eric: review+

Tavis Ormandy

Reported 2008-08-15 06:20:51 PDT

<html> <STYLE TYPE="text/css"> .test { -webkit-border-bottom-left-radius: 1 px; } </STYLE> </html>

Attachments
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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Mark Rowe (bdash)

Comment 1 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

Mark Rowe (bdash)

Comment 2 2008-08-15 07:00:48 PDT

<rdar://problem/6152273>

Glenn Wilson

Comment 3 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: -webkit-border-top-right-radius -webkit-border-top-left-radius -webkit-border-bottom-left-radius -webkit-border-bottom-right-radius -webkit-border-radius

Glenn Wilson

Comment 4 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().

Eric Seidel (no email)

Comment 5 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. checkForOrphanedUnits(); 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.

Glenn Wilson

Comment 6 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)

Eric Seidel (no email)

Comment 7 2008-08-21 20:11:41 PDT

Comment on attachment 22928 [details] Possible fix for bug 20397 great! Congrats on your first patch!

Mark Rowe (bdash)

Comment 8 2008-09-02 23:12:42 PDT

Landed in r36046.

Note You need to log in before you can comment on or make changes to this bug.