[fuchsia] Fix a use-after-free in WebEngineURLLoaderThrottle

It was assumed that all WebEngineURLLoaderThrottles would be
destroyed before their corresponding RenderFrame was destroyed.
This assumption is incorrect, resulting in a potential use-after-free
when attempting to access a UrlRequestRulesReceiver. This object has
its lifespan bound to a RenderFrameObserver and can be destroyed
before all of the WebEngineURLLoaderThrottles that use it have been
destroyed.

This CL fixes the issue by passing the UrlRequestRewriteRules directly
to the WebEngineURLLoaderThrottle constructor. As a result, URL
requests in-flight while a user updates the rewrite rules will use the
older version of the rewrite rules. We deem this change in behavior to
be acceptable since such an occurrence is bound to be rare and the
delay between a WebEngineURLLoaderThrottle creation and its actual use
is bound to be short.

This CL also modifies the logic a bit to skip creation of the throttle
if there are no rewrite rules to apply at the time of the network
request creation. Since the rules provider should now be accessed from
the same thread in both the renderer and the browser process, the locks
around accessing the rules have been removed. Instead, each throttle
keeps a reference to a thread-safe ref-counted set of rules.

Since this fix is still speculative, the extra logs for debugging this
issue are left in place in this CL, until we get data confirming the
crashes are no longer occurring.

(cherry picked from commit 4c38547)

