Modified: trunk/Source/WebKit/ChangeLog (271771 => 271772)
--- trunk/Source/WebKit/ChangeLog 2021-01-23 01:28:11 UTC (rev 271771)
+++ trunk/Source/WebKit/ChangeLog 2021-01-23 02:13:17 UTC (rev 271772)
@@ -1,3 +1,47 @@
+2021-01-22 Wenson Hsieh <[email protected]>
+
+ The web process should be killed after failing to decode display list items in the GPU process
+ https://bugs.webkit.org/show_bug.cgi?id=219097
+ <rdar://problem/71546526>
+
+ Reviewed by Chris Dumez.
+
+ Handle `StopReplayReason::InvalidItem` by terminating the web process via MESSAGE_CHECK. See below for more
+ details.
+
+ * GPUProcess/GPUConnectionToWebProcess.cpp:
+ (WebKit::GPUConnectionToWebProcess::didReceiveInvalidMessage):
+ (WebKit::GPUConnectionToWebProcess::terminateWebProcess):
+ * GPUProcess/GPUConnectionToWebProcess.h:
+
+ Pull logic for terminating the web process out into a separate helper method on `GPUConnectionToWebProcess`, and
+ use this helper in `GPUConnectionToWebProcess::didReceiveInvalidMessage`, as well as in `RemoteRenderingBackend`
+ below.
+
+ * GPUProcess/graphics/RemoteRenderingBackend.cpp:
+
+ Add macro definitions for `MESSAGE_CHECK` and `MESSAGE_CHECK_WITH_RETURN_VALUE`. Since the methods on
+ `RemoteRenderingBackend` that trigger message checks all run on a background queue, the normal (main-thread) way
+ of defining these macros (`MESSAGE_CHECK_WITH_RETURN_VALUE_BASE` and `MESSAGE_CHECK_BASE`) don't work. We
+ instead call into the GPU to web process connection object directly to send a `TerminateWebProcess` message to
+ the parent (UI) process, and additionally log a given failure message.
+
+ (WebKit::RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists):
+
+ Replace a number FIXMEs throughout these IPC message handlers with `MESSAGE_CHECK`s. Additionally, add a new
+ `MESSAGE_CHECK` for the case where we stopped replay early due to a corrupted or invalid display list item.
+
+ (WebKit::RemoteRenderingBackend::wakeUpAndApplyDisplayList):
+ (WebKit::RemoteRenderingBackend::setNextItemBufferToRead):
+
+ Simply remove the FIXME and early return here; this can only happen in the case where a compromised web content
+ process attempts to append redundant item buffer change items. However, doing so will either be (1) harmless,
+ since the pending item buffer information will just be overwritten, or (2) result in hitting a MESSAGE_CHECK
+ when decoding items if we try to execute the contents of another item buffer that has not been written to.
+
+ (WebKit::RemoteRenderingBackend::didCreateSharedDisplayListHandle):
+ * GPUProcess/graphics/RemoteRenderingBackend.h:
+
2021-01-22 Simon Fraser <[email protected]>
[iOS WK2] Make the "in stable state" bit in visible content rect updates more fine-grained
Modified: trunk/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp (271771 => 271772)
--- trunk/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp 2021-01-23 01:28:11 UTC (rev 271771)
+++ trunk/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp 2021-01-23 02:13:17 UTC (rev 271772)
@@ -224,6 +224,11 @@
void GPUConnectionToWebProcess::didReceiveInvalidMessage(IPC::Connection& connection, IPC::MessageName messageName)
{
RELEASE_LOG_FAULT(IPC, "Received an invalid message '%" PUBLIC_LOG_STRING "' from WebContent process %" PRIu64 ", requesting for it to be terminated.", description(messageName), m_webProcessIdentifier.toUInt64());
+ terminateWebProcess();
+}
+
+void GPUConnectionToWebProcess::terminateWebProcess()
+{
gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_webProcessIdentifier), 0);
}
Modified: trunk/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h (271771 => 271772)
--- trunk/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h 2021-01-23 01:28:11 UTC (rev 271771)
+++ trunk/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h 2021-01-23 02:13:17 UTC (rev 271772)
@@ -112,6 +112,8 @@
RemoteAudioSessionProxyManager& audioSessionManager();
#endif
+ void terminateWebProcess();
+
private:
GPUConnectionToWebProcess(GPUProcess&, WebCore::ProcessIdentifier, IPC::Connection::Identifier, PAL::SessionID);
Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp (271771 => 271772)
--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp 2021-01-23 01:28:11 UTC (rev 271771)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp 2021-01-23 02:13:17 UTC (rev 271772)
@@ -44,6 +44,24 @@
#include <wtf/cocoa/MachSemaphore.h>
#endif
+#define TERMINATE_WEB_PROCESS_WITH_MESSAGE(message) \
+ RELEASE_LOG_FAULT(IPC, "Requesting termination of web process %" PRIu64 " for reason: %" PUBLIC_LOG_STRING, m_gpuConnectionToWebProcess->webProcessIdentifier().toUInt64(), #message); \
+ m_gpuConnectionToWebProcess->terminateWebProcess();
+
+#define MESSAGE_CHECK(assertion, message) do { \
+ if (UNLIKELY(!(assertion))) { \
+ TERMINATE_WEB_PROCESS_WITH_MESSAGE(message); \
+ return; \
+ } \
+} while (0)
+
+#define MESSAGE_CHECK_WITH_RETURN_VALUE(assertion, returnValue, message) do { \
+ if (UNLIKELY(!(assertion))) { \
+ TERMINATE_WEB_PROCESS_WITH_MESSAGE(message); \
+ return (returnValue); \
+ } \
+} while (0)
+
namespace WebKit {
using namespace WebCore;
@@ -173,31 +191,19 @@
while (destination) {
auto displayList = handle.displayListForReading(offset, sizeToRead, *this);
- if (UNLIKELY(!displayList)) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- break;
- }
+ MESSAGE_CHECK_WITH_RETURN_VALUE(displayList, nullptr, "Failed to map display list from shared memory");
auto result = submit(*displayList, *destination);
sizeToRead = handle.advance(result.numberOfBytesRead);
+ MESSAGE_CHECK_WITH_RETURN_VALUE(result.reasonForStopping != DisplayList::StopReplayReason::InvalidItem, nullptr, "Detected invalid display list item");
CheckedSize checkedOffset = offset;
checkedOffset += result.numberOfBytesRead;
- if (UNLIKELY(checkedOffset.hasOverflowed())) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- break;
- }
+ MESSAGE_CHECK_WITH_RETURN_VALUE(!checkedOffset.hasOverflowed(), nullptr, "Overflowed when advancing shared display list handle offset");
offset = checkedOffset.unsafeGet();
+ MESSAGE_CHECK_WITH_RETURN_VALUE(offset <= handle.sharedMemory().size(), nullptr, "Out-of-bounds offset into shared display list handle");
- if (UNLIKELY(offset > handle.sharedMemory().size())) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- break;
- }
-
if (result.reasonForStopping == DisplayList::StopReplayReason::ChangeDestinationImageBuffer) {
destination = makeRefPtr(m_remoteResourceCache.cachedImageBuffer(*result.nextDestinationImageBuffer));
if (!destination) {
@@ -232,27 +238,14 @@
break;
sizeToRead = handle.unreadBytes();
- if (UNLIKELY(!sizeToRead)) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- break;
- }
+ MESSAGE_CHECK_WITH_RETURN_VALUE(sizeToRead, nullptr, "No unread bytes when resuming display list processing");
auto newDestinationIdentifier = makeObjectIdentifier<RenderingResourceIdentifierType>(resumeReadingInfo->destination);
- if (UNLIKELY(!newDestinationIdentifier)) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- break;
- }
+ MESSAGE_CHECK_WITH_RETURN_VALUE(newDestinationIdentifier, nullptr, "Invalid image buffer destination when resuming display list processing");
destination = makeRefPtr(m_remoteResourceCache.cachedImageBuffer(newDestinationIdentifier));
+ MESSAGE_CHECK_WITH_RETURN_VALUE(destination, nullptr, "Missing image buffer destination when resuming display list processing");
- if (UNLIKELY(!destination)) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- break;
- }
-
offset = resumeReadingInfo->offset;
if (!destination) {
@@ -272,24 +265,14 @@
TraceScope tracingScope(WakeUpAndApplyDisplayListStart, WakeUpAndApplyDisplayListEnd);
auto destinationImageBuffer = makeRefPtr(m_remoteResourceCache.cachedImageBuffer(arguments.destinationImageBufferIdentifier));
- if (UNLIKELY(!destinationImageBuffer)) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- return;
- }
+ MESSAGE_CHECK(destinationImageBuffer, "Missing destination image buffer");
auto initialHandle = m_sharedDisplayListHandles.get(arguments.itemBufferIdentifier);
- if (UNLIKELY(!initialHandle)) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- return;
- }
+ MESSAGE_CHECK(initialHandle, "Missing initial shared display list handle");
destinationImageBuffer = nextDestinationImageBufferAfterApplyingDisplayLists(*destinationImageBuffer, arguments.offset, *initialHandle, arguments.reason);
- if (!destinationImageBuffer) {
- RELEASE_ASSERT(m_pendingWakeupInfo);
+ if (!destinationImageBuffer)
return;
- }
while (m_pendingWakeupInfo) {
if (m_pendingWakeupInfo->missingCachedResourceIdentifier)
@@ -305,20 +288,13 @@
// Otherwise, continue reading the next display list item buffer from the start.
auto arguments = std::exchange(m_pendingWakeupInfo, WTF::nullopt)->arguments;
destinationImageBuffer = nextDestinationImageBufferAfterApplyingDisplayLists(*destinationImageBuffer, arguments.offset, *nextHandle, arguments.reason);
- if (!destinationImageBuffer) {
- RELEASE_ASSERT(m_pendingWakeupInfo);
+ if (!destinationImageBuffer)
break;
- }
}
}
void RemoteRenderingBackend::setNextItemBufferToRead(DisplayList::ItemBufferIdentifier identifier, WebCore::RenderingResourceIdentifier destinationIdentifier)
{
- if (UNLIKELY(m_pendingWakeupInfo)) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- return;
- }
m_pendingWakeupInfo = {{{ identifier, SharedDisplayListHandle::headerSize(), destinationIdentifier, GPUProcessWakeupReason::Unspecified }, WTF::nullopt }};
}
@@ -405,13 +381,8 @@
void RemoteRenderingBackend::didCreateSharedDisplayListHandle(DisplayList::ItemBufferIdentifier identifier, const SharedMemory::IPCHandle& handle, RenderingResourceIdentifier destinationBufferIdentifier)
{
ASSERT(!RunLoop::isMain());
+ MESSAGE_CHECK(!m_sharedDisplayListHandles.contains(identifier), "Duplicate shared display list handle");
- if (UNLIKELY(m_sharedDisplayListHandles.contains(identifier))) {
- // FIXME: Add a message check to terminate the web process.
- ASSERT_NOT_REACHED();
- return;
- }
-
if (auto sharedMemory = SharedMemory::map(handle.handle, SharedMemory::Protection::ReadWrite))
m_sharedDisplayListHandles.set(identifier, DisplayListReaderHandle::create(identifier, sharedMemory.releaseNonNull()));
