RESOLVED FIXED 20397
Invalid webkit-border-bottom-left-radius property causes crash 
https://bugs.webkit.org/show_bug.cgi?id=20397
Summary Invalid webkit-border-bottom-left-radius property causes crash
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-
DetailsFormatted DiffDiff
Possible fix for bug 20397 (4.05 KB, patch)
2008-08-21 18:46 PDT, Glenn Wilson 		eric: review+
DetailsFormatted DiffDiff
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.