[subresource_filter] SB throttle can send multiple speculative requests.

components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_client.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/single_thread_task_runner.h"
+#include "components/safe_browsing_db/util.h"
+#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace subresource_filter {
+
+constexpr base::TimeDelta
+    SubresourceFilterSafeBrowsingClientRequest::kCheckURLTimeout;
+
+SubresourceFilterSafeBrowsingClient::SubresourceFilterSafeBrowsingClient(
+    scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> database_manager,
+    const base::WeakPtr<SubresourceFilterSafeBrowsingActivationThrottle>&
+        throttle,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_io_task_runner)

[engedy, 2017/04/25 22:03:54]

nit: s//ui_task_runner|throttle_task_runner/

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+    : database_manager_(std::move(database_manager)),
+      throttle_(throttle),
+      io_task_runner_(std::move(io_task_runner)),
+      ui_task_runner_(std::move(ui_io_task_runner)) {}
+
+SubresourceFilterSafeBrowsingClient::~SubresourceFilterSafeBrowsingClient() {}
+
+void SubresourceFilterSafeBrowsingClient::CheckUrlOnIO(const GURL& url,
+                                                       size_t request_id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  DCHECK(!url.is_empty());
+
+  auto request = base::MakeUnique<SubresourceFilterSafeBrowsingClientRequest>(
+      url, request_id, database_manager_, io_task_runner_, this);
+  auto* raw_request = request.get();
+  DCHECK(requests_.find(raw_request) == requests_.end());
+  requests_[raw_request] = std::move(request);
+  raw_request->Start();

[engedy, 2017/04/25 22:03:54]

nit: Add comment that |raw_request| might be destroyed after this line.

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+}
+
+void SubresourceFilterSafeBrowsingClient::OnCheckBrowseUrlResult(
+    SubresourceFilterSafeBrowsingClientRequest* request,
+    safe_browsing::SBThreatType threat_type,
+    const safe_browsing::ThreatMetadata& metadata) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  ui_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SubresourceFilterSafeBrowsingActivationThrottle::
+                                OnCheckUrlResultOnUI,
+                            throttle_, request->url(), request->request_id(),
+                            threat_type, metadata.threat_pattern_type));
+
+  DCHECK(requests_.find(request) != requests_.end());
+  requests_.erase(request);
+}
+
+SubresourceFilterSafeBrowsingClientRequest::
+    SubresourceFilterSafeBrowsingClientRequest(
+        const GURL& url,
+        size_t request_id,
+        scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+            database_manager,
+        scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+        SubresourceFilterSafeBrowsingClient* client)
+    : url_(url),
+      request_id_(request_id),
+      database_manager_(std::move(database_manager)),
+      client_(client) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  timer_.SetTaskRunner(std::move(io_task_runner));
+}
+
+SubresourceFilterSafeBrowsingClientRequest::
+    ~SubresourceFilterSafeBrowsingClientRequest() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  database_manager_->CancelCheck(this);

[engedy, 2017/04/25 22:03:54]

Could you please double-check that spurious calls to this are OK, plus that it
cannot happen that there is already a task posted to invoke the callback (or it
is handled).

[Charlie Harrison, 2017/04/26 14:30:25]

For the former:  I don't think spurious calls are always ok (the remote DB
DCHECKs this fact). Let me tweak the class.

For the latter: I also think this is fine AFAICT because we use weak ptrs to
service the posted tasks. So as long as we immediately destroy the request we
should be ok.

nparker: would you verify?

[engedy, 2017/04/26 14:56:39]

To clarify the latter: I was wondering if the following might happen.

  1.) SubresourceFilterSafeBrowsingClientRequest::OnCheckBrowseUrlResult
      is posted by something inside SBDBMgr to the IO thread.
  2.) SubresourceFilterSafeBrowsingClient is destroyed, and destroys all
      ClientRequests along with it.
  3.) SBDBMgr::CancelCheck called, but too late.
  4.) The task posted in (1) is processed anyway and results in UAF.

[Charlie Harrison, 2017/04/26 15:29:16]

Yes that was my understanding of your question. As far as I could tell in the
code, this is handled by using weak pointers that are invalidated by calls to
Cancel. So the tasks should never be serviced if Cancel is called (even if it is
already posted).

+  timer_.Stop();
+}
+
+void SubresourceFilterSafeBrowsingClientRequest::Start() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  start_time_ = base::TimeTicks::Now();
+  if (database_manager_->CheckUrlForSubresourceFilter(url_, this)) {
+    OnCheckBrowseUrlResult(url_, safe_browsing::SB_THREAT_TYPE_SAFE,
+                           safe_browsing::ThreatMetadata());
+    return;
+  }
+  timer_.Start(
+      FROM_HERE, kCheckURLTimeout,

[engedy, 2017/04/25 22:03:54]

nit: #include "base/location.h"

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+      base::Bind(&SubresourceFilterSafeBrowsingClientRequest::OnCheckUrlTimeout,
+                 base::Unretained(this)));

[engedy, 2017/04/25 22:03:54]

nit: #include "base/bind_helpers.h"

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+}
+
+void SubresourceFilterSafeBrowsingClientRequest::OnCheckBrowseUrlResult(
+    const GURL& url,
+    safe_browsing::SBThreatType threat_type,
+    const safe_browsing::ThreatMetadata& metadata) {

[engedy, 2017/04/25 22:03:54]

nit: DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

[Charlie Harrison, 2017/04/26 14:30:25]

Done.

+  DCHECK_EQ(url_, url);
+  UMA_HISTOGRAM_TIMES("SubresourceFilter.SafeBrowsing.CheckTime",
+                      base::TimeTicks::Now() - start_time_);
+  client_->OnCheckBrowseUrlResult(this, threat_type, metadata);
+}
+
+void SubresourceFilterSafeBrowsingClientRequest::OnCheckUrlTimeout() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  database_manager_->CancelCheck(this);
+  OnCheckBrowseUrlResult(url_, safe_browsing::SB_THREAT_TYPE_SAFE,
+                         safe_browsing::ThreatMetadata());
+}
+
+}  // namespace subresource_filter