[webkit-reviews] review requested: [Bug 204355] Make sendWithAsyncReply() safe to call from any thread : [Attachment 383873] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 19 08:50:38 PST 2019


Chris Dumez <cdumez at apple.com> has asked  for review:
Bug 204355: Make sendWithAsyncReply() safe to call from any thread
https://bugs.webkit.org/show_bug.cgi?id=204355

Attachment 383873: Patch

https://bugs.webkit.org/attachment.cgi?id=383873&action=review




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 383873
  --> https://bugs.webkit.org/attachment.cgi?id=383873
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::Proces
sWasResumed(), [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...


More information about the webkit-reviews mailing list