Bug 225781 – [GPU Process] Ensure RemoteRenderingBackendProxy is restored to its original state when GPUP connection is closed

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

RemoteRenderingBackendProxy::gpuProcessConnectionDidClose() is supposed to do that. But some members of RemoteRenderingBackendProxy are not cleared in this function.

[Said Abou-Hallawa]

<rdar://76678163>

[Said Abou-Hallawa]

Created attachment 428561 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 428561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428561&action=review

> Source/WebKit/ChangeLog:27
> +        No need for m_getPixelBufferSharedMemoryLength. The share memory length
> +        can be retrieved from SharedMemory::size();

Are you sure you're not undoing a security hardening here?

[Said Abou-Hallawa]

Comment on attachment 428561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428561&action=review

>> Source/WebKit/ChangeLog:27
>> +        can be retrieved from SharedMemory::size();
> 
> Are you sure you're not undoing a security hardening here?

Can you please explain your point a little? I did not get it especially we have many data sizes in this context

IPCHandle::dataSize
IPCHandle::handle::m_size
SharedMemory::m_size // which is copied form the IPCHandle::handle::m_size.

What is the security hardening in adding a fourth one?

[Simon Fraser (smfr)]

Comment on attachment 428561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428561&action=review

>>> Source/WebKit/ChangeLog:27
>>> +        can be retrieved from SharedMemory::size();
>> 
>> Are you sure you're not undoing a security hardening here?
> 
> Can you please explain your point a little? I did not get it especially we have many data sizes in this context
> 
> IPCHandle::dataSize
> IPCHandle::handle::m_size
> SharedMemory::m_size // which is copied form the IPCHandle::handle::m_size.
> 
> What is the security hardening in adding a fourth one?

There's a lot of discussion in https://bugs.webkit.org/show_bug.cgi?id=223732 which added this code. It seems that what Myles checked in was different from the last patch on the bug. He should explain why the code is as it is.

[Ryosuke Niwa]

Comment on attachment 428561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428561&action=review

>>>> Source/WebKit/ChangeLog:27
>>>> +        can be retrieved from SharedMemory::size();
>>> 
>>> Are you sure you're not undoing a security hardening here?
>> 
>> Can you please explain your point a little? I did not get it especially we have many data sizes in this context
>> 
>> IPCHandle::dataSize
>> IPCHandle::handle::m_size
>> SharedMemory::m_size // which is copied form the IPCHandle::handle::m_size.
>> 
>> What is the security hardening in adding a fourth one?
> 
> There's a lot of discussion in https://bugs.webkit.org/show_bug.cgi?id=223732 which added this code. It seems that what Myles checked in was different from the last patch on the bug. He should explain why the code is as it is.

This code runs in WebContent process. There is no need to harden what gets sent back from GPU process.
In general, we trust stuff sent or replied from GPU process.
Also, SharedMemory::size is always correct. mach_vm_map ensures that it is
IPCHandle::dataSize could be arbitrary value, however, until SharedMemory::map is called since that's where we validate.

[Said Abou-Hallawa]

Created attachment 428678 [details]
Patch

[EWS]

Committed r277562 (237790@main): <https://commits.webkit.org/237790@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428678 [details].