Bug: 1181062
Change-Id: I580664f9ed2fdf9874e36b1cbc508c2301ef36e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2812464
Commit-Queue: Fabrice de Gans-Riberi <fdegans@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#870653}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2815225
Reviewed-by: Fabrice de Gans-Riberi <fdegans@chromium.org>
Commit-Queue: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/4471@{#2}
Cr-Branched-From: f5ba97e-refs/heads/master@{#870382}

fuchsia/engine/browser/url_request_rewrite_rules_manager.cc

@@ -137,18 +137,20 @@ UrlRequestRewriteRulesManager::UrlRequestRewriteRulesManager(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents) {}
 
-UrlRequestRewriteRulesManager::~UrlRequestRewriteRulesManager() = default;
+UrlRequestRewriteRulesManager::~UrlRequestRewriteRulesManager() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
 zx_status_t UrlRequestRewriteRulesManager::OnRulesUpdated(
     std::vector<fuchsia::web::UrlRequestRewriteRule> rules,
     fuchsia::web::Frame::SetUrlRequestRewriteRulesCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!ValidateRules(rules)) {
-    base::AutoLock auto_lock(lock_);
     cached_rules_ = nullptr;
     return ZX_ERR_INVALID_ARGS;
   }
 
-  base::AutoLock auto_lock(lock_);
   cached_rules_ =
       base::MakeRefCounted<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>(
           mojo::ConvertTo<std::vector<mojom::UrlRequestRulePtr>>(
@@ -165,16 +167,18 @@ zx_status_t UrlRequestRewriteRulesManager::OnRulesUpdated(
   return ZX_OK;
 }
 
-scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
+scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>&
 UrlRequestRewriteRulesManager::GetCachedRules() {
-  base::AutoLock auto_lock(lock_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return cached_rules_;
 }
 
-UrlRequestRewriteRulesManager::UrlRequestRewriteRulesManager() {}
+UrlRequestRewriteRulesManager::UrlRequestRewriteRulesManager() = default;
 
 void UrlRequestRewriteRulesManager::RenderFrameCreated(
     content::RenderFrameHost* render_frame_host) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // Register the frame rules receiver.
   mojo::AssociatedRemote<mojom::UrlRequestRulesReceiver> rules_receiver;
   render_frame_host->GetRemoteAssociatedInterfaces()->GetInterface(
@@ -183,7 +187,6 @@ void UrlRequestRewriteRulesManager::RenderFrameCreated(
       render_frame_host->GetGlobalFrameRoutingId(), std::move(rules_receiver));
   DCHECK(iter.second);
 
-  base::AutoLock auto_lock(lock_);
   if (cached_rules_) {
     // Send an initial set of rules.
     iter.first->second->OnRulesUpdated(mojo::Clone(cached_rules_->data));
@@ -192,6 +195,8 @@ void UrlRequestRewriteRulesManager::RenderFrameCreated(
 
 void UrlRequestRewriteRulesManager::RenderFrameDeleted(
     content::RenderFrameHost* render_frame_host) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   size_t removed =
       active_remotes_.erase(render_frame_host->GetGlobalFrameRoutingId());
   DCHECK_EQ(removed, 1u);

fuchsia/engine/browser/url_request_rewrite_rules_manager.h

@@ -7,7 +7,7 @@
 
 #include <fuchsia/web/cpp/fidl.h>
 
-#include "base/synchronization/lock.h"
+#include "base/sequence_checker.h"
 #include "content/public/browser/global_routing_id.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "fuchsia/engine/common/web_engine_url_loader_throttle.h"
@@ -23,8 +23,7 @@ class WebContents;
 // Adapts the UrlRequestRewrite FIDL API to be sent to the renderers over the
 // over the UrlRequestRewrite Mojo API.
 class WEB_ENGINE_EXPORT UrlRequestRewriteRulesManager
-    : public content::WebContentsObserver,
-      public WebEngineURLLoaderThrottle::CachedRulesProvider {
+    : public content::WebContentsObserver {
  public:
   static std::unique_ptr<UrlRequestRewriteRulesManager> CreateForTesting();
 
@@ -38,9 +37,8 @@ class WEB_ENGINE_EXPORT UrlRequestRewriteRulesManager
       std::vector<fuchsia::web::UrlRequestRewriteRule> rules,
       fuchsia::web::Frame::SetUrlRequestRewriteRulesCallback callback);
 
-  // WebEngineURLLoaderThrottle::CachedRulesProvider implementation.
-  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-  GetCachedRules() override;
+  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>&
+  GetCachedRules();
 
  private:
   // Test-only constructor.
@@ -50,15 +48,16 @@ class WEB_ENGINE_EXPORT UrlRequestRewriteRulesManager
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
   void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
 
-  base::Lock lock_;
   scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-      cached_rules_ GUARDED_BY(lock_);
+      cached_rules_;
 
   // Map of GlobalRoutingID to their current associated remote.
   std::map<content::GlobalFrameRoutingId,
            mojo::AssociatedRemote<mojom::UrlRequestRulesReceiver>>
       active_remotes_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(UrlRequestRewriteRulesManager);
 };
 

fuchsia/engine/browser/web_engine_content_browser_client.cc

@@ -257,9 +257,13 @@ WebEngineContentBrowserClient::CreateURLLoaderThrottles(
   }
 
   std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;
-  throttles.emplace_back(std::make_unique<WebEngineURLLoaderThrottle>(
+  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>& rules =
       FrameImpl::FromWebContents(wc_getter.Run())
-          ->url_request_rewrite_rules_manager()));
+          ->url_request_rewrite_rules_manager()
+          ->GetCachedRules();
+  if (rules) {
+    throttles.emplace_back(std::make_unique<WebEngineURLLoaderThrottle>(rules));
+  }
   return throttles;
 }
 

fuchsia/engine/common/web_engine_url_loader_throttle.cc

@@ -171,9 +171,9 @@ bool IsRequestAllowed(
 }  // namespace
 
 WebEngineURLLoaderThrottle::WebEngineURLLoaderThrottle(
-    CachedRulesProvider* cached_rules_provider)
-    : cached_rules_provider_(cached_rules_provider) {
-  DCHECK(cached_rules_provider);
+    scoped_refptr<UrlRequestRewriteRules> rules)
+    : rules_(rules) {
+  DCHECK(rules_);
 }
 
 WebEngineURLLoaderThrottle::~WebEngineURLLoaderThrottle() = default;
@@ -183,19 +183,14 @@ void WebEngineURLLoaderThrottle::DetachFromCurrentSequence() {}
 void WebEngineURLLoaderThrottle::WillStartRequest(
     network::ResourceRequest* request,
     bool* defer) {
-  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-      cached_rules = cached_rules_provider_->GetCachedRules();
-  // |cached_rules| may be empty if no rule was ever sent to WebEngine.
-  if (cached_rules) {
-    if (!IsRequestAllowed(request, *cached_rules)) {
-      delegate_->CancelWithError(net::ERR_ABORTED,
-                                 "Resource load blocked by embedder policy.");
-      return;
-    }
-
-    for (const auto& rule : cached_rules->data)
-      ApplyRule(request, rule);
+  if (!IsRequestAllowed(request, *rules_)) {
+    delegate_->CancelWithError(net::ERR_ABORTED,
+                               "Resource load blocked by embedder policy.");
+    return;
   }
+
+  for (const auto& rule : rules_->data)
+    ApplyRule(request, rule);
   *defer = false;
 }
 

fuchsia/engine/common/web_engine_url_loader_throttle.h

@@ -7,7 +7,6 @@
 
 #include <vector>
 
-#include "base/containers/flat_set.h"
 #include "base/memory/ref_counted.h"
 #include "fuchsia/engine/url_request_rewrite.mojom.h"
 #include "fuchsia/engine/web_engine_export.h"
@@ -22,21 +21,14 @@ class WEB_ENGINE_EXPORT WebEngineURLLoaderThrottle
   using UrlRequestRewriteRules =
       base::RefCountedData<std::vector<mojom::UrlRequestRulePtr>>;
 
-  // An interface to provide rewrite rules to the throttle. Its
-  // implementation must outlive the WebEngineURLLoaderThrottle.
-  class CachedRulesProvider {
-   public:
-    virtual ~CachedRulesProvider() = default;
-
-    // Gets cached rules. This call can be made on any sequence, as
-    // URLLoaderThrottles are not guaranteed to stay on the same sequence.
-    virtual scoped_refptr<UrlRequestRewriteRules> GetCachedRules() = 0;
-  };
-
   explicit WebEngineURLLoaderThrottle(
-      CachedRulesProvider* cached_rules_provider);
+      scoped_refptr<UrlRequestRewriteRules> rules);
   ~WebEngineURLLoaderThrottle() override;
 
+  WebEngineURLLoaderThrottle(const WebEngineURLLoaderThrottle&) = delete;
+  WebEngineURLLoaderThrottle& operator=(const WebEngineURLLoaderThrottle&) =
+      delete;
+
   // blink::URLLoaderThrottle implementation.
   void DetachFromCurrentSequence() override;
   void WillStartRequest(network::ResourceRequest* request,
@@ -58,9 +50,7 @@ class WEB_ENGINE_EXPORT WebEngineURLLoaderThrottle
       network::ResourceRequest* request,
       const mojom::UrlRequestRewriteAddHeadersPtr& add_headers);
 
-  CachedRulesProvider* const cached_rules_provider_;
-
-  DISALLOW_COPY_AND_ASSIGN(WebEngineURLLoaderThrottle);
+  scoped_refptr<UrlRequestRewriteRules> rules_;
 };
 
 #endif  // FUCHSIA_ENGINE_COMMON_WEB_ENGINE_URL_LOADER_THROTTLE_H_

fuchsia/engine/common/web_engine_url_loader_throttle_unittest.cc

@@ -22,31 +22,6 @@ constexpr char kMixedCaseCorsExemptHeader2[] = "Another-CoRs-ExEmPt-2";
 constexpr char kUpperCaseCorsExemptHeader2[] = "ANOTHER-CORS-EXEMPT-2";
 constexpr char kRequiresCorsHeader[] = "requires-cors";
 
-class TestCachedRulesProvider
-    : public WebEngineURLLoaderThrottle::CachedRulesProvider {
- public:
-  TestCachedRulesProvider() = default;
-  ~TestCachedRulesProvider() override = default;
-
-  void SetCachedRules(
-      scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-          cached_rules) {
-    cached_rules_ = cached_rules;
-  }
-
-  // WebEngineURLLoaderThrottle::CachedRulesProvider implementation.
-  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-  GetCachedRules() override {
-    return cached_rules_;
-  }
-
- private:
-  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-      cached_rules_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestCachedRulesProvider);
-};
-
 }  // namespace
 
 class WebEngineURLLoaderThrottleTest : public testing::Test {
@@ -77,12 +52,9 @@ TEST_F(WebEngineURLLoaderThrottleTest, WildcardHosts) {
   std::vector<mojom::UrlRequestRulePtr> rules;
   rules.push_back(std::move(rule));
 
-  TestCachedRulesProvider provider;
-  provider.SetCachedRules(
+  WebEngineURLLoaderThrottle throttle(
       base::MakeRefCounted<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>(
           std::move(rules)));
-
-  WebEngineURLLoaderThrottle throttle(&provider);
   bool defer = false;
 
   network::ResourceRequest request1;
@@ -134,13 +106,10 @@ TEST_F(WebEngineURLLoaderThrottleTest, CorsAwareHeaders) {
   std::vector<mojom::UrlRequestRulePtr> rules;
   rules.push_back(std::move(rule));
 
-  TestCachedRulesProvider provider;
-  provider.SetCachedRules(
+  WebEngineURLLoaderThrottle throttle(
       base::MakeRefCounted<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>(
           std::move(rules)));
 
-  WebEngineURLLoaderThrottle throttle(&provider);
-
   network::ResourceRequest request;
   request.url = GURL("http://test.net");
   bool defer = false;
@@ -181,12 +150,9 @@ TEST_F(WebEngineURLLoaderThrottleTest, DataReplacementUrl) {
   std::vector<mojom::UrlRequestRulePtr> rules;
   rules.push_back(std::move(rule));
 
-  TestCachedRulesProvider provider;
-  provider.SetCachedRules(
+  WebEngineURLLoaderThrottle throttle(
       base::MakeRefCounted<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>(
           std::move(rules)));
-
-  WebEngineURLLoaderThrottle throttle(&provider);
   bool defer = false;
 
   network::ResourceRequest request;
@@ -240,12 +206,9 @@ TEST_F(WebEngineURLLoaderThrottleTest, AllowAndDeny) {
     rules.push_back(std::move(rule));
   }
 
-  TestCachedRulesProvider provider;
-  provider.SetCachedRules(
+  WebEngineURLLoaderThrottle throttle(
       base::MakeRefCounted<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>(
           std::move(rules)));
-
-  WebEngineURLLoaderThrottle throttle(&provider);
   bool defer = false;
 
   TestThrottleDelegate delegate;

fuchsia/engine/renderer/url_request_rules_receiver.cc

@@ -28,6 +28,12 @@ UrlRequestRulesReceiver::~UrlRequestRulesReceiver() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
+scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>&
+UrlRequestRulesReceiver::GetCachedRules() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return cached_rules_;
+}
+
 void UrlRequestRulesReceiver::OnUrlRequestRulesReceiverAssociatedReceiver(
     mojo::PendingAssociatedReceiver<mojom::UrlRequestRulesReceiver> receiver) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -38,14 +44,7 @@ void UrlRequestRulesReceiver::OnUrlRequestRulesReceiverAssociatedReceiver(
 void UrlRequestRulesReceiver::OnRulesUpdated(
     std::vector<mojom::UrlRequestRulePtr> rules) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  base::AutoLock auto_lock(lock_);
   cached_rules_ =
       base::MakeRefCounted<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>(
           std::move(rules));
 }
-
-scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-UrlRequestRulesReceiver::GetCachedRules() {
-  base::AutoLock auto_lock(lock_);
-  return cached_rules_;
-}

fuchsia/engine/renderer/url_request_rules_receiver.h

@@ -6,9 +6,6 @@
 #define FUCHSIA_ENGINE_RENDERER_URL_REQUEST_RULES_RECEIVER_H_
 
 #include "base/sequence_checker.h"
-#include "base/synchronization/lock.h"
-#include "base/thread_annotations.h"
-#include "content/public/renderer/render_frame_observer.h"
 #include "fuchsia/engine/common/web_engine_url_loader_throttle.h"
 #include "fuchsia/engine/url_request_rewrite.mojom.h"
 #include "mojo/public/cpp/bindings/associated_receiver.h"
@@ -21,42 +18,31 @@ class RenderFrame;
 
 // Provides rewriting rules for network requests. Owned by
 // WebEngineRenderFrameObserver, this object will be destroyed on RenderFrame
-// destruction. This is guaranteed to outlive any WebEngineURLLoaderThrottle
-// that uses it as the RenderFrame destruction will have triggered the
-// destruction of all pending WebEngineURLLoaderThrottles.
-// This class should only be used on the IO thread, with the exception of the
-// GetCachedRules() implementation, which can be called from any sequence.
-class UrlRequestRulesReceiver
-    : public mojom::UrlRequestRulesReceiver,
-      public WebEngineURLLoaderThrottle::CachedRulesProvider {
+// destruction. This class should only be used on the IO thread.
+class UrlRequestRulesReceiver : public mojom::UrlRequestRulesReceiver {
  public:
-  UrlRequestRulesReceiver(content::RenderFrame* render_frame);
+  explicit UrlRequestRulesReceiver(content::RenderFrame* render_frame);
   ~UrlRequestRulesReceiver() override;
 
+  UrlRequestRulesReceiver(const UrlRequestRulesReceiver&) = delete;
+  UrlRequestRulesReceiver& operator=(const UrlRequestRulesReceiver&) = delete;
+
+  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>&
+  GetCachedRules();
+
  private:
   void OnUrlRequestRulesReceiverAssociatedReceiver(
       mojo::PendingAssociatedReceiver<mojom::UrlRequestRulesReceiver> receiver);
 
   // mojom::UrlRequestRulesReceiver implementation.
   void OnRulesUpdated(std::vector<mojom::UrlRequestRulePtr> rules) override;
 
-  // WebEngineURLLoaderThrottle::CachedRulesProvider implementation.
   scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-  GetCachedRules() override;
-
-  base::Lock lock_;
-
-  // This is accessed by WebEngineURLLoaderThrottles, which can be off-sequence
-  // in the case of synchronous network requests.
-  scoped_refptr<WebEngineURLLoaderThrottle::UrlRequestRewriteRules>
-      cached_rules_ GUARDED_BY(lock_);
-
+      cached_rules_;
   mojo::AssociatedReceiver<mojom::UrlRequestRulesReceiver>
       url_request_rules_receiver_{this};
 
   SEQUENCE_CHECKER(sequence_checker_);
-
-  DISALLOW_COPY_AND_ASSIGN(UrlRequestRulesReceiver);
 };
 
 #endif  // FUCHSIA_ENGINE_RENDERER_URL_REQUEST_RULES_RECEIVER_H_