Summary: | WTF::StringImpl is not thread-safe | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Lord <clord> | ||||||||
Component: | WebKit Misc. | Assignee: | Chris Lord <clord> | ||||||||
Status: | NEW --- | ||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, koivisto, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Chris Lord
2020-11-25 09:06:27 PST
Using a very simple ref/deref benchmark on StringImpl, changing m_refCount to std::atomic<unsigned> makes it run at about 35% of current speed on a release build on this Xeon machine. Making refs use relaxed memory ordering makes that 40%, making them both relaxed to gauge what a GC thread might give us in terms of ref/deref perf increase is about the same (which surprised me a bit...) Switching to an atomic ref count has a significant hit, the question will be how significant is it on real-world use-cases, once code has been refactored to take into account that Strings can be shared across threads now... Created attachment 415120 [details]
Patch
Created attachment 415130 [details]
Patch
Created attachment 415133 [details]
Patch
The attached patch is a first run at (1) and (2). speedtest goes from a 64.9 to a 60 on my machine (an 8% hit, which is above the error threshold) and Jetstream 1.1 is basically the same, I couldn't get Jetstream 2.0 to run (it hangs on bomb-workers). I haven't done any profiling yet to see where the hits are, but I think it's worth doing a first run at (3) before that. Comment on attachment 415133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415133&action=review > Source/WTF/wtf/text/StringImpl.h:1124 > + if (!(refCount - s_refCountIncrement)) { > + StringImpl::destroy(this); > + return; > + } I think this is not safe. Let's consider the following scenario. 1. There are two threads, A and B. 2. A thread deref-ed an atomic string "TEST", and about to calling StringImpl::destroy 3. But at (2), still the atomic string "TEST" is in AtomStringTable 4. B thread attempts to get "TEST" atomic string from the table, and B thread found (2)'s atomic string. Ref it and start using it. 5. A thread starts calling StringImpl::destroy and (2)'s string gets destroyed 6. Now, B thread is having destroyed StringImpl, UAF. I think blink's StringImpl is not thread-safe too. https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/wtf/text/string_impl.h;l=267?q=StringImpl&ss=chromium |