Make sendWithAsyncReply() safe to call from any thread, similarly to send().
Created attachment 383873 [details] Patch
Comment on attachment 383873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383873&action=review > Source/WebKit/Platform/IPC/Connection.cpp:255 > + ASSERT(asyncReplyHandlerMapLock().isHeld()); JSC often does this by passing an unused LockHolder& as a parameter. > Source/WebKit/Platform/IPC/Connection.cpp:1145 > + LockHolder locker(asyncReplyHandlerMapLock()); > auto map = asyncReplyHandlerMap().take(reinterpret_cast<uintptr_t>(&connection)); > for (auto& handler : map.values()) { The lock should only be held during the taking from the map, not during the iterating and calling. This needs an additional scope. > Source/WebKit/Platform/IPC/Connection.h:204 > + // Main thread only. Add an assert? > Source/WebKit/Platform/IPC/Connection.h:211 > + // Main thread only. ditto. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:320 > + parentProcessConnection()->sendWithAsyncReply(Messages::WebProcessProxy::ProcessWasResumed(), [this] { Protect this or capture a weak pointer. This looks like a UAF waiting to happen. Even if there's something else assuring this is safe, it wouldn't hurt.
Comment on attachment 383873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383873&action=review >> Source/WebKit/Platform/IPC/Connection.cpp:255 >> + ASSERT(asyncReplyHandlerMapLock().isHeld()); > > JSC often does this by passing an unused LockHolder& as a parameter. And you'd prefer that? >> Source/WebKit/Platform/IPC/Connection.cpp:1145 >> for (auto& handler : map.values()) { > > The lock should only be held during the taking from the map, not during the iterating and calling. This needs an additional scope. I don't understand how that can be true.. You cannot iterate over a container that may be modified from another thread, this would not be safe. >> Source/WebKit/Platform/IPC/Connection.h:204 >> + // Main thread only. > > Add an assert? There are already assertions. >> Source/WebKit/Platform/IPC/Connection.h:211 >> + // Main thread only. > > ditto. Ditto. >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:320 >> + parentProcessConnection()->sendWithAsyncReply(Messages::WebProcessProxy::ProcessWasResumed(), [this] { > > Protect this or capture a weak pointer. This looks like a UAF waiting to happen. Even if there's something else assuring this is safe, it wouldn't hurt. WebProcess is a singleton...
Comment on attachment 383873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383873&action=review >>> Source/WebKit/Platform/IPC/Connection.cpp:1145 >>> for (auto& handler : map.values()) { >> >> The lock should only be held during the taking from the map, not during the iterating and calling. This needs an additional scope. > > I don't understand how that can be true.. You cannot iterate over a container that may be modified from another thread, this would not be safe. Oh since we're taking, it is safe. will fix this.
Created attachment 383875 [details] Patch
Comment on attachment 383875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383875&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:320 > + parentProcessConnection()->sendWithAsyncReply(Messages::WebProcessProxy::ProcessWasResumed(), [this] { 🤦 singleton it is.
(In reply to Alex Christensen from comment #6) > Comment on attachment 383875 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383875&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:320 > > + parentProcessConnection()->sendWithAsyncReply(Messages::WebProcessProxy::ProcessWasResumed(), [this] { > > 🤦 > singleton it is. I checked, WebProcess does not subclass CanMakeWeakPtr so nobody else had this idea yet at least :)
Comment on attachment 383875 [details] Patch Clearing flags on attachment: 383875 Committed r252636: <https://trac.webkit.org/changeset/252636>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57326449>