Changeset 217445 in webkit

Timestamp:

May 25, 2017 1:53:49 PM (7 years ago)

Author:

Chris Dumez

Message:

DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader
​https://bugs.webkit.org/show_bug.cgi?id=172578
<rdar://problem/30754582>

Reviewed by Youenn Fablet.

Source/WebCore:

DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader. The rest of the methods do not.
It is unsafe for it to rely on the resource's loader because it gets cleared when the load completes. A CachedRawresource
may be reused from the memory cache once its load has completed.

This would cause crashes in CachedRawResource::didAddClient() when replaying the redirects because it would call
DocumentThreadableLoader::redirectReceived() and potentially not have a loader anymore. To hit this exact code path,
you would need to make repeated XHR to a cacheable simple cross-origin resource that has cacheable redirect.

Test: http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash.html

• loader/DocumentThreadableLoader.cpp:

(WebCore::DocumentThreadableLoader::redirectReceived):

• loader/DocumentThreadableLoader.h:

LayoutTests:

Add layout test coverage.

• http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash-expected.txt: Added.
• http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash-expected.txt (added)
• LayoutTests/http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/DocumentThreadableLoader.cpp (modified) (3 diffs)
• Source/WebCore/loader/DocumentThreadableLoader.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-05-25  Chris Dumez  <cdumez@apple.com>
+
+        DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader
+        https://bugs.webkit.org/show_bug.cgi?id=172578
+        <rdar://problem/30754582>
+
+        Reviewed by Youenn Fablet.
+
+        Add layout test coverage.
+
+        * http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash-expected.txt: Added.
+        * http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash.html: Added.
+
 2017-05-24  Jiewen Tan  <jiewen_tan@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-05-25  Chris Dumez  <cdumez@apple.com>
+
+        DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader
+        https://bugs.webkit.org/show_bug.cgi?id=172578
+        <rdar://problem/30754582>
+
+        Reviewed by Youenn Fablet.
+
+        DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader. The rest of the methods do not.
+        It is unsafe for it to rely on the resource's loader because it gets cleared when the load completes. A CachedRawresource
+        may be reused from the memory cache once its load has completed.
+
+        This would cause crashes in CachedRawResource::didAddClient() when replaying the redirects because it would call
+        DocumentThreadableLoader::redirectReceived() and potentially not have a loader anymore. To hit this exact code path,
+        you would need to make repeated XHR to a cacheable simple cross-origin resource that has cacheable redirect.
+
+        Test: http/tests/xmlhttprequest/cacheable-cross-origin-redirect-crash.html
+
+        * loader/DocumentThreadableLoader.cpp:
+        (WebCore::DocumentThreadableLoader::redirectReceived):
+        * loader/DocumentThreadableLoader.h:
+
 2017-05-25  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp

@@ -229 +229 @@
 
     Ref<DocumentThreadableLoader> protectedThis(*this);
+    ++m_redirectCount;
 
     // FIXME: We restrict this check to Fetch API for the moment, as this might disrupt WorkerScriptLoader.
@@ -253 +254 @@
 
     ASSERT(m_resource);
-    ASSERT(m_resource->loader());
+    ASSERT(m_originalHeaders);
+
+    // Use a unique for subsequent loads if needed.
+    // https://fetch.spec.whatwg.org/#concept-http-redirect-fetch (Step 10).
     ASSERT(m_options.mode == FetchOptions::Mode::Cors);
-    ASSERT(m_originalHeaders);
-
-    // Loader might have modified the origin to a unique one, let's reuse it for subsequent loads.
-    m_origin = m_resource->loader()->origin();
+    if (!securityOrigin().canRequest(redirectResponse.url()) && !SecurityOrigin::create(redirectResponse.url())->canRequest(request.url()))
+        m_origin = SecurityOrigin::createUnique();
 
     // Except in case where preflight is needed, loading should be able to continue on its own.
@@ -266 +268 @@
 
     m_options.allowCredentials = DoNotAllowStoredCredentials;
-    m_options.maxRedirectCount -= m_resource->loader()->redirectCount();
+    m_options.maxRedirectCount -= m_redirectCount;
+    m_redirectCount = 0;
 
     clearResource();

trunk/Source/WebCore/loader/DocumentThreadableLoader.h

@@ -135 +135 @@
         std::optional<CrossOriginPreflightChecker> m_preflightChecker;
         std::optional<HTTPHeaderMap> m_originalHeaders;
+        unsigned m_redirectCount { 0 };
 
         ShouldLogError m_shouldLogError;