Changeset 250799 in webkit

Timestamp:

Oct 7, 2019 3:14:41 PM (5 years ago)

Author:

pvollan@apple.com

Message:

[macOS] Layering violation in AuxiliaryProcessProxy::didFinishLaunching
​https://bugs.webkit.org/show_bug.cgi?id=201617

Reviewed by Brent Fulgham.

The commit <​https://trac.webkit.org/changeset/249649> introduced a layering violation in AuxiliaryProcessProxy::didFinishLaunching
where we inspect the pending message queue looking for a local file load message which needs the PID to create a sandbox extension
for the WebContent process. The layering violation can be fixed by creating a virtual method in AuxiliaryProcessProxy and override
the method in the WebProcessProxy to do the work needed to replace the message with a load request message containing a sandbox
extension created using the PID of the WebContent process. No new tests have been created, since this is covered by existing tests.

• UIProcess/AuxiliaryProcessProxy.cpp:

(WebKit::AuxiliaryProcessProxy::didFinishLaunching):

• UIProcess/AuxiliaryProcessProxy.h:

(WebKit::AuxiliaryProcessProxy::shouldSendPendingMessage):

• UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::shouldSendPendingMessage):

• UIProcess/WebProcessProxy.h:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/AuxiliaryProcessProxy.cpp (modified) (2 diffs)
• UIProcess/AuxiliaryProcessProxy.h (modified) (2 diffs)
• UIProcess/WebProcessProxy.cpp (modified) (3 diffs)
• UIProcess/WebProcessProxy.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-10-07  Per Arne Vollan  <pvollan@apple.com>
+
+        [macOS] Layering violation in AuxiliaryProcessProxy::didFinishLaunching
+        https://bugs.webkit.org/show_bug.cgi?id=201617
+
+        Reviewed by Brent Fulgham.
+
+        The commit <https://trac.webkit.org/changeset/249649> introduced a layering violation in AuxiliaryProcessProxy::didFinishLaunching
+        where we inspect the pending message queue looking for a local file load message which needs the PID to create a sandbox extension
+        for the WebContent process. The layering violation can be fixed by creating a virtual method in AuxiliaryProcessProxy and override
+        the method in the WebProcessProxy to do the work needed to replace the message with a load request message containing a sandbox
+        extension created using the PID of the WebContent process. No new tests have been created, since this is covered by existing tests.
+
+        * UIProcess/AuxiliaryProcessProxy.cpp:
+        (WebKit::AuxiliaryProcessProxy::didFinishLaunching):
+        * UIProcess/AuxiliaryProcessProxy.h:
+        (WebKit::AuxiliaryProcessProxy::shouldSendPendingMessage):
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::shouldSendPendingMessage):
+        * UIProcess/WebProcessProxy.h:
+
 2019-10-07  Dean Jackson  <dino@apple.com>
 

trunk/Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp

@@ -28 +28 @@
 
 #include "AuxiliaryProcessMessages.h"
-#include "LoadParameters.h"
 #include "Logging.h"
-#include "WebPageMessages.h"
 #include "WebPageProxy.h"
 #include "WebProcessProxy.h"
@@ -212 +210 @@
 
     for (auto&& pendingMessage : std::exchange(m_pendingMessages, { })) {
+        if (!shouldSendPendingMessage(pendingMessage))
+            continue;
         auto encoder = WTFMove(pendingMessage.encoder);
         auto sendOptions = pendingMessage.sendOptions;
-#if HAVE(SANDBOX_ISSUE_MACH_EXTENSION_TO_PROCESS_BY_PID)
-        if (encoder->messageName() == "LoadRequestWaitingForPID") {
-            auto buffer = encoder->buffer();
-            auto bufferSize = encoder->bufferSize();
-            std::unique_ptr<IPC::Decoder> decoder = makeUnique<IPC::Decoder>(buffer, bufferSize, nullptr, Vector<IPC::Attachment> { });
-            LoadParameters loadParameters;
-            URL resourceDirectoryURL;
-            WebPageProxyIdentifier pageID;
-            if (decoder->decode(loadParameters) && decoder->decode(resourceDirectoryURL) && decoder->decode(pageID)) {
-                if (auto* page = WebProcessProxy::webPage(pageID)) {
-                    page->maybeInitializeSandboxExtensionHandle(static_cast<WebProcessProxy&>(*this), loadParameters.request.url(), resourceDirectoryURL, loadParameters.sandboxExtensionHandle);
-                    send(Messages::WebPage::LoadRequest(loadParameters), decoder->destinationID());
-                }
-            } else
-                ASSERT_NOT_REACHED();
-            continue;
-        }
-#endif
         if (pendingMessage.asyncReplyInfo)
             IPC::addAsyncReplyHandler(*connection(), pendingMessage.asyncReplyInfo->second, WTFMove(pendingMessage.asyncReplyInfo->first));

trunk/Source/WebKit/UIProcess/AuxiliaryProcessProxy.h

@@ -123 +123 @@
     virtual void platformGetLaunchOptions(ProcessLauncher::LaunchOptions&) { };
 
-private:
-    virtual void connectionWillOpen(IPC::Connection&);
-    virtual void processWillShutDown(IPC::Connection&) = 0;
-
     struct PendingMessage {
         std::unique_ptr<IPC::Encoder> encoder;
@@ -132 +128 @@
         Optional<std::pair<CompletionHandler<void(IPC::Decoder*)>, uint64_t>> asyncReplyInfo;
     };
-    
+
+    virtual bool shouldSendPendingMessage(const PendingMessage&) { return true; }
+
+private:
+    virtual void connectionWillOpen(IPC::Connection&);
+    virtual void processWillShutDown(IPC::Connection&) = 0;
+
     Vector<PendingMessage> m_pendingMessages;
     RefPtr<ProcessLauncher> m_processLauncher;

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -32 +32 @@
 #include "DataReference.h"
 #include "DownloadProxyMap.h"
+#include "LoadParameters.h"
 #include "Logging.h"
 #include "PluginInfoStore.h"
@@ -44 +45 @@
 #include "WebNotificationManagerProxy.h"
 #include "WebPageGroup.h"
+#include "WebPageMessages.h"
 #include "WebPageProxy.h"
 #include "WebPasteboardProxy.h"
@@ -303 +305 @@
 #endif
 
+bool WebProcessProxy::shouldSendPendingMessage(const PendingMessage& message)
+{
+#if HAVE(SANDBOX_ISSUE_MACH_EXTENSION_TO_PROCESS_BY_PID)
+    if (message.encoder->messageName() == "LoadRequestWaitingForPID") {
+        auto buffer = message.encoder->buffer();
+        auto bufferSize = message.encoder->bufferSize();
+        std::unique_ptr<IPC::Decoder> decoder = makeUnique<IPC::Decoder>(buffer, bufferSize, nullptr, Vector<IPC::Attachment> { });
+        LoadParameters loadParameters;
+        URL resourceDirectoryURL;
+        WebPageProxyIdentifier pageID;
+        if (decoder->decode(loadParameters) && decoder->decode(resourceDirectoryURL) && decoder->decode(pageID)) {
+            if (auto* page = WebProcessProxy::webPage(pageID)) {
+                page->maybeInitializeSandboxExtensionHandle(static_cast<WebProcessProxy&>(*this), loadParameters.request.url(), resourceDirectoryURL, loadParameters.sandboxExtensionHandle);
+                send(Messages::WebPage::LoadRequest(loadParameters), decoder->destinationID());
+            }
+        } else
+            ASSERT_NOT_REACHED();
+        return false;
+    }
+#endif
+    return true;
+}
+
 void WebProcessProxy::connectionWillOpen(IPC::Connection& connection)
 {

trunk/Source/WebKit/UIProcess/WebProcessProxy.h

@@ -330 +330 @@
     void connectionWillOpen(IPC::Connection&) override;
     void processWillShutDown(IPC::Connection&) override;
-
+    bool shouldSendPendingMessage(const PendingMessage&) final;
+    
     // ProcessLauncher::Client
     void didFinishLaunching(ProcessLauncher*, IPC::Connection::Identifier) override;