Bug 219097

Created attachment 418180 [details] Patch

Comment on attachment 418180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418180&action=review > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:272 > + callOnMainRunLoop([this, protectedThis = makeRef(*this), messageName = *m_currentMessageName] () mutable { Is it OK that this is asynchronous? If the GPU Process is compromised, does this leave a window of vulnerability before this gets back to the originating web content process?

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 418180 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418180&action=review > > > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:272 > > + callOnMainRunLoop([this, protectedThis = makeRef(*this), messageName = *m_currentMessageName] () mutable { > > Is it OK that this is asynchronous? If the GPU Process is compromised, does > this leave a window of vulnerability before this gets back to the > originating web content process? My understanding was that there would be a "window of vulnerability" in any case, since we need to call out to the parent process connection (i.e. UI process) from the UI process to do the killing. But also, in the case where the GPU Process is compromised, would the attacker just be able to avoid this logic altogether? (My understanding was that the GPU Process, by design, isn't supposed to be compromised; otherwise, we end up with the same security pitfalls as we have now with just the web content process).

Comment on attachment 418180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418180&action=review Overall, I am not a fan of this patch. I think it adds quite a bit of complexity and does not look nice. My proposal would be whenever we get an unexpected value from the WebProcess, call a macro that does: RELEASE_LOG_FAULT(xxx); m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_webProcessIdentifier), 0); xxx being a parameter passed to the macro that is a clear message of what was wrong. > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:407 > + SetForScope setCurrentMessageName(m_currentMessageName, IPC::MessageName::RemoteRenderingBackend_DidCreateSharedDisplayListHandle); This looks really ugly and it is very unfortunate people will have to remember to do this every time they add a new IPC.

(In reply to Chris Dumez from comment #5) > Comment on attachment 418180 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418180&action=review > > Overall, I am not a fan of this patch. I think it adds quite a bit of > complexity and does not look nice. > > My proposal would be whenever we get an unexpected value from the > WebProcess, call a macro that does: > RELEASE_LOG_FAULT(xxx); > > m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()- > >send(Messages::GPUProcessProxy::TerminateWebProcess(m_webProcessIdentifier), > 0); > > xxx being a parameter passed to the macro that is a clear message of what > was wrong. > > > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:407 > > + SetForScope setCurrentMessageName(m_currentMessageName, IPC::MessageName::RemoteRenderingBackend_DidCreateSharedDisplayListHandle); > > This looks really ugly and it is very unfortunate people will have to > remember to do this every time they add a new IPC. That's a really good point — splitting the logging out from the WP termination message would make this patch a lot less messy. I'll give this a try.

Created attachment 418197 [details] Patch

Comment on attachment 418197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418197&action=review Much nicer IMO > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:49 > + m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_gpuConnectionToWebProcess->webProcessIdentifier()), 0); We may want to add a terminateWebProcess() function to GPUConnectionToWebProcess to not have to duplicate this logic in 2 places.

Comment on attachment 418197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418197&action=review >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:49 >> + m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_gpuConnectionToWebProcess->webProcessIdentifier()), 0); > > We may want to add a terminateWebProcess() function to GPUConnectionToWebProcess to not have to duplicate this logic in 2 places. Sounds good! Pulled out the logic into a helper method: void GPUConnectionToWebProcess::terminateWebProcess() { gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_webProcessIdentifier), 0); }

Created attachment 418200 [details] For landing

Committed r271772: <https://trac.webkit.org/changeset/271772> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418200 [details].