diff --git a/chrome/updater/app/app_net_worker_unittest.cc b/chrome/updater/app/app_net_worker_unittest.cc index a25b498a0c7c1..0e01480aba7d3 100644 --- a/chrome/updater/app/app_net_worker_unittest.cc +++ b/chrome/updater/app/app_net_worker_unittest.cc @@ -8,9 +8,12 @@ #include #include #include +#include #include #include +#include "base/containers/span.h" +#include "base/files/file.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_file.h" #include "base/functional/bind.h" @@ -184,6 +187,52 @@ TEST_F(AppNetWorkerTest, DownloadFile) { EXPECT_TRUE(base::ContentsEqual(output.path(), payload_path)); } +TEST_F(AppNetWorkerTest, DownloadedFileIsFreshCopy) { + base::FilePath payload_path = updater::test::GetTestFilePath("signed.exe.gz"); + std::string payload; + ASSERT_TRUE(base::ReadFileToString(payload_path, &payload)); + net::EmbeddedTestServer test_server; + test_server.RegisterRequestHandler(base::BindLambdaForTesting( + [&](const net::test_server::HttpRequest& request) + -> std::unique_ptr { + auto http_response = + std::make_unique(); + http_response->set_code(net::HTTP_OK); + http_response->set_content(payload); + http_response->set_content_type("application/octet-stream"); + return http_response; + })); + ASSERT_TRUE(test_server.Start()); + + base::ScopedTempFile output; + ASSERT_TRUE(output.Create()); + base::File output_file(output.path(), + base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_WRITE); + ASSERT_TRUE(output_file.IsValid()); + // Retain a writable handle equivalent to the one sent to the worker. + base::File retained = output_file.Duplicate(); + ASSERT_TRUE(retained.IsValid()); + + base::RunLoop run_loop; + remote_->DownloadToFile( + test_server.GetURL("/"), std::move(output_file), + MakeFileDownloadObserver( + output.path(), base::DoNothing(), base::DoNothing(), + base::BindLambdaForTesting( + [&](int32_t net_error, int64_t content_length) { + EXPECT_EQ(net_error, 0); + run_loop.Quit(); + }))); + run_loop.Run(); + ASSERT_TRUE(base::ContentsEqual(output.path(), payload_path)); + + // Writes via the descriptor that was sent to the worker must not affect the + // file at `output.path()`. + static constexpr std::string_view kOverwrite = "overwritten"; + ASSERT_TRUE(retained.WriteAndCheck(0, base::as_byte_span(kOverwrite))); + EXPECT_TRUE(base::ContentsEqual(output.path(), payload_path)); +} + TEST_F(AppNetWorkerTest, DownloadMultipleFiles) { base::FilePath payload_path = updater::test::GetTestFilePath("signed.exe.gz"); std::optional payload_size = base::GetFileSize(payload_path); diff --git a/chrome/updater/net/fetcher_callback_adapter.cc b/chrome/updater/net/fetcher_callback_adapter.cc index a27764ceef68a..4ac603c621832 100644 --- a/chrome/updater/net/fetcher_callback_adapter.cc +++ b/chrome/updater/net/fetcher_callback_adapter.cc @@ -11,10 +11,16 @@ #include #include "base/check.h" +#include "base/files/file_path.h" +#include "base/files/file_util.h" #include "base/functional/bind.h" #include "base/functional/callback.h" +#include "base/location.h" #include "base/process/process.h" #include "base/task/bind_post_task.h" +#include "base/task/task_traits.h" +#include "base/task/thread_pool.h" +#include "chrome/updater/constants.h" #include "chrome/updater/net/mac/mojom/updater_fetcher.mojom.h" #include "chrome/updater/util/util.h" #include "components/update_client/network.h" @@ -118,6 +124,23 @@ class FileDownloadObserverImpl : public mojom::FileDownloadObserver { DownloadToFileCompleteCallback download_complete_callback_; }; +int ReplaceFileWithCopy(const base::FilePath& file_path, int net_error) { + if (net_error != 0) { + return net_error; + } + base::FilePath temp; + if (!base::CreateTemporaryFileInDir(file_path.DirName(), &temp) || + !base::CopyFile(file_path, temp)) { + base::DeleteFile(temp); + return kErrorFailedToWriteFile; + } + if (!base::ReplaceFile(temp, file_path, /*error=*/nullptr)) { + base::DeleteFile(temp); + return kErrorFailedToWriteFile; + } + return net_error; +} + } // namespace // Binds a callback which creates a self-owned PostRequestObserverImpl to @@ -166,4 +189,29 @@ MakeFileDownloadObserver( std::move(download_complete_callback))); } +base::OnceCallback)> +MakeFileDownloadObserver( + const base::FilePath& file_path, + ResponseStartedCallback response_started_callback, + ProgressCallback progress_callback, + DownloadToFileCompleteCallback download_complete_callback) { + return MakeFileDownloadObserver( + response_started_callback, progress_callback, + base::BindOnce( + [](const base::FilePath& file_path, + DownloadToFileCompleteCallback callback, int net_error, + int64_t content_size) { + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, {base::MayBlock()}, + base::BindOnce(&ReplaceFileWithCopy, file_path, net_error), + base::BindOnce( + [](DownloadToFileCompleteCallback callback, + int64_t content_size, int net_error) { + std::move(callback).Run(net_error, content_size); + }, + std::move(callback), content_size)); + }, + file_path, std::move(download_complete_callback))); +} + } // namespace updater diff --git a/chrome/updater/net/fetcher_callback_adapter.h b/chrome/updater/net/fetcher_callback_adapter.h index b542c87da7a52..63ea6ad9719cc 100644 --- a/chrome/updater/net/fetcher_callback_adapter.h +++ b/chrome/updater/net/fetcher_callback_adapter.h @@ -10,6 +10,10 @@ #include "components/update_client/network.h" #include "mojo/public/cpp/bindings/pending_receiver.h" +namespace base { +class FilePath; +} + namespace updater { base::OnceCallback)> @@ -28,6 +32,19 @@ MakeFileDownloadObserver( update_client::NetworkFetcher::DownloadToFileCompleteCallback download_complete_callback); +// Same as above, but additionally replaces `file_path` with a freshly-created +// copy of itself before running `download_complete_callback`. After the +// callback runs, the file backing `file_path` is no longer referenced by any +// descriptor that was sent to the remote process. +base::OnceCallback)> +MakeFileDownloadObserver( + const base::FilePath& file_path, + update_client::NetworkFetcher::ResponseStartedCallback + response_started_callback, + update_client::NetworkFetcher::ProgressCallback progress_callback, + update_client::NetworkFetcher::DownloadToFileCompleteCallback + download_complete_callback); + } // namespace updater #endif // CHROME_UPDATER_NET_FETCHER_CALLBACK_ADAPTER_H_ diff --git a/chrome/updater/net/network_fetcher_mac.mm b/chrome/updater/net/network_fetcher_mac.mm index dd09ec0656521..d6bbf423d01d9 100644 --- a/chrome/updater/net/network_fetcher_mac.mm +++ b/chrome/updater/net/network_fetcher_mac.mm @@ -678,6 +678,7 @@ class OutOfProcessNetworkFetcher : public update_client::NetworkFetcher { void DoDownloadFile( const GURL& url, + const base::FilePath& file_path, update_client::NetworkFetcher::ResponseStartedCallback response_started_callback, update_client::NetworkFetcher::ProgressCallback progress_callback, @@ -827,7 +828,7 @@ base::OnceClosure OutOfProcessNetworkFetcher::DownloadToFile( }, file_path), base::BindOnce(&OutOfProcessNetworkFetcher::DoDownloadFile, - base::Unretained(this), url, + base::Unretained(this), url, file_path, wrapped_response_started_callback, progress_callback, std::move(wrapped_download_to_file_complete_callback))); return base::DoNothing(); @@ -835,6 +836,7 @@ base::OnceClosure OutOfProcessNetworkFetcher::DownloadToFile( void OutOfProcessNetworkFetcher::DoDownloadFile( const GURL& url, + const base::FilePath& file_path, update_client::NetworkFetcher::ResponseStartedCallback response_started_callback, update_client::NetworkFetcher::ProgressCallback progress_callback, @@ -859,7 +861,7 @@ void OutOfProcessNetworkFetcher::DoDownloadFile( remote_->DownloadToFile( url, std::move(output), MakeFileDownloadObserver( - response_started_callback, progress_callback, + file_path, response_started_callback, progress_callback, mojo::WrapCallbackWithDefaultInvokeIfNotRun( std::move(download_complete_callback), kErrorIpcDisconnect, -1))); }