Crash in StringHash::equal due to unaligned string data

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yong Li]

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?

[David Levin]

> 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]

(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]

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]

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]

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]

Created attachment 43204 [details]
Apply defines to StringHash::Equal

[WebKit Commit Bot]

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 on attachment 43204 [details]
Apply defines to StringHash::Equal

Clearing flags on attachment: 43204

Committed r51006: <http://trac.webkit.org/changeset/51006>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 31726 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

Cherry-picked into QtWebKit for Qt 4.6 with commit e3dc4ef2b801d91e115c54f833fa7766d392ceda