SimpleURLLoader: Make downloaded file path accessible after destruction.

When downloading to a file, if code deleted the SimpleURLLoader on
completion, and then tried to access the FilePath object passed to them
by the SimpleURLLoader, they're run into a use-after-free. This CL fixes
that.

Bug: 835963
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I8b840544be743aa979145bf46c8d35de4b25c12c
Reviewed-on: https://chromium-review.googlesource.com/1024694
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553745}

chrome/browser/chromeos/customization/customization_wallpaper_downloader.cc

@@ -150,7 +150,7 @@ void CustomizationWallpaperDownloader::OnWallpaperDirectoryCreated(
 }
 
 void CustomizationWallpaperDownloader::OnSimpleLoaderComplete(
-    const base::FilePath& response_path) {
+    base::FilePath response_path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   const bool error = response_path.empty();
@@ -158,9 +158,6 @@ void CustomizationWallpaperDownloader::OnSimpleLoaderComplete(
   VLOG(1) << "CustomizationWallpaperDownloader::OnURLFetchComplete(): status="
           << simple_loader_->NetError();
 
-  // Save the response_path before resetting SimplerURLLoader. It gets nulled
-  // out afterwards.
-  base::FilePath copy_response_path(response_path);
   simple_loader_.reset();
 
   if (error) {
@@ -171,7 +168,7 @@ void CustomizationWallpaperDownloader::OnSimpleLoaderComplete(
   std::unique_ptr<bool> success(new bool(false));
 
   base::OnceClosure rename_closure = base::BindOnce(
-      &RenameTemporaryFile, copy_response_path, wallpaper_downloaded_file_,
+      &RenameTemporaryFile, response_path, wallpaper_downloaded_file_,
       base::Unretained(success.get()));
   base::OnceClosure on_rename_closure = base::BindOnce(
       &CustomizationWallpaperDownloader::OnTemporaryFileRenamed,

chrome/browser/chromeos/customization/customization_wallpaper_downloader.h

@@ -63,7 +63,7 @@ class CustomizationWallpaperDownloader {
   void Retry();
 
   // This is called when the download has finished.
-  void OnSimpleLoaderComplete(const base::FilePath& file_path);
+  void OnSimpleLoaderComplete(base::FilePath file_path);
 
   // Called on UI thread.
   void OnWallpaperDirectoryCreated(std::unique_ptr<bool> success);

chrome/browser/net/file_downloader.cc

@@ -65,8 +65,7 @@ FileDownloader::FileDownloader(
 
 FileDownloader::~FileDownloader() {}
 
-void FileDownloader::OnSimpleDownloadComplete(
-    const base::FilePath& response_path) {
+void FileDownloader::OnSimpleDownloadComplete(base::FilePath response_path) {
   if (response_path.empty()) {
     if (simple_url_loader_->ResponseInfo() &&
         simple_url_loader_->ResponseInfo()->headers) {

chrome/browser/net/file_downloader.h

@@ -50,7 +50,7 @@ class FileDownloader {
   static bool IsSuccess(Result result) { return result != FAILED; }
 
  private:
-  void OnSimpleDownloadComplete(const base::FilePath& response_path);
+  void OnSimpleDownloadComplete(base::FilePath response_path);
 
   void OnFileExistsCheckDone(bool exists);
 

services/network/public/cpp/simple_url_loader.cc

@@ -687,7 +687,7 @@ class SaveToFileBodyHandler : public BodyHandler {
     // destructor.
     FileWriter::Destroy(std::move(file_writer_));
 
-    std::move(download_to_file_complete_callback_).Run(path_);
+    std::move(download_to_file_complete_callback_).Run(std::move(path_));
   }
 
   void PrepareToRetry(base::OnceClosure retry_callback) override {

services/network/public/cpp/simple_url_loader.h

@@ -43,15 +43,13 @@ class SimpleURLLoaderStreamConsumer;
 // complexity of the API.
 //
 // Deleting a SimpleURLLoader before it completes cancels the requests and frees
-// any resources it is using (including any partially downloaded files).
+// any resources it is using (including any partially downloaded files). A
+// SimpleURLLoader may be safely deleted while it's invoking any callback method
+// that was passed it.
 //
 // Each SimpleURLLoader can only be used for a single request.
 //
 // TODO(mmenke): Support the following:
-// * Consumer-provided methods to receive streaming (with backpressure).
-// * Uploads (Fixed strings, files, data streams (with backpressure), chunked
-// uploads). ResourceRequest may already have some support, but should make it
-// simple.
 // * Maybe some sort of retry backoff or delay?  ServiceURLLoaderContext enables
 // throttling for its URLFetchers.  Could additionally/alternatively support
 // 503 + Retry-After.
@@ -81,14 +79,16 @@ class COMPONENT_EXPORT(NETWORK_CPP) SimpleURLLoader {
   static const size_t kMaxUploadStringSizeToCopy;
 
   // Callback used when downloading the response body as a std::string.
-  // |response_body| is the body of the response, or nullptr on failure.
+  // |response_body| is the body of the response, or nullptr on failure. It is
+  // safe to delete the SimpleURLLoader during the callback.
   using BodyAsStringCallback =
       base::OnceCallback<void(std::unique_ptr<std::string> response_body)>;
 
   // Callback used when download the response body to a file. On failure, |path|
-  // will be empty.
+  // will be empty. It is safe to delete the SimpleURLLoader during the
+  // callback.
   using DownloadToFileCompleteCallback =
-      base::OnceCallback<void(const base::FilePath& path)>;
+      base::OnceCallback<void(base::FilePath path)>;
 
   // Callback used when a redirect is being followed. It is safe to delete the
   // SimpleURLLoader during the callback.

services/network/public/cpp/simple_url_loader_stream_consumer.h

@@ -15,8 +15,8 @@ namespace network {
 // Interface to handle streaming data from SimpleURLLoader. All methods are
 // invoked on the sequence the SimpleURLLoader was started on, and all callbacks
 // must be invoked on the same sequence. The SimpleURLLoader may be deleted at
-// any time. None of these methods will be called during SimpleURLLoader
-// destruction.
+// any time, including in any of the callbacks it invokes. None of these methods
+// will be called during SimpleURLLoader destruction.
 class COMPONENT_EXPORT(NETWORK_CPP) SimpleURLLoaderStreamConsumer {
  public:
   // Called as body data is received.

services/network/public/cpp/simple_url_loader_unittest.cc

@@ -227,6 +227,14 @@ class SimpleLoaderTestHelper : public SimpleURLLoaderStreamConsumer {
     download_to_stream_destroy_on_retry_ = download_to_stream_destroy_on_retry;
   }
 
+  // Sets whether the SimpleURLLoader should be destroyed when invoking the
+  // completion callback. When enabled, it will be destroyed before touching the
+  // completion data, to make sure it's still available after the destruction of
+  // the SimpleURLLoader.
+  void set_destroy_loader_on_complete(bool destroy_loader_on_complete) {
+    destroy_loader_on_complete_ = destroy_loader_on_complete;
+  }
+
   // Received response body, if any. Returns nullptr if no body was received
   // (Which is different from a 0-length body). For DownloadType::TO_STRING,
   // this is just the value passed to the callback. For DownloadType::TO_FILE,
@@ -281,19 +289,25 @@ class SimpleLoaderTestHelper : public SimpleURLLoaderStreamConsumer {
     EXPECT_EQ(DownloadType::TO_STRING, download_type_);
     EXPECT_FALSE(response_body_);
 
+    if (destroy_loader_on_complete_)
+      simple_url_loader_.reset();
+
     response_body_ = std::move(response_body);
 
     done_ = true;
     run_loop_.Quit();
   }
 
-  void DownloadedToFile(const base::FilePath& file_path) {
+  void DownloadedToFile(base::FilePath file_path) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     EXPECT_FALSE(done_);
     EXPECT_TRUE(download_type_ == DownloadType::TO_FILE ||
                 download_type_ == DownloadType::TO_TEMP_FILE);
     EXPECT_FALSE(response_body_);
 
+    if (destroy_loader_on_complete_)
+      simple_url_loader_.reset();
+
     base::ScopedAllowBlockingForTesting allow_blocking;
 
     if (!file_path.empty()) {
@@ -373,6 +387,9 @@ class SimpleLoaderTestHelper : public SimpleURLLoaderStreamConsumer {
           std::make_unique<std::string>(download_as_stream_response_body_);
     }
 
+    if (destroy_loader_on_complete_)
+      simple_url_loader_.reset();
+
     done_ = true;
     run_loop_.Quit();
   }
@@ -423,6 +440,8 @@ class SimpleLoaderTestHelper : public SimpleURLLoaderStreamConsumer {
   bool download_to_stream_async_retry_ = false;
   bool download_to_stream_destroy_on_retry_ = false;
 
+  bool destroy_loader_on_complete_ = false;
+
   bool allow_http_error_results_ = false;
 
   base::ScopedTempDir temp_dir_;
@@ -839,6 +858,23 @@ TEST_P(SimpleURLLoaderTest, DeleteInOnResponseStartedCallback) {
   run_loop.Run();
 }
 
+// Check the case where the SimpleURLLoader is deleted in the completion
+// callback.
+TEST_P(SimpleURLLoaderTest, DestroyLoaderInOnComplete) {
+  std::unique_ptr<network::ResourceRequest> resource_request =
+      std::make_unique<network::ResourceRequest>();
+  // Use a more interesting request than "/echo", just to verify more than the
+  // request URL is hooked up.
+  resource_request->url = test_server_.GetURL("/echoheader?foo");
+  resource_request->headers.SetHeader("foo", "Expected Response");
+  std::unique_ptr<SimpleLoaderTestHelper> test_helper =
+      CreateHelper(std::move(resource_request));
+  test_helper->set_destroy_loader_on_complete(true);
+  test_helper->StartSimpleLoaderAndWait(url_loader_factory_.get());
+  ASSERT_TRUE(test_helper->response_body());
+  EXPECT_EQ("Expected Response", *test_helper->response_body());
+}
+
 // Check the case where a URLLoaderFactory with a closed Mojo pipe was passed
 // in.
 TEST_P(SimpleURLLoaderTest, DisconnectedURLLoader) {