Attachment 373353 Details for Bug 199418 – Patch

[patch] Patch

bug-199418-20190702125302.patch (text/plain), 17.96 KB, created by Chris Dumez on 2019-07-02 12:53:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-07-02 12:53:03 PDT

Size: 17.96 KB

patch

obsolete

>Subversion Revision: 247051
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 7378c06294ebfb60810d340357a61510eb64a427..7681dc6a2af771fb750c3792d68fa178216663b1 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,51 @@
>+2019-07-02  Chris Dumez  <cdumez@apple.com>
>+
>+        Protect NetworkProcess::m_networkSessions against corruption
>+        https://bugs.webkit.org/show_bug.cgi?id=199418
>+        <rdar://problem/50614019>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        I believe the most likely reason for the crash at <rdar://problem/50614019> is that NetworkProcess::m_networkSessions
>+        is getting corrupted and is returning us a bad pointer.
>+
>+        To harden our code, I added debug assertions to make sure that this HashMap is only used on the main thread and
>+        to make sure that the sessionID used as key is always valid. I have also added if checks to avoid crashing in
>+        release whenever possible.
>+
>+        Note that we came to a similar conclusion for NetworkProcess::m_swServers when investigating rdar://problem/51859081,
>+        so the two radars are potentially related. Both HashMaps are owned by the NetworkProcess and use a SessionID
>+        as key.
>+
>+        * NetworkProcess/NetworkConnectionToWebProcess.cpp:
>+        (WebKit::NetworkConnectionToWebProcess::cookiesForDOM):
>+        (WebKit::NetworkConnectionToWebProcess::setCookiesFromDOM):
>+        (WebKit::NetworkConnectionToWebProcess::logUserInteraction):
>+        (WebKit::NetworkConnectionToWebProcess::logWebSocketLoading):
>+        (WebKit::NetworkConnectionToWebProcess::logSubresourceLoading):
>+        (WebKit::NetworkConnectionToWebProcess::logSubresourceRedirect):
>+        (WebKit::NetworkConnectionToWebProcess::hasStorageAccess):
>+        (WebKit::NetworkConnectionToWebProcess::requestStorageAccess):
>+        (WebKit::NetworkConnectionToWebProcess::requestStorageAccessUnderOpener):
>+        * NetworkProcess/NetworkProcess.cpp:
>+        (WebKit::NetworkProcess::networkSession const):
>+        (WebKit::NetworkProcess::setSession):
>+        (WebKit::NetworkProcess::destroySession):
>+        (WebKit::NetworkProcess::addKeptAliveLoad):
>+        (WebKit::NetworkProcess::removeKeptAliveLoad):
>+        (WebKit::NetworkProcess::webProcessWasDisconnected):
>+        * NetworkProcess/NetworkProcess.h:
>+        * NetworkProcess/NetworkResourceLoader.cpp:
>+        (WebKit::NetworkResourceLoader::retrieveCacheEntry):
>+        (WebKit::NetworkResourceLoader::didFinishWithRedirectResponse):
>+        (WebKit::NetworkResourceLoader::tryStoreAsCacheEntry):
>+        (WebKit::NetworkResourceLoader::shouldLogCookieInformation):
>+        * NetworkProcess/ios/NetworkConnectionToWebProcessIOS.mm:
>+        (WebKit::NetworkConnectionToWebProcess::paymentCoordinatorBoundInterfaceIdentifier):
>+        (WebKit::NetworkConnectionToWebProcess::paymentCoordinatorCTDataConnectionServiceType):
>+        (WebKit::NetworkConnectionToWebProcess::paymentCoordinatorSourceApplicationBundleIdentifier):
>+        (WebKit::NetworkConnectionToWebProcess::paymentCoordinatorSourceApplicationSecondaryIdentifier):
>+
> 2019-07-02  Chris Dumez  <cdumez@apple.com>
> 
>         Simplify logic that handles registering WebProcessProxy objects with their WebsiteDataStore
>diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>index 5992639caa2e1ba3bd12b014098955cb0de73dc9..b5bc8a3fbd27e49defbb95516befdc9bb9c5fa80 100644
>--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>@@ -531,7 +531,7 @@ void NetworkConnectionToWebProcess::cookiesForDOM(PAL::SessionID sessionID, cons
>     auto& networkStorageSession = storageSession(networkProcess(), sessionID);
>     auto result = networkStorageSession.cookiesForDOM(firstParty, sameSiteInfo, url, frameID, pageID, includeSecureCookies);
> #if ENABLE(RESOURCE_LOAD_STATISTICS) && !RELEASE_LOG_DISABLED
>-    if (auto session = networkProcess().networkSession(sessionID)) {
>+    if (auto* session = networkProcess().networkSession(sessionID)) {
>         if (session->shouldLogCookieInformation())
>             NetworkResourceLoader::logCookieInformation(*this, "NetworkConnectionToWebProcess::cookiesForDOM", reinterpret_cast<const void*>(this), networkStorageSession, firstParty, sameSiteInfo, url, emptyString(), frameID, pageID, WTF::nullopt);
>     }
>@@ -544,7 +544,7 @@ void NetworkConnectionToWebProcess::setCookiesFromDOM(PAL::SessionID sessionID,
>     auto& networkStorageSession = storageSession(networkProcess(), sessionID);
>     networkStorageSession.setCookiesFromDOM(firstParty, sameSiteInfo, url, frameID, pageID, cookieString);
> #if ENABLE(RESOURCE_LOAD_STATISTICS) && !RELEASE_LOG_DISABLED
>-    if (auto session = networkProcess().networkSession(sessionID)) {
>+    if (auto* session = networkProcess().networkSession(sessionID)) {
>         if (session->shouldLogCookieInformation())
>             NetworkResourceLoader::logCookieInformation(*this, "NetworkConnectionToWebProcess::setCookiesFromDOM", reinterpret_cast<const void*>(this), networkStorageSession, firstParty, sameSiteInfo, url, emptyString(), frameID, pageID, WTF::nullopt);
>     }
>@@ -670,7 +670,7 @@ void NetworkConnectionToWebProcess::logUserInteraction(PAL::SessionID sessionID,
>     if (!sessionID.isValid())
>         return;
> 
>-    if (auto networkSession = networkProcess().networkSession(sessionID)) {
>+    if (auto* networkSession = networkProcess().networkSession(sessionID)) {
>         if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics())
>             resourceLoadStatistics->logUserInteraction(domain, [] { });
>     }
>@@ -678,7 +678,7 @@ void NetworkConnectionToWebProcess::logUserInteraction(PAL::SessionID sessionID,
> 
> void NetworkConnectionToWebProcess::logWebSocketLoading(PAL::SessionID sessionID, const RegistrableDomain& targetDomain, const RegistrableDomain& topFrameDomain, WallTime lastSeen)
> {
>-    if (auto networkSession = networkProcess().networkSession(sessionID)) {
>+    if (auto* networkSession = networkProcess().networkSession(sessionID)) {
>         if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics())
>             resourceLoadStatistics->logWebSocketLoading(targetDomain, topFrameDomain, lastSeen, [] { });
>     }
>@@ -686,7 +686,7 @@ void NetworkConnectionToWebProcess::logWebSocketLoading(PAL::SessionID sessionID
> 
> void NetworkConnectionToWebProcess::logSubresourceLoading(PAL::SessionID sessionID, const RegistrableDomain& targetDomain, const RegistrableDomain& topFrameDomain, WallTime lastSeen)
> {
>-    if (auto networkSession = networkProcess().networkSession(sessionID)) {
>+    if (auto* networkSession = networkProcess().networkSession(sessionID)) {
>         if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics())
>             resourceLoadStatistics->logSubresourceLoading(targetDomain, topFrameDomain, lastSeen, [] { });
>     }
>@@ -694,7 +694,7 @@ void NetworkConnectionToWebProcess::logSubresourceLoading(PAL::SessionID session
> 
> void NetworkConnectionToWebProcess::logSubresourceRedirect(PAL::SessionID sessionID, const RegistrableDomain& sourceDomain, const RegistrableDomain& targetDomain)
> {
>-    if (auto networkSession = networkProcess().networkSession(sessionID)) {
>+    if (auto* networkSession = networkProcess().networkSession(sessionID)) {
>         if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics())
>             resourceLoadStatistics->logSubresourceRedirect(sourceDomain, targetDomain, [] { });
>     }
>@@ -713,7 +713,7 @@ void NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(Vector<WebCore
> 
> void NetworkConnectionToWebProcess::hasStorageAccess(PAL::SessionID sessionID, const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain, uint64_t frameID, PageIdentifier pageID, CompletionHandler<void(bool)>&& completionHandler)
> {
>-    if (auto networkSession = networkProcess().networkSession(sessionID)) {
>+    if (auto* networkSession = networkProcess().networkSession(sessionID)) {
>         if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics()) {
>             resourceLoadStatistics->hasStorageAccess(subFrameDomain, topFrameDomain, frameID, pageID, WTFMove(completionHandler));
>             return;
>@@ -725,7 +725,7 @@ void NetworkConnectionToWebProcess::hasStorageAccess(PAL::SessionID sessionID, c
> 
> void NetworkConnectionToWebProcess::requestStorageAccess(PAL::SessionID sessionID, const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain, uint64_t frameID, PageIdentifier pageID, CompletionHandler<void(WebCore::StorageAccessWasGranted wasGranted, WebCore::StorageAccessPromptWasShown promptWasShown)>&& completionHandler)
> {
>-    if (auto networkSession = networkProcess().networkSession(sessionID)) {
>+    if (auto* networkSession = networkProcess().networkSession(sessionID)) {
>         if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics()) {
>             resourceLoadStatistics->requestStorageAccess(subFrameDomain, topFrameDomain, frameID, pageID, WTFMove(completionHandler));
>             return;
>@@ -737,7 +737,7 @@ void NetworkConnectionToWebProcess::requestStorageAccess(PAL::SessionID sessionI
> 
> void NetworkConnectionToWebProcess::requestStorageAccessUnderOpener(PAL::SessionID sessionID, WebCore::RegistrableDomain&& domainInNeedOfStorageAccess, PageIdentifier openerPageID, WebCore::RegistrableDomain&& openerDomain)
> {
>-    if (auto networkSession = networkProcess().networkSession(sessionID)) {
>+    if (auto* networkSession = networkProcess().networkSession(sessionID)) {
>         if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics())
>             resourceLoadStatistics->requestStorageAccessUnderOpener(WTFMove(domainInNeedOfStorageAccess), openerPageID, WTFMove(openerDomain));
>     }
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index 3ae2c9ea9dc42eae36bd23ccd07c147afe0e2e2e..12c21dedf1aab33c10c25054ceae9b2a1ea10f63 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -559,7 +559,9 @@ void NetworkProcess::forEachNetworkStorageSession(const Function<void(WebCore::N
> 
> NetworkSession* NetworkProcess::networkSession(const PAL::SessionID& sessionID) const
> {
>-    return m_networkSessions.get(sessionID);
>+    ASSERT(RunLoop::isMain());
>+    ASSERT(sessionID.isValid());
>+    return sessionID.isValid() ? m_networkSessions.get(sessionID) : nullptr;
> }
> 
> NetworkSession* NetworkProcess::networkSessionByConnection(IPC::Connection& connection) const
>@@ -572,11 +574,17 @@ NetworkSession* NetworkProcess::networkSessionByConnection(IPC::Connection& conn
> 
> void NetworkProcess::setSession(const PAL::SessionID& sessionID, Ref<NetworkSession>&& session)
> {
>+    ASSERT(RunLoop::isMain());
>+    ASSERT(sessionID.isValid());
>+    if (!sessionID.isValid())
>+        return;
>+
>     m_networkSessions.set(sessionID, WTFMove(session));
> }
> 
> void NetworkProcess::destroySession(const PAL::SessionID& sessionID)
> {
>+    ASSERT(RunLoop::isMain());
>     ASSERT(sessionID.isValid());
>     if (!sessionID.isValid())
>         return;
>@@ -2646,13 +2654,13 @@ void NetworkProcess::markAdClickAttributionsAsExpiredForTesting(PAL::SessionID s
> 
> void NetworkProcess::addKeptAliveLoad(Ref<NetworkResourceLoader>&& loader)
> {
>-    if (auto session = m_networkSessions.get(loader->sessionID()))
>+    if (auto* session = networkSession(loader->sessionID()))
>         session->addKeptAliveLoad(WTFMove(loader));
> }
> 
> void NetworkProcess::removeKeptAliveLoad(NetworkResourceLoader& loader)
> {
>-    if (auto session = m_networkSessions.get(loader.sessionID()))
>+    if (auto* session = networkSession(loader.sessionID()))
>         session->removeKeptAliveLoad(loader);
> }
> 
>@@ -2705,10 +2713,8 @@ void NetworkProcess::webProcessWasDisconnected(IPC::Connection& connection)
>         return;
> 
>     auto sessionID = m_sessionByConnection.take(connectionID);
>-    if (!m_networkSessions.contains(sessionID))
>-        return;
>-
>-    networkSession(sessionID)->storageManager().processDidCloseConnection(connection);
>+    if (auto* session = networkSession(sessionID))
>+        session->storageManager().processDidCloseConnection(connection);
> }
> 
> void NetworkProcess::webProcessSessionChanged(IPC::Connection& connection, PAL::SessionID newSessionID, const Vector<PageIdentifier>& pageIDs)
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.h b/Source/WebKit/NetworkProcess/NetworkProcess.h
>index 4f09984fe4bff176f99fc7d0169380244af9c7d7..83b4f3484640fbb3c456d71bb54f17b020f84761 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.h
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.h
>@@ -164,7 +164,7 @@ public:
>     NetworkCache::Cache* cache() { return m_cache.get(); }
> 
>     void setSession(const PAL::SessionID&, Ref<NetworkSession>&&);
>-    NetworkSession* networkSession(const PAL::SessionID&) const override;
>+    NetworkSession* networkSession(const PAL::SessionID&) const final;
>     NetworkSession* networkSessionByConnection(IPC::Connection&) const;
>     void destroySession(const PAL::SessionID&);
> 
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>index 0b3c081ec8321f74d779dbd43d9c770202a89a80..ea51d5398eba4613a5192df5f595fbd04be8f39a 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>@@ -213,7 +213,7 @@ void NetworkResourceLoader::retrieveCacheEntry(const ResourceRequest& request)
>     RefPtr<NetworkResourceLoader> loader(this);
>     if (isMainFrameLoad()) {
>         ASSERT(m_parameters.options.mode == FetchOptions::Mode::Navigate);
>-        if (auto session = m_connection->networkProcess().networkSession(sessionID())) {
>+        if (auto* session = m_connection->networkProcess().networkSession(sessionID())) {
>             if (auto entry = session->prefetchCache().take(request.url())) {
>                 if (!entry->redirectRequest.isNull()) {
>                     auto maxAgeCap = validateCacheEntryForMaxAgeCapValidation(request, entry->redirectRequest, entry->response);
>@@ -691,7 +691,7 @@ void NetworkResourceLoader::didFinishWithRedirectResponse(WebCore::ResourceReque
>     redirectResponse.setType(ResourceResponse::Type::Opaqueredirect);
>     if (!isCrossOriginPrefetch())
>         didReceiveResponse(WTFMove(redirectResponse), [] (auto) { });
>-    else if (auto session = m_connection->networkProcess().networkSession(sessionID()))
>+    else if (auto* session = m_connection->networkProcess().networkSession(sessionID()))
>         session->prefetchCache().storeRedirect(m_networkLoad->currentRequest().url(), WTFMove(redirectResponse), WTFMove(redirectRequest));
> 
>     WebCore::NetworkLoadMetrics networkLoadMetrics;
>@@ -820,7 +820,7 @@ void NetworkResourceLoader::tryStoreAsCacheEntry()
>         return;
> 
>     if (isCrossOriginPrefetch()) {
>-        if (auto session = m_connection->networkProcess().networkSession(sessionID()))
>+        if (auto* session = m_connection->networkProcess().networkSession(sessionID()))
>             session->prefetchCache().store(m_networkLoad->currentRequest().url(), WTFMove(m_response), WTFMove(m_bufferedDataForCache));
>         return;
>     }
>@@ -981,7 +981,7 @@ bool NetworkResourceLoader::shouldCaptureExtraNetworkLoadMetrics() const
> #if ENABLE(RESOURCE_LOAD_STATISTICS) && !RELEASE_LOG_DISABLED
> bool NetworkResourceLoader::shouldLogCookieInformation(NetworkConnectionToWebProcess& connection, const PAL::SessionID& sessionID)
> {
>-    if (auto session = connection.networkProcess().networkSession(sessionID))
>+    if (auto* session = connection.networkProcess().networkSession(sessionID))
>         return session->shouldLogCookieInformation();
>     return false;
> }
>diff --git a/Source/WebKit/NetworkProcess/ios/NetworkConnectionToWebProcessIOS.mm b/Source/WebKit/NetworkProcess/ios/NetworkConnectionToWebProcessIOS.mm
>index eb894981f21113cc9da2b472a748810bb5ccde9b..23745288a06d0114591968b5086deb5f45aa0ac6 100644
>--- a/Source/WebKit/NetworkProcess/ios/NetworkConnectionToWebProcessIOS.mm
>+++ b/Source/WebKit/NetworkProcess/ios/NetworkConnectionToWebProcessIOS.mm
>@@ -54,28 +54,28 @@ UIViewController *NetworkConnectionToWebProcess::paymentCoordinatorPresentingVie
> 
> const String& NetworkConnectionToWebProcess::paymentCoordinatorBoundInterfaceIdentifier(const WebPaymentCoordinatorProxy&, PAL::SessionID sessionID)
> {
>-    if (auto session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>+    if (auto* session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>         return session->boundInterfaceIdentifier();
>     return emptyString();
> }
> 
> const String& NetworkConnectionToWebProcess::paymentCoordinatorCTDataConnectionServiceType(const WebPaymentCoordinatorProxy&, PAL::SessionID sessionID)
> {
>-    if (auto session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>+    if (auto* session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>         return session->ctDataConnectionServiceType();
>     return emptyString();
> }
> 
> const String& NetworkConnectionToWebProcess::paymentCoordinatorSourceApplicationBundleIdentifier(const WebPaymentCoordinatorProxy&, PAL::SessionID sessionID)
> {
>-    if (auto session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>+    if (auto* session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>         return session->sourceApplicationBundleIdentifier();
>     return emptyString();
> }
> 
> const String& NetworkConnectionToWebProcess::paymentCoordinatorSourceApplicationSecondaryIdentifier(const WebPaymentCoordinatorProxy&, PAL::SessionID sessionID)
> {
>-    if (auto session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>+    if (auto* session = static_cast<NetworkSessionCocoa*>(m_networkProcess->networkSession(sessionID)))
>         return session->sourceApplicationSecondaryIdentifier();
>     return emptyString();
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 199418: 373353 | 373357