Fix race condition between BinaryUploadService and downloads

If the BinaryUploadService attempts to read the file before the download
code has renamed it to its final location, we can end up having a race
condition between the renaming and reading the file contents. So the
BinaryUploadService will now wait for the file to be renamed before reading
the contents.

We'll have to revisit this decision when launching a blocking mode, since
then the file isn't renamed until after the upload.

Bug: 980784
Change-Id: I5228e8610df9d5db7e84fbfbc2bb53bea79a9e9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769677Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690791}

chrome/browser/safe_browsing/download_protection/download_item_request.cc

@@ -17,6 +17,9 @@ namespace {
 
 std::string GetFileContentsBlocking(base::FilePath path) {
   base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!file.IsValid())
+    return "";
+
   int64_t file_size = file.GetLength();
   std::string contents;
   contents.resize(file_size);
@@ -38,7 +41,10 @@ std::string GetFileContentsBlocking(base::FilePath path) {
 
 DownloadItemRequest::DownloadItemRequest(download::DownloadItem* item,
                                          BinaryUploadService::Callback callback)
-    : Request(std::move(callback)), item_(item), weakptr_factory_(this) {
+    : Request(std::move(callback)),
+      item_(item),
+      download_item_renamed_(false),
+      weakptr_factory_(this) {
   item_->AddObserver(this);
 }
 
@@ -54,18 +60,38 @@ void DownloadItemRequest::GetFileContents(
     return;
   }
 
+  pending_callbacks_.push_back(std::move(callback));
+
+  if (download_item_renamed_)
+    RunPendingGetFileContentsCallbacks();
+}
+
+void DownloadItemRequest::RunPendingGetFileContentsCallbacks() {
+  for (auto it = pending_callbacks_.begin(); it != pending_callbacks_.end();
+       it++) {
     base::PostTaskAndReplyWithResult(
         FROM_HERE,
-      {base::ThreadPool(), base::TaskPriority::USER_VISIBLE, base::MayBlock()},
+        {base::ThreadPool(), base::TaskPriority::USER_VISIBLE,
+         base::MayBlock()},
         base::BindOnce(&GetFileContentsBlocking, item_->GetFullPath()),
         base::BindOnce(&DownloadItemRequest::OnGotFileContents,
-                     weakptr_factory_.GetWeakPtr(), std::move(callback)));
+                       weakptr_factory_.GetWeakPtr(), std::move(*it)));
+  }
+
+  pending_callbacks_.clear();
 }
 
 size_t DownloadItemRequest::GetFileSize() {
   return item_ == nullptr ? 0 : item_->GetTotalBytes();
 }
 
+void DownloadItemRequest::OnDownloadUpdated(download::DownloadItem* download) {
+  if (download == item_ && item_->GetFullPath() == item_->GetTargetFilePath()) {
+    download_item_renamed_ = true;
+    RunPendingGetFileContentsCallbacks();
+  }
+}
+
 void DownloadItemRequest::OnDownloadDestroyed(
     download::DownloadItem* download) {
   if (download == item_)

chrome/browser/safe_browsing/download_protection/download_item_request.h

@@ -33,15 +33,27 @@ class DownloadItemRequest : public BinaryUploadService::Request,
 
   // download::DownloadItem::Observer implementation.
   void OnDownloadDestroyed(download::DownloadItem* download) override;
+  void OnDownloadUpdated(download::DownloadItem* download) override;
 
  private:
   void OnGotFileContents(base::OnceCallback<void(const std::string&)> callback,
                          const std::string& contents);
 
+  // Calls to GetFileContents can be deferred if the download item is not yet
+  // renamed to its final location. When ready, this method runs those
+  // callbacks.
+  void RunPendingGetFileContentsCallbacks();
+
   // Pointer the download item for upload. This must be accessed only the UI
-  // thread.
+  // thread. Unowned.
   download::DownloadItem* item_;
 
+  // Whether the download item has been renamed to its final destination yet.
+  bool download_item_renamed_;
+
+  // All pending callbacks to GetFileContents before the download item is ready.
+  std::vector<base::OnceCallback<void(const std::string&)>> pending_callbacks_;
+
   base::WeakPtrFactory<DownloadItemRequest> weakptr_factory_;
 };
 }  // namespace safe_browsing

chrome/browser/safe_browsing/download_protection/download_item_request_unittest.cc

+// Copyright 2019 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 "chrome/browser/safe_browsing/download_protection/download_item_request.h"
+
+#include "base/bind_helpers.h"
+#include "base/files/scoped_temp_dir.h"
+#include "components/download/public/common/mock_download_item.h"
+#include "content/public/test/browser_task_environment.h"
+#include "content/public/test/test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace safe_browsing {
+
+using ::testing::Return;
+using ::testing::ReturnRef;
+
+class DownloadItemRequestTest : public ::testing::Test {
+ public:
+  DownloadItemRequestTest() : item_(), request_(&item_, base::DoNothing()) {}
+
+  void SetUp() override {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    download_path_ = temp_dir_.GetPath().AppendASCII("download_location");
+    download_temporary_path_ =
+        temp_dir_.GetPath().AppendASCII("temporary_location");
+
+    base::File file(download_path_,
+                    base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+    ASSERT_TRUE(file.IsValid());
+
+    download_contents_ = "download contents";
+    file.Write(0, download_contents_.c_str(), download_contents_.size());
+    file.Close();
+
+    ON_CALL(item_, GetTotalBytes())
+        .WillByDefault(Return(download_contents_.size()));
+    ON_CALL(item_, GetTargetFilePath())
+        .WillByDefault(ReturnRef(download_path_));
+  }
+
+ protected:
+  content::TestBrowserThreadBundle thread_bundle_;
+  download::MockDownloadItem item_;
+  DownloadItemRequest request_;
+  base::ScopedTempDir temp_dir_;
+  base::FilePath download_path_;
+  base::FilePath download_temporary_path_;
+  std::string download_contents_;
+};
+
+TEST_F(DownloadItemRequestTest, GetsSize) {
+  EXPECT_EQ(request_.GetFileSize(), download_contents_.size());
+}
+
+TEST_F(DownloadItemRequestTest, GetsContentsWaitsUntilRename) {
+  ON_CALL(item_, GetFullPath())
+      .WillByDefault(ReturnRef(download_temporary_path_));
+
+  std::string download_contents = "";
+  request_.GetFileContents(base::BindOnce(
+      [](std::string* target_contents, const std::string& contents) {
+        *target_contents = contents;
+      },
+      &download_contents));
+  content::RunAllTasksUntilIdle();
+  EXPECT_EQ(download_contents, "");
+
+  ON_CALL(item_, GetFullPath()).WillByDefault(ReturnRef(download_path_));
+  item_.NotifyObserversDownloadUpdated();
+
+  content::RunAllTasksUntilIdle();
+  EXPECT_EQ(download_contents, "download contents");
+}
+
+}  // namespace safe_browsing

chrome/test/BUILD.gn

@@ -4574,6 +4574,7 @@ test("unit_tests") {
       "../browser/safe_browsing/download_protection/binary_upload_service_unittest.cc",
       "../browser/safe_browsing/download_protection/download_feedback_service_unittest.cc",
       "../browser/safe_browsing/download_protection/download_feedback_unittest.cc",
+      "../browser/safe_browsing/download_protection/download_item_request_unittest.cc",
       "../browser/safe_browsing/download_protection/download_protection_service_unittest.cc",
       "../browser/safe_browsing/download_protection/file_analyzer_unittest.cc",
       "../browser/safe_browsing/download_protection/multipart_uploader_unittest.cc",