[macOS] Layering violation in AuxiliaryProcessProxy::didFinishLaunching
authorpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Oct 2019 22:14:41 +0000 (22:14 +0000)
committerpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Oct 2019 22:14:41 +0000 (22:14 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250799 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp
Source/WebKit/UIProcess/AuxiliaryProcessProxy.h
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/UIProcess/WebProcessProxy.h

index 8a215dc..f606c4a 100644 (file)
@@ -1,3 +1,24 @@
+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>
 
         Provide options for DTTZ to happen in more situations
index eb26cb8..891d797 100644 (file)
@@ -27,9 +27,7 @@
 #include "AuxiliaryProcessProxy.h"
 
 #include "AuxiliaryProcessMessages.h"
-#include "LoadParameters.h"
 #include "Logging.h"
-#include "WebPageMessages.h"
 #include "WebPageProxy.h"
 #include "WebProcessProxy.h"
 #include <wtf/RunLoop.h>
@@ -211,26 +209,10 @@ void AuxiliaryProcessProxy::didFinishLaunching(ProcessLauncher*, IPC::Connection
     m_connection->open();
 
     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));
         m_connection->sendMessage(WTFMove(encoder), sendOptions);
index ca4aa9e..a6594b2 100644 (file)
@@ -122,16 +122,18 @@ protected:
     virtual void getLaunchOptions(ProcessLauncher::LaunchOptions&);
     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;
         OptionSet<IPC::SendOption> sendOptions;
         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;
     RefPtr<IPC::Connection> m_connection;
index ffafa7e..749525a 100644 (file)
@@ -31,6 +31,7 @@
 #include "APIPageHandle.h"
 #include "DataReference.h"
 #include "DownloadProxyMap.h"
+#include "LoadParameters.h"
 #include "Logging.h"
 #include "PluginInfoStore.h"
 #include "PluginProcessManager.h"
@@ -43,6 +44,7 @@
 #include "WebNavigationDataStore.h"
 #include "WebNotificationManagerProxy.h"
 #include "WebPageGroup.h"
+#include "WebPageMessages.h"
 #include "WebPageProxy.h"
 #include "WebPasteboardProxy.h"
 #include "WebProcessCache.h"
@@ -302,6 +304,29 @@ void WebProcessProxy::platformGetLaunchOptions(ProcessLauncher::LaunchOptions& l
 }
 #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)
 {
     ASSERT(this->connection() == &connection);
index a73dc89..5b05218 100644 (file)
@@ -329,7 +329,8 @@ protected:
     void platformGetLaunchOptions(ProcessLauncher::LaunchOptions&) override;
     void connectionWillOpen(IPC::Connection&) override;
     void processWillShutDown(IPC::Connection&) override;
-
+    bool shouldSendPendingMessage(const PendingMessage&) final;
+    
     // ProcessLauncher::Client
     void didFinishLaunching(ProcessLauncher*, IPC::Connection::Identifier) override;