31475 2009-11-13 09:02:44 -0800 Crash in StringHash::equal due to unaligned string data 2010-04-06 07:46:57 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Critical --- 1 yong.li.webkit webkit-unassigned abarth ap darin dave+webkit hausmann joenotcharles levin staikos steveblock oldest_to_newest 163331 0 yong.li.webkit 2009-11-13 09:02:44 -0800 We've found a cash on espn.go.com. The reason is: 1. StringHash::equal assumes String::characters() is aligned to 4-byte boundary. 2. When PassRefPtr<StringImpl> StringImpl::create(const JSC::UString& str) uses shared buffer with UString, m_data is not guaranteed to be 4-byte aligned. because UString::Rep::data() can point to any offset of the internal buffer. The solution that Dave Tapuska suggests is: When UString::data() is not aligned to 4-byte, we just don't use the shared buffer. Anyone please give some comments? 163432 1 levin 2009-11-13 12:20:48 -0800 > The solution that Dave Tapuska suggests is: When UString::data() is not aligned > to 4-byte, we just don't use the shared buffer. > > Anyone please give some comments? Tricky. I created this bug unfortunately. I can see at least two solutions: 1. Dave Tapuska;s suggestion. 2. Change StringHash::Equal to use memcmp You could try each solution separately in a ship build and run drameo and see which has less of a perf impact. I suspect that #1 is the better option. 163439 2 yong.li.webkit 2009-11-13 12:27:49 -0800 (In reply to comment #1) > > The solution that Dave Tapuska suggests is: When UString::data() is not aligned > > to 4-byte, we just don't use the shared buffer. > > > > Anyone please give some comments? > > Tricky. I created this bug unfortunately. > > I can see at least two solutions: > 1. Dave Tapuska;s suggestion. > 2. Change StringHash::Equal to use memcmp > > You could try each solution separately in a ship build and run drameo and see > which has less of a perf impact. > > I suspect that #1 is the better option. Yeah, that's what we tried, and either way can fix the problem. But we haven't run any performance test so far. BTW, the performance affect may rely on platform/compiler. memcmp (or wmemcmp?) could probably be optimized with some inline code by compiler. 163456 3 darin 2009-11-13 12:54:36 -0800 It's easy to make an alternate StringHash::equal in a way that does not depend on type punning in a non-portable way. The performance optimization of comparing four bytes at a time is not needed just to have WebKit. So I suggest starting by changing StringHash::equal to only do the optimization on platforms where it is safe. The other question, though, is whether the unaligned strings are having some sort of performance cost on platforms where the optimization is safe. 163458 4 joenotcharles 2009-11-13 12:58:56 -0800 This thread discusses the issue, but it seems their conclusion was never adopted: http://lists.macosforge.org/pipermail/webkit-dev/2009-July/008630.html 163463 5 yong.li.webkit 2009-11-13 13:13:57 -0800 Another thing about StringImpl (not related to this bug): newUCharVector() is defined, but never used. deleteUCharVector() is used, but I don't think it ever gets called because m_data is never a standalone memory block. So need some cleanup. Probably I should raise another bug for that? 163523 6 43204 dave+webkit 2009-11-13 14:25:31 -0800 Created attachment 43204 Apply defines to StringHash::Equal 163606 7 43204 commit-queue 2009-11-13 17:24:13 -0800 Comment on attachment 43204 Apply defines to StringHash::Equal Rejecting patch 43204 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11623 test cases. http/tests/xmlhttprequest/workers/shared-worker-close.html -> crashed Exiting early after 1 failures. 9309 tests run. 363.31s total testing time 9308 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output 163799 8 43204 abarth 2009-11-15 15:34:17 -0800 Comment on attachment 43204 Apply defines to StringHash::Equal Clearing flags on attachment: 43204 Committed r51006: <http://trac.webkit.org/changeset/51006> 163800 9 abarth 2009-11-15 15:34:24 -0800 All reviewed patches have been landed. Closing bug. 165487 10 ap 2009-11-20 16:07:43 -0800 *** Bug 31726 has been marked as a duplicate of this bug. *** 209044 11 hausmann 2010-04-06 07:46:57 -0700 Cherry-picked into QtWebKit for Qt 4.6 with commit e3dc4ef2b801d91e115c54f833fa7766d392ceda 43204 2009-11-13 14:25:31 -0800 2009-11-15 15:34:17 -0800 Apply defines to StringHash::Equal StringHash.patch text/plain 2150 dave+webkit ZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9TdHJpbmdIYXNoLmggYi9XZWJDb3Jl L3BsYXRmb3JtL3RleHQvU3RyaW5nSGFzaC5oCmluZGV4IDMzNmRjZTMuLjVkMDFlYTggMTAwNjQ0 Ci0tLSBhL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9TdHJpbmdIYXNoLmgKKysrIGIvV2ViQ29yZS9w bGF0Zm9ybS90ZXh0L1N0cmluZ0hhc2guaApAQCAtMSw1ICsxLDYgQEAKIC8qCiAgKiBDb3B5cmln aHQgKEMpIDIwMDYsIDIwMDcsIDIwMDggQXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkCisg KiBDb3B5cmlnaHQgKEMpIFJlc2VhcmNoIEluIE1vdGlvbiBMaW1pdGVkIDIwMDkuIEFsbCByaWdo dHMgcmVzZXJ2ZWQuCiAgKgogICogVGhpcyBsaWJyYXJ5IGlzIGZyZWUgc29mdHdhcmU7IHlvdSBj YW4gcmVkaXN0cmlidXRlIGl0IGFuZC9vcgogICogbW9kaWZ5IGl0IHVuZGVyIHRoZSB0ZXJtcyBv ZiB0aGUgR05VIExpYnJhcnkgR2VuZXJhbCBQdWJsaWMKQEAgLTQ3LDYgKzQ4LDE2IEBAIG5hbWVz cGFjZSBXZWJDb3JlIHsKICAgICAgICAgICAgIGlmIChhTGVuZ3RoICE9IGJMZW5ndGgpCiAgICAg ICAgICAgICAgICAgcmV0dXJuIGZhbHNlOwogCisjaWYgUExBVEZPUk0oQVJNKSB8fCBQTEFURk9S TShTSDQpCisgICAgICAgICAgICBjb25zdCBVQ2hhciogYUNoYXJzID0gYS0+Y2hhcmFjdGVycygp OworICAgICAgICAgICAgY29uc3QgVUNoYXIqIGJDaGFycyA9IGItPmNoYXJhY3RlcnMoKTsKKyAg ICAgICAgICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgIT0gYUxlbmd0aDsgKytpKSB7CisgICAg ICAgICAgICAgICAgaWYgKCphQ2hhcnMrKyAhPSAqYkNoYXJzKyspCisgICAgICAgICAgICAgICAg ICAgIHJldHVybiBmYWxzZTsKKyAgICAgICAgICAgIH0KKyAgICAgICAgICAgIHJldHVybiB0cnVl OworI2Vsc2UKKyAgICAgICAgICAgIC8qIERvIGl0IDQtYnl0ZXMtYXQtYS10aW1lIG9uIGFyY2hp dGVjdHVyZXMgd2hlcmUgaXQncyBzYWZlICovCiAgICAgICAgICAgICBjb25zdCB1aW50MzJfdCog YUNoYXJzID0gcmVpbnRlcnByZXRfY2FzdDxjb25zdCB1aW50MzJfdCo+KGEtPmNoYXJhY3RlcnMo KSk7CiAgICAgICAgICAgICBjb25zdCB1aW50MzJfdCogYkNoYXJzID0gcmVpbnRlcnByZXRfY2Fz dDxjb25zdCB1aW50MzJfdCo+KGItPmNoYXJhY3RlcnMoKSk7CiAKQEAgLTU5LDYgKzcwLDcgQEAg bmFtZXNwYWNlIFdlYkNvcmUgewogICAgICAgICAgICAgICAgIHJldHVybiBmYWxzZTsKIAogICAg ICAgICAgICAgcmV0dXJuIHRydWU7CisjZW5kaWYKICAgICAgICAgfQogCiAgICAgICAgIHN0YXRp YyB1bnNpZ25lZCBoYXNoKGNvbnN0IFJlZlB0cjxTdHJpbmdJbXBsPiYga2V5KSB7IHJldHVybiBr ZXktPmhhc2goKTsgfQpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3ZDNmYmQ2Li5iOWE1NDhiIDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5n ZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDA5LTExLTEz ICBEYXZlIFRhcHVza2EgIDxkdGFwdXNrYUByaW0uY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorICAgICAgICAKKyAgICAgICAgQ29tcGFyZSBVQ2hhcnMgc2luZ2xl IHVuaXQgYXQgYSB0aW1lIGFzIG9wcG9zZWQgdG8gdGhlIHVpbnQzMl90CisgICAgICAgIGFwcHJv YWNoIGFzIGNhc3RpbmcgdG8gdW5hbGlnbmVkIGFkZHJlc3NlcyBtYXkgY2F1c2UgYSBidXMgZmFp bHVyZQorICAgICAgICBvbiBBUk12NSBhbmQgYmVsb3cuIFRoaXMgY2hhbmdlIHJlcGxpY2F0ZXMg dGhlIHNhbWUgZGVmaW5lcyB0aGF0CisgICAgICAgIGV4aXN0cyBpbiBBdG9taWNTdHJpbmcuY3Bw CisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMxNDc1 CisgICAgICAgIAorICAgICAgICAqIHBsYXRmb3JtL3RleHQvU3RyaW5nSGFzaC5oOgorICAgICAg ICAoV2ViQ29yZTo6U3RyaW5nSGFzaDo6ZXF1YWwpOgorCiAyMDA5LTEwLTI4ICBHZW9yZ2UgU3Rh aWtvcyAgPGdlb3JnZS5zdGFpa29zQHRvcmNobW9iaWxlLmNvbT4KIAogICAgICAgICBBdHRlbXB0 IHRvIGZpeCB0aGUgTWFjIGRlYnVnIGJ1aWxkIGFmdGVyIDUwMjI1Lgo=