20397 2008-08-15 06:20:51 -0700 Invalid webkit-border-bottom-left-radius property causes crash 2008-09-02 23:12:42 -0700 1 1 1 Unclassified WebKit CSS 525.x (Safari 3.1) PC OS X 10.5 RESOLVED FIXED HasReduction, InRadar P2 Normal --- 1 taviso webkit-unassigned bdakin gwilson hyatt mrowe oldest_to_newest 88555 0 taviso 2008-08-15 06:20:51 -0700 <html> <STYLE TYPE="text/css"> .test { -webkit-border-bottom-left-radius: 1 px; } </STYLE> </html> 88564 1 mrowe 2008-08-15 06:56:57 -0700 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 88565 2 mrowe 2008-08-15 07:00:48 -0700 <rdar://problem/6152273> 88878 3 22885 gwilson 2008-08-19 18:10:36 -0700 Created attachment 22885 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 88920 4 22895 gwilson 2008-08-20 09:53:55 -0700 Created attachment 22895 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(). 88993 5 22895 eric 2008-08-20 23:51:48 -0700 Comment on attachment 22895 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. 89047 6 22928 gwilson 2008-08-21 18:46:32 -0700 Created attachment 22928 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) 89055 7 22928 eric 2008-08-21 20:11:41 -0700 Comment on attachment 22928 Possible fix for bug 20397 great! Congrats on your first patch! 89977 8 mrowe 2008-09-02 23:12:42 -0700 Landed in r36046. 22885 2008-08-19 18:10:36 -0700 2008-08-19 18:10:36 -0700 Reduced testcase bug-20397.html text/html 177 gwilson PGh0bWw+DQo8U1RZTEUgVFlQRT0idGV4dC9jc3MiPg0KLnRlc3Qgew0KCSAgY29sb3I6IGdyZWVu Ow0KICAgICAgICAtd2Via2l0LWJvcmRlci1ib3R0b20tbGVmdC1yYWRpdXM6IDEgcHg7DQp9DQo8 L1NUWUxFPg0KPGJvZHk+DQo8ZGl2IGNsYXNzPSJ0ZXN0Ij5QQVNTPC9kaXY+DQo8L2JvZHk+DQo8 L2h0bWw+ 22895 2008-08-20 09:53:55 -0700 2008-08-21 18:46:32 -0700 Possible fix for bug 20397 patch20397.txt text/plain 1123 gwilson SW5kZXg6IFdlYkNvcmUvY3NzL0NTU1BhcnNlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9j c3MvQ1NTUGFyc2VyLmNwcAkocmV2aXNpb24gMzU4NTUpCisrKyBXZWJDb3JlL2Nzcy9DU1NQYXJz ZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00MDUsNiArNDA1LDcgQEAgYm9vbCBDU1NQYXJzZXI6 OnZhbGlkVW5pdChDU1NQYXJzZXJWYWx1ZQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKICAgICBi b29sIGIgPSBmYWxzZTsKKwkKICAgICBzd2l0Y2godmFsdWUtPnVuaXQpIHsKICAgICBjYXNlIENT U1ByaW1pdGl2ZVZhbHVlOjpDU1NfTlVNQkVSOgogICAgICAgICBiID0gKHVuaXRmbGFncyAmIEZO dW1iZXIpOwpAQCAtMTE1NSw4ICsxMTU2LDEwIEBAIGJvb2wgQ1NTUGFyc2VyOjpwYXJzZVZhbHVl KGludCBwcm9wSWQsIGIKICAgICAgICAgUmVmUHRyPENTU1ByaW1pdGl2ZVZhbHVlPiBwYXJzZWRW YWx1ZTEgPSBDU1NQcmltaXRpdmVWYWx1ZTo6Y3JlYXRlKHZhbHVlLT5mVmFsdWUsIChDU1NQcmlt aXRpdmVWYWx1ZTo6VW5pdFR5cGVzKXZhbHVlLT51bml0KTsKICAgICAgICAgUmVmUHRyPENTU1By aW1pdGl2ZVZhbHVlPiBwYXJzZWRWYWx1ZTI7CiAgICAgICAgIGlmIChudW0gPT0gMikgewotICAg ICAgICAgICAgdmFsdWUgPSBtX3ZhbHVlTGlzdC0+bmV4dCgpOwotICAgICAgICAgICAgdmFsaWRf cHJpbWl0aXZlID0gdmFsaWRVbml0KHZhbHVlLCBGTGVuZ3RoLCBtX3N0cmljdCk7CisgICAgICAg ICAgICBDU1NQYXJzZXJWYWx1ZSogdmFsdWUyID0gbV92YWx1ZUxpc3QtPm5leHQoKTsKKwkJCWlm KHZhbHVlMikKKwkJCQl2YWx1ZSA9IHZhbHVlMjsKKwkJCXZhbGlkX3ByaW1pdGl2ZSA9IHZhbGlk VW5pdCh2YWx1ZSwgRkxlbmd0aCwgbV9zdHJpY3QpOwogICAgICAgICAgICAgaWYgKCF2YWxpZF9w cmltaXRpdmUpCiAgICAgICAgICAgICAgICAgcmV0dXJuIGZhbHNlOwogICAgICAgICAgICAgcGFy c2VkVmFsdWUyID0gQ1NTUHJpbWl0aXZlVmFsdWU6OmNyZWF0ZSh2YWx1ZS0+ZlZhbHVlLCAoQ1NT UHJpbWl0aXZlVmFsdWU6OlVuaXRUeXBlcyl2YWx1ZS0+dW5pdCk7Cg== 22928 2008-08-21 18:46:32 -0700 2008-08-21 20:11:41 -0700 Possible fix for bug 20397 patch20397-2.txt text/plain 4150 gwilson SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzNTg4MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjMgQEAKKzIwMDgtMDgtMjEgIEdsZW5uIFdpbHNvbiAgPHdpbHNvbmdAZ21haWwu Y29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZp eGVkIGJ1ZyAjMjAzOTcuICBTb21lIGNzcyBhdHRyaWJ1dGVzIHdpdGggb3JwaGFuZWQgdW5pdHMg d291bGQgY2F1c2UgYSBjcmFzaC4KKyAgICAgICAgRm9yIGV4YW1wbGUsICItd2Via2l0LWJvcmRl ci1ib3R0b20tbGVmdC1yYWRpdXM6IDEgcHg7IiB3b3VsZCBjYXVzZSB0aGlzIHByb2JsZW0uCisK KyAgICAgICAgVGhlIGZ1bmN0aW9uIGNoZWNrRm9yT3JwaGFuZWRVbml0cygpIHdvdWxkIGNoYW5n ZSB0aGUgbGVuZ3RoIG9mIGEgbGlzdCB3aG9zZSBzaXplIHdhcworICAgICAgICB3YXMgYWxyZWFk eSBkZXRlcm1pbmVkIGJlZm9yZSB0aGUgY2FsbCB0byBjaGVja0Zvck9ycGhhbmVkVW5pdHMgd2Fz IG1hZGUuICBMYXRlciBpbiAKKyAgICAgICAgdGhlIGNhbGxlciwgdGhlIG9sZCBzaXplIHdhcyBi ZWluZyB1c2VkIGZvciBib3VuZGFyeSBtYW5hZ2VtZW50LiAgCisKKyAgICAgICAgVGhpcyBoYXMg YmVlbiBmaXhlZCBieSBtb3ZpbmcgdGhlIGNhbGwgdG8gY2hlY2tGb3JPcnBoYW5lZFVuaXRzKCkg ZWFybGllciBpbiB0aGUgCisgICAgICAgIGNhbGxpbmcgZnVuY3Rpb24sIGJlZm9yZSB0aGUgc2l6 ZSBvZiB0aGUgbGlzdCBpcyBkZXRlcm1pbmVkLiAKKworICAgICAgICBUZXN0OiBmYXN0L2Nzcy9v cnBoYW5lZF91bml0c19jcmFzaC5odG1sCisKKyAgICAgICAgKiBDaGFuZ2VMb2c6CisgICAgICAg ICogY3NzL0NTU1BhcnNlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDU1NQYXJzZXI6OnBhcnNl VmFsdWUpOiAgTW92ZWQgY2FsbCB0byBjaGVja0Zvck9ycGhhbmVkVW5pdHMoKSBlYXJsaWVyIGlu IHRoZSBmdW5jdGlvbgorCiAyMDA4LTA4LTIxICBNYXhpbWUgQnJpdHRvICA8YnJpdHRvQGFwcGxl LmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBLZXZpbiBNY0N1bGxvdWdoLgpJbmRleDogV2Vi Q29yZS9jc3MvQ1NTUGFyc2VyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2Nzcy9DU1NQYXJz ZXIuY3BwCShyZXZpc2lvbiAzNTg3OCkKKysrIFdlYkNvcmUvY3NzL0NTU1BhcnNlci5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTUyOSw2ICs1MjksMTAgQEAgYm9vbCBDU1NQYXJzZXI6OnBhcnNlVmFs dWUoaW50IHByb3BJZCwgYgogCiAgICAgaW50IGlkID0gdmFsdWUtPmlkOwogCisgICAgLy8gSW4g cXVpcmtzIG1vZGUsIHdlIHdpbGwgbG9vayBmb3IgdW5pdHMgdGhhdCBoYXZlIGJlZW4gaW5jb3Jy ZWN0bHkgc2VwYXJhdGVkIGZyb20gdGhlIG51bWJlciB0aGV5IGJlbG9uZyB0bworICAgIC8vIGJ5 IGEgc3BhY2UuICBXZSBnbyBhaGVhZCBhbmQgYXNzb2NpYXRlIHRoZSB1bml0IHdpdGggdGhlIG51 bWJlciBldmVuIHRob3VnaCBpdCBpcyBpbnZhbGlkIENTUy4KKyAgICBjaGVja0Zvck9ycGhhbmVk VW5pdHMoKTsKKyAgICAKICAgICBpbnQgbnVtID0gaW5TaG9ydGhhbmQoKSA/IDEgOiBtX3ZhbHVl TGlzdC0+c2l6ZSgpOwogCiAgICAgaWYgKGlkID09IENTU1ZhbHVlSW5oZXJpdCkgewpAQCAtNTU1 LDEwICs1NTksNiBAQCBib29sIENTU1BhcnNlcjo6cGFyc2VWYWx1ZShpbnQgcHJvcElkLCBiCiAg ICAgYm9vbCB2YWxpZF9wcmltaXRpdmUgPSBmYWxzZTsKICAgICBSZWZQdHI8Q1NTVmFsdWU+IHBh cnNlZFZhbHVlOwogCi0gICAgLy8gSW4gcXVpcmtzIG1vZGUsIHdlIHdpbGwgbG9vayBmb3IgdW5p dHMgdGhhdCBoYXZlIGJlZW4gaW5jb3JyZWN0bHkgc2VwYXJhdGVkIGZyb20gdGhlIG51bWJlciB0 aGV5IGJlbG9uZyB0bwotICAgIC8vIGJ5IGEgc3BhY2UuICBXZSBnbyBhaGVhZCBhbmQgYXNzb2Np YXRlIHRoZSB1bml0IHdpdGggdGhlIG51bWJlciBldmVuIHRob3VnaCBpdCBpcyBpbnZhbGlkIENT Uy4KLSAgICBjaGVja0Zvck9ycGhhbmVkVW5pdHMoKTsKLQogICAgIHN3aXRjaCAoc3RhdGljX2Nh c3Q8Q1NTUHJvcGVydHlJRD4ocHJvcElkKSkgewogICAgICAgICAvKiBUaGUgY29tbWVudCB0byB0 aGUgbGVmdCBkZWZpbmVzIGFsbCB2YWxpZCB2YWx1ZSBvZiB0aGlzIHByb3BlcnRpZXMgYXMgZGVm aW5lZAogICAgICAgICAgKiBpbiBDU1MgMiwgQXBwZW5kaXggRi4gUHJvcGVydHkgaW5kZXgKSW5k ZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFu Z2VMb2cJKHJldmlzaW9uIDM1ODgyKQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMDgtMDgtMjEgIEdsZW5uIFdpbHNvbiAgPHdpbHNv bmdAZ21haWwuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIEFkZGVkIG5ldyB0ZXN0cyB0byBjaGVjayB3aGV0aGVyIGFuIG9ycGhhbmVkIHVuaXQg aWRlbnRpZmllcgorICAgICAgICBpbiBwYXJ0aWN1bGFyIGNzcyBhdHRyaWJ1dGVzIHdpbGwgY3Jh c2ggdGhlIGJyb3dzZXIuCisKKyAgICAgICAgKiBmYXN0L2Nzcy9vcnBoYW5lZF91bml0c19jcmFz aC1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGZhc3QvY3NzL29ycGhhbmVkX3VuaXRz X2NyYXNoLmh0bWw6IEFkZGVkLgorCiAyMDA4LTA4LTIxICBNYXhpbWUgQnJpdHRvICA8YnJpdHRv QGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBLZXZpbiBNY0N1bGxvdWdoLgpJbmRl eDogTGF5b3V0VGVzdHMvZmFzdC9jc3Mvb3JwaGFuZWRfdW5pdHNfY3Jhc2gtZXhwZWN0ZWQudHh0 Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvY3NzL29ycGhhbmVkX3VuaXRzX2NyYXNo LWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2Zhc3QvY3NzL29ycGhh bmVkX3VuaXRzX2NyYXNoLWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKQEAgLTAsMCArMSwyIEBA CitUaGlzIHRlc3QgdmVyaWZpZXMgdGhhdCBzdHlsZXMgd2l0aCBvcnBoYW5lZCB1bml0cyBkbyBu b3QgY3Jhc2ggdGhlIGJyb3dzZXIuIFlvdSBzaG91bGQgc2VlIHRoZSB3b3JkICJQQVNTIiBiZWxv dy4KK1BBU1MKSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvY3NzL29ycGhhbmVkX3VuaXRzX2NyYXNo Lmh0bWwKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvZmFzdC9jc3Mvb3JwaGFuZWRfdW5pdHNf Y3Jhc2guaHRtbAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2Zhc3QvY3NzL29ycGhhbmVk X3VuaXRzX2NyYXNoLmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMTggQEAKKzxodG1sPgor PGhlYWQ+CisgICAgPHN0eWxlPgorICAgICAgICAudGVzdCB7CisgICAgICAgICAgICAtd2Via2l0 LWJvcmRlci1ib3R0b20tbGVmdC1yYWRpdXM6IDEgcHg7CisgICAgICAgIH0KKyAgICA8L3N0eWxl PgorICAgIDxzY3JpcHQgbGFuZ3VhZ2U9ImphdmFzY3JpcHQiPgorICAgICAgICBpZiAod2luZG93 LmxheW91dFRlc3RDb250cm9sbGVyKQorICAgICAgICAgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIu ZHVtcEFzVGV4dCgpOworICAgIDwvc2NyaXB0PgorPC9oZWFkPgorPGJvZHk+CitUaGlzIHRlc3Qg dmVyaWZpZXMgdGhhdCBzdHlsZXMgd2l0aCBvcnBoYW5lZCB1bml0cyBkbyBub3QgY3Jhc2ggdGhl IGJyb3dzZXIuICBZb3Ugc2hvdWxkIHNlZSB0aGUgd29yZCAiUEFTUyIgYmVsb3cuCisgICAgPGRp diBjbGFzcz0idGVzdCI+UEFTUzwvZGl2PgorPC9ib2R5PgorPC9odG1sPgorCg==