Attachment 373122 Details for Bug 199298 – Patch

[patch] Patch

bug-199298-20190628092738.patch (text/plain), 9.96 KB, created by youenn fablet on 2019-06-28 09:27:39 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-06-28 09:27:39 PDT

Size: 9.96 KB

patch

obsolete

>Subversion Revision: 246881
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 7ca0b72822fdec42bedd3e3b6e478166f50f5386..03bf19c9e188eaf269c14d9bc2e52de7c0ac4e72 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,39 @@
>+2019-06-27  Youenn Fablet  <youenn@apple.com>
>+
>+        Protect NetworkProcess::m_swServers from bad session IDs
>+        https://bugs.webkit.org/show_bug.cgi?id=199298
>+        <rdar://problem/51859081>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Protect NetworkProcess from receiving bad session IDs in service worker code path.
>+        If a WebSWClientConnection ever gets a bad session ID, this connection will not send any IPC to the network process.
>+
>+        Made some refactoring to remove swOriginStoreForSession method.
>+        In the one call site where it is used, the store should already be created.
>+
>+        In get/clear data methods, add a check that the path be not empty.
>+        This reduces the number of call to swServerForSession.
>+
>+        Added a bunch of additional ASSERTs.
>+
>+        * NetworkProcess/NetworkProcess.cpp:
>+        (WebKit::NetworkProcess::destroySession):
>+        (WebKit::NetworkProcess::deleteWebsiteData):
>+        (WebKit::NetworkProcess::deleteWebsiteDataForOrigins):
>+        (WebKit::NetworkProcess::actualPrepareToSuspend):
>+        (WebKit::NetworkProcess::swServerForSession):
>+        (WebKit::NetworkProcess::registerSWServerConnection):
>+        (WebKit::NetworkProcess::unregisterSWServerConnection):
>+        (WebKit::NetworkProcess::swOriginStoreForSession): Deleted.
>+        * NetworkProcess/NetworkProcess.h:
>+        * WebProcess/Network/NetworkProcessConnection.cpp:
>+        (WebKit::NetworkProcessConnection::initializeSWClientConnection):
>+        * WebProcess/Storage/WebSWClientConnection.cpp:
>+        (WebKit::WebSWClientConnection::WebSWClientConnection):
>+        (WebKit::WebSWClientConnection::initializeConnectionIfNeeded):
>+        (WebKit::WebSWClientConnection::ensureConnectionAndSend):
>+
> 2019-06-27  Youenn Fablet  <youenn@apple.com>
> 
>         Fix build after revision 246877
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index c3bca8127681ad185adc99e0d12198098abc4792..3ae2c9ea9dc42eae36bd23ccd07c147afe0e2e2e 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -577,6 +577,10 @@ void NetworkProcess::setSession(const PAL::SessionID& sessionID, Ref<NetworkSess
> 
> void NetworkProcess::destroySession(const PAL::SessionID& sessionID)
> {
>+    ASSERT(sessionID.isValid());
>+    if (!sessionID.isValid())
>+        return;
>+
>     ASSERT(sessionID != PAL::SessionID::defaultSessionID());
> 
>     if (auto session = m_networkSessions.take(sessionID))
>@@ -1250,6 +1254,10 @@ static void fetchDiskCacheEntries(NetworkCache::Cache* cache, PAL::SessionID ses
> 
> void NetworkProcess::fetchWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, uint64_t callbackID)
> {
>+    ASSERT(sessionID.isValid());
>+    if (!sessionID.isValid())
>+        return;
>+
>     struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {
>         explicit CallbackAggregator(Function<void (WebsiteData)>&& completionHandler)
>             : m_completionHandler(WTFMove(completionHandler))
>@@ -1344,6 +1352,10 @@ void NetworkProcess::fetchWebsiteData(PAL::SessionID sessionID, OptionSet<Websit
> 
> void NetworkProcess::deleteWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, WallTime modifiedSince, uint64_t callbackID)
> {
>+    ASSERT(sessionID.isValid());
>+    if (!sessionID.isValid())
>+        return;
>+
> #if PLATFORM(COCOA)
>     if (websiteDataTypes.contains(WebsiteDataType::HSTSCache)) {
>         if (auto* networkStorageSession = storageSession(sessionID))
>@@ -1551,6 +1563,10 @@ static Vector<WebCore::SecurityOriginData> filterForRegistrableDomains(const Has
> 
> void NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, HashMap<RegistrableDomain, WebsiteDataToRemove>&& domains, bool shouldNotifyPage, CompletionHandler<void(const HashSet<RegistrableDomain>&)>&& completionHandler)
> {
>+    ASSERT(sessionID.isValid());
>+    if (!sessionID.isValid())
>+        return;
>+
>     OptionSet<WebsiteDataFetchOption> fetchOptions = WebsiteDataFetchOption::DoNotCreateProcesses;
> 
>     struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {
>@@ -1749,6 +1765,10 @@ void NetworkProcess::deleteCookiesForTesting(PAL::SessionID sessionID, Registrab
> 
> void NetworkProcess::registrableDomainsWithWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, bool shouldNotifyPage, CompletionHandler<void(HashSet<RegistrableDomain>&&)>&& completionHandler)
> {
>+    ASSERT(sessionID.isValid());
>+    if (!sessionID.isValid())
>+        return;
>+
>     OptionSet<WebsiteDataFetchOption> fetchOptions = WebsiteDataFetchOption::DoNotCreateProcesses;
>     
>     struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {
>@@ -2035,8 +2055,10 @@ void NetworkProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend
>         connection->cleanupForSuspension([delayedTaskCounter] { });
> 
> #if ENABLE(SERVICE_WORKER)
>-    for (auto& server : m_swServers.values())
>+    for (auto& server : m_swServers.values()) {
>+        ASSERT(m_swServers.get(server->sessionID()) == server.get());
>         server->startSuspension([delayedTaskCounter] { });
>+    }
> #endif
> 
>     for (auto& session : m_networkSessions)
>@@ -2367,7 +2389,7 @@ bool NetworkProcess::needsServerToContextConnectionForRegistrableDomain(const Re
> SWServer& NetworkProcess::swServerForSession(PAL::SessionID sessionID)
> {
>     ASSERT(sessionID.isValid());
>-    
>+
>     auto result = m_swServers.ensure(sessionID, [&] {
>         auto path = m_swDatabasePaths.get(sessionID);
>         // There should already be a registered path for this PAL::SessionID.
>@@ -2383,13 +2405,12 @@ SWServer& NetworkProcess::swServerForSession(PAL::SessionID sessionID)
>     return *result.iterator->value;
> }
> 
>-WebSWOriginStore& NetworkProcess::swOriginStoreForSession(PAL::SessionID sessionID)
>-{
>-    return static_cast<WebSWOriginStore&>(swServerForSession(sessionID).originStore());
>-}
>-
> WebSWOriginStore* NetworkProcess::existingSWOriginStoreForSession(PAL::SessionID sessionID) const
> {
>+    ASSERT(sessionID.isValid());
>+    if (!sessionID.isValid())
>+        return nullptr;
>+
>     auto* swServer = m_swServers.get(sessionID);
>     if (!swServer)
>         return nullptr;
>@@ -2430,7 +2451,10 @@ void NetworkProcess::registerSWServerConnection(WebSWServerConnection& connectio
>     ASSERT(parentProcessHasServiceWorkerEntitlement());
>     ASSERT(!m_swServerConnections.contains(connection.identifier()));
>     m_swServerConnections.add(connection.identifier(), &connection);
>-    swOriginStoreForSession(connection.sessionID()).registerSWServerConnection(connection);
>+    auto* store = existingSWOriginStoreForSession(connection.sessionID());
>+    ASSERT(store);
>+    if (store)
>+        store->registerSWServerConnection(connection);
> }
> 
> void NetworkProcess::unregisterSWServerConnection(WebSWServerConnection& connection)
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.h b/Source/WebKit/NetworkProcess/NetworkProcess.h
>index 0e1dabd00ee4ca94e22b2d25e0c32fbbc4054fdd..4f09984fe4bff176f99fc7d0169380244af9c7d7 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.h
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.h
>@@ -463,7 +463,6 @@ private:
>     
>     void disableServiceWorkerProcessTerminationDelay();
>     
>-    WebSWOriginStore& swOriginStoreForSession(PAL::SessionID);
>     WebSWOriginStore* existingSWOriginStoreForSession(PAL::SessionID) const;
>     bool needsServerToContextConnectionForRegistrableDomain(const WebCore::RegistrableDomain&) const;
> 
>diff --git a/Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp b/Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp
>index 90b4b54d1676595ec2d390165f59676cf4e69fd5..d9adcc21a8a96b2aa545366de095d047c6257edf 100644
>--- a/Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp
>+++ b/Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp
>@@ -279,6 +279,7 @@ void NetworkProcessConnection::removeSWClientConnection(WebSWClientConnection& c
> 
> SWServerConnectionIdentifier NetworkProcessConnection::initializeSWClientConnection(WebSWClientConnection& connection)
> {
>+    ASSERT(connection.sessionID().isValid());
>     SWServerConnectionIdentifier identifier;
>     bool result = m_connection->sendSync(Messages::NetworkConnectionToWebProcess::EstablishSWServerConnection(connection.sessionID()), Messages::NetworkConnectionToWebProcess::EstablishSWServerConnection::Reply(identifier), 0);
>     ASSERT_UNUSED(result, result);
>diff --git a/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp b/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp
>index 579054505bbb6626942954408b11b21b04ac5053..2b5ed70d677ba6f58ffd97813f55ee9651b814bb 100644
>--- a/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp
>+++ b/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp
>@@ -56,7 +56,6 @@ WebSWClientConnection::WebSWClientConnection(SessionID sessionID)
>     : m_sessionID(sessionID)
>     , m_swOriginTable(makeUniqueRef<WebSWOriginTable>())
> {
>-    ASSERT(sessionID.isValid());
>     initializeConnectionIfNeeded();
> }
> 
>@@ -68,6 +67,10 @@ WebSWClientConnection::~WebSWClientConnection()
> 
> void WebSWClientConnection::initializeConnectionIfNeeded()
> {
>+    ASSERT(m_sessionID.isValid());
>+    if (!m_sessionID.isValid())
>+        return;
>+
>     if (m_connection)
>         return;
> 
>@@ -83,7 +86,8 @@ template<typename U>
> void WebSWClientConnection::ensureConnectionAndSend(const U& message)
> {
>     initializeConnectionIfNeeded();
>-    send(message);
>+    if (m_connection)
>+        send(message);
> }
> 
> void WebSWClientConnection::scheduleJobInServer(const ServiceWorkerJobData& jobData)

Actions: View | Formatted Diff | Diff

Attachments on bug 199298: 373075 | 373122 | 373148