WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
31475
Crash in StringHash::equal due to unaligned string data
https://bugs.webkit.org/show_bug.cgi?id=31475
Summary
Crash in StringHash::equal due to unaligned string data
Yong Li
Reported
2009-11-13 09:02:44 PST
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?
Attachments
Apply defines to StringHash::Equal
(2.10 KB, patch)
2009-11-13 14:25 PST
,
David Tapuska
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
David Levin
Comment 1
2009-11-13 12:20:48 PST
> 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.
Yong Li
Comment 2
2009-11-13 12:27:49 PST
(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.
Darin Adler
Comment 3
2009-11-13 12:54:36 PST
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.
Joe Mason
Comment 4
2009-11-13 12:58:56 PST
This thread discusses the issue, but it seems their conclusion was never adopted:
http://lists.macosforge.org/pipermail/webkit-dev/2009-July/008630.html
Yong Li
Comment 5
2009-11-13 13:13:57 PST
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?
David Tapuska
Comment 6
2009-11-13 14:25:31 PST
Created
attachment 43204
[details]
Apply defines to StringHash::Equal
WebKit Commit Bot
Comment 7
2009-11-13 17:24:13 PST
Comment on
attachment 43204
[details]
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
Adam Barth
Comment 8
2009-11-15 15:34:17 PST
Comment on
attachment 43204
[details]
Apply defines to StringHash::Equal Clearing flags on attachment: 43204 Committed
r51006
: <
http://trac.webkit.org/changeset/51006
>
Adam Barth
Comment 9
2009-11-15 15:34:24 PST
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 10
2009-11-20 16:07:43 PST
***
Bug 31726
has been marked as a duplicate of this bug. ***
Simon Hausmann
Comment 11
2010-04-06 07:46:57 PDT
Cherry-picked into QtWebKit for Qt 4.6 with commit e3dc4ef2b801d91e115c54f833fa7766d392ceda
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug