Bug 204355 – Make sendWithAsyncReply() safe to call from any thread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Make sendWithAsyncReply() safe to call from any thread, similarly to send().

[Chris Dumez]

Created attachment 383873 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 383875 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

(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 :)

[WebKit Commit Bot]

Comment on attachment 383875 [details]
Patch

Clearing flags on attachment: 383875

Committed r252636: <https://trac.webkit.org/changeset/252636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57326449>