Stop leaking files extracted by mini_installer.

With this CL, mini_installer now holds the files it creates open with
FLAG_DELETE_ON_CLOSE so that they are automatically deleted by the
filesystem when they are no longer needed.

The most risky operation is the multi-step process to drop write
permission on the files so that they can be opened for consumption by
parties that do not allow writers. New process exits codes have been
introduced to track whether or not this operation fails in practice.

BUG=516207

Change-Id: Ic5e25692cf3dca0fcc7cd01faf5759648f5c6890
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2307250Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790735}

chrome/installer/mini_installer/BUILD.gn

@@ -76,6 +76,7 @@ source_set("unit_tests") {
     "mini_file_test.cc",
     "mini_installer_unittest.cc",
     "mini_string_test.cc",
+    "pe_resource_test.cc",
   ]
 
   public_deps = [ ":lib" ]

chrome/installer/mini_installer/decompress.cc

@@ -22,7 +22,7 @@ struct ExpandContext {
   const wchar_t* const dest_path;
 
   // The destination file; valid once the destination is created.
-  mini_installer::MiniFile dest_file;
+  mini_installer::MiniFile& dest_file;
 
   // Set to true if the file was extracted to |dest_path|. Note that |dest_file|
   // may be valid even in case of failure.
@@ -247,7 +247,7 @@ bool InitializeFdi() {
 
 namespace mini_installer {
 
-bool Expand(const wchar_t* source, const wchar_t* destination) {
+bool Expand(const wchar_t* source, const wchar_t* destination, MiniFile& file) {
   if (!InitializeFdi())
     return false;
 
@@ -274,7 +274,7 @@ bool Expand(const wchar_t* source, const wchar_t* destination) {
   if (!fdi)
     return false;
 
-  ExpandContext context = {destination, {}, /*succeeded=*/false};
+  ExpandContext context = {destination, file, /*succeeded=*/false};
   g_FDICopy(fdi, source_name_utf8, source_path_utf8, 0, &Notify, nullptr,
             &context);
   g_FDIDestroy(fdi);
@@ -282,8 +282,7 @@ bool Expand(const wchar_t* source, const wchar_t* destination) {
     return true;
 
   // Delete the output file if it was created.
-  if (context.dest_file.IsValid())
-    context.dest_file.DeleteOnClose();
+  file.Close();
 
   return false;
 }

chrome/installer/mini_installer/decompress.h

@@ -7,11 +7,14 @@
 
 namespace mini_installer {
 
-// Same as the tool, expand.exe.  Decompresses a file that was compressed
-// using Microsoft's MSCF compression algorithm.
-// |source| is the full path of the file to decompress and |destination|
-// is the full path of the target file.
-bool Expand(const wchar_t* source, const wchar_t* destination);
+class MiniFile;
+
+// Expands the first file in |source| to the file |destination| using
+// Microsoft's MSCF compression algorithm (a la expand.exe). Returns true on
+// success, in which case |file| holds an open handle to the destination file.
+// |file| will be opened with exclusive write access and shared read and delete
+// access, and will be marked as delete-on-close.
+bool Expand(const wchar_t* source, const wchar_t* destination, MiniFile& file);
 
 }  // namespace mini_installer
 

chrome/installer/mini_installer/decompress_test.cc

@@ -5,9 +5,11 @@
 #include <windows.h>
 
 #include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "chrome/installer/mini_installer/decompress.h"
+#include "chrome/installer/mini_installer/mini_file.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 TEST(MiniDecompressTest, ExpandTest) {
@@ -27,11 +29,21 @@ TEST(MiniDecompressTest, ExpandTest) {
       temp_dir.GetPath().Append(FILE_PATH_LITERAL("setup.exe")));
 
   // Decompress our test file.
-  EXPECT_TRUE(mini_installer::Expand(source_path.value().c_str(),
-                                     dest_path.value().c_str()));
+  mini_installer::MiniFile file(mini_installer::MiniFile::DeleteOnClose::kYes);
+  ASSERT_TRUE(mini_installer::Expand(source_path.value().c_str(),
+                                     dest_path.value().c_str(), file));
+  ASSERT_TRUE(file.IsValid());
+  ASSERT_PRED1(base::PathExists, dest_path);
+
+  // Drop write permission so that the call below can open the file.
+  ASSERT_TRUE(file.DropWritePermission());
 
   // Check if the expanded file is a valid executable.
   DWORD type = static_cast<DWORD>(-1);
   EXPECT_TRUE(GetBinaryType(dest_path.value().c_str(), &type));
   EXPECT_EQ(static_cast<DWORD>(SCS_32BIT_BINARY), type);
+
+  // Closing the handle should delete the file.
+  file.Close();
+  EXPECT_FALSE(base::PathExists(dest_path));
 }

chrome/installer/mini_installer/exit_code.h

@@ -42,6 +42,11 @@ enum ExitCode {
   RUN_SETUP_FAILED_FILE_NOT_FOUND = 122,            // ERROR_FILE_NOT_FOUND.
   RUN_SETUP_FAILED_PATH_NOT_FOUND = 123,            // ERROR_PATH_NOT_FOUND.
   RUN_SETUP_FAILED_COULD_NOT_CREATE_PROCESS = 124,  // All other errors.
+  UNABLE_TO_OPEN_PATCHED_SETUP = 125,
+  DROP_WRITE_ON_EXTRACTED_ARCHIVE_FAILED = 126,
+  DROP_WRITE_ON_EXTRACTED_SETUP_PATCH_FAILED = 127,
+  DROP_WRITE_ON_EXTRACTED_SETUP_FAILED = 128,
+  DROP_WRITE_ON_EXPANDED_SETUP_FAILED = 129,
 };
 
 }  // namespace mini_installer

chrome/installer/mini_installer/mini_file.cc

@@ -8,7 +8,10 @@
 
 namespace mini_installer {
 
-MiniFile::MiniFile() = default;
+MiniFile::MiniFile(DeleteOnClose delete_on_close)
+    : delete_on_close_flag_(delete_on_close != DeleteOnClose::kNo
+                                ? FILE_FLAG_DELETE_ON_CLOSE
+                                : 0) {}
 
 MiniFile::~MiniFile() {
   Close();
@@ -26,9 +29,11 @@ bool MiniFile::Create(const wchar_t* path) {
   Close();
   if (!path_.assign(path))
     return false;
-  handle_ =
-      ::CreateFileW(path_.get(), DELETE | GENERIC_WRITE, FILE_SHARE_DELETE,
-                    nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
+  handle_ = ::CreateFileW(path_.get(), GENERIC_WRITE,
+                          FILE_SHARE_DELETE | FILE_SHARE_READ,
+                          /*lpSecurityAttributes=*/nullptr, CREATE_ALWAYS,
+                          FILE_ATTRIBUTE_NORMAL | delete_on_close_flag_,
+                          /*hTemplateFile=*/nullptr);
   if (handle_ != INVALID_HANDLE_VALUE)
     return true;
   path_.clear();
@@ -39,11 +44,76 @@ bool MiniFile::IsValid() const {
   return handle_ != INVALID_HANDLE_VALUE;
 }
 
-bool MiniFile::DeleteOnClose() {
-  FILE_DISPOSITION_INFO disposition = {/*DeleteFile=*/TRUE};
-  return IsValid() &&
-         ::SetFileInformationByHandle(handle_, FileDispositionInfo,
-                                      &disposition, sizeof(disposition));
+bool MiniFile::DropWritePermission() {
+  // The original file was opened with write access (of course), so it will take
+  // a little hoop jumping to get a handle without it. First, get a new handle
+  // that doesn't have write access. This one must allow others to write on
+  // account of the fact that the original handle has write access.
+  HANDLE without_write =
+      ::ReOpenFile(handle_, GENERIC_READ,
+                   FILE_SHARE_DELETE | FILE_SHARE_WRITE | FILE_SHARE_READ,
+                   delete_on_close_flag_);
+  if (without_write == INVALID_HANDLE_VALUE) {
+    const auto error = ::GetLastError();
+    Close();
+    ::SetLastError(error);
+    return false;
+  }
+
+  // Next, close the original handle so that there are no longer any writers.
+  // This will mark the file for deletion if the original handle was opened with
+  // FILE_FLAG_DELETE_ON_CLOSE.
+  ::CloseHandle(std::exchange(handle_, INVALID_HANDLE_VALUE));
+
+  // Now unmark the file for deletion if needed.
+  if (delete_on_close_flag_) {
+    FILE_DISPOSITION_INFO disposition = {/*DeleteFile=*/FALSE};
+    if (!::SetFileInformationByHandle(without_write, FileDispositionInfo,
+                                      &disposition, sizeof(disposition))) {
+      const auto error = ::GetLastError();
+      ::CloseHandle(std::exchange(without_write, INVALID_HANDLE_VALUE));
+      Close();
+      ::SetLastError(error);
+      return false;
+    }
+  }
+
+  // Now open a read-only handle (with FILE_FLAG_DELETE_ON_CLOSE as needed) that
+  // doesn't allow others to write. Note that there is a potential race here:
+  // another party could open the file for shared write access at this precise
+  // moment, causing this ReOpenFile to fail. This would likely be an issue
+  // anyway, as one common thing to do with the file is to execute it, which
+  // will fail if there are writers.
+  handle_ =
+      ::ReOpenFile(without_write, GENERIC_READ,
+                   FILE_SHARE_DELETE | FILE_SHARE_READ, delete_on_close_flag_);
+  if (handle_ == INVALID_HANDLE_VALUE) {
+    const auto error = ::GetLastError();
+    ::CloseHandle(std::exchange(without_write, INVALID_HANDLE_VALUE));
+    Close();
+    ::SetLastError(error);
+    return false;
+  }
+
+  // Closing the handle that allowed shared writes may once again mark the file
+  // for deletion.
+  ::CloseHandle(std::exchange(without_write, INVALID_HANDLE_VALUE));
+
+  // Everything went according to plan; |handle_| is now lacking write access
+  // and does not allow other writers. The last step is to unmark the file for
+  // deletion once again, as the closure of |without_write| has re-marked it.
+  if (delete_on_close_flag_) {
+    FILE_DISPOSITION_INFO disposition = {/*DeleteFile=*/FALSE};
+    if (!::SetFileInformationByHandle(handle_, FileDispositionInfo,
+                                      &disposition, sizeof(disposition))) {
+      const auto error = ::GetLastError();
+      Close();
+      ::SetLastError(error);
+      return false;
+    }
+  }
+
+  return true;
 }
 
 void MiniFile::Close() {
@@ -64,6 +134,18 @@ HANDLE MiniFile::DuplicateHandle() const {
              : INVALID_HANDLE_VALUE;
 }
 
+bool MiniFile::Open(const PathString& path) {
+  Close();
+  handle_ = ::CreateFileW(path.get(), GENERIC_READ,
+                          FILE_SHARE_DELETE | FILE_SHARE_READ,
+                          /*lpSecurityAttributes=*/nullptr, OPEN_EXISTING,
+                          delete_on_close_flag_, /*hTemplateFile=*/nullptr);
+  if (handle_ == INVALID_HANDLE_VALUE)
+    return false;
+  path_.assign(path);
+  return true;
+}
+
 HANDLE MiniFile::GetHandleUnsafe() const {
   return handle_;
 }

chrome/installer/mini_installer/mini_file.h

@@ -14,10 +14,11 @@ namespace mini_installer {
 // A simple abstraction over a path to a file and a Windows file handle to it.
 class MiniFile {
  public:
-  MiniFile();
+  enum class DeleteOnClose : bool { kNo = false, kYes = true };
+  explicit MiniFile(DeleteOnClose delete_on_close);
 
   // Closes the file if the instance holds a valid handle. The file will be
-  // deleted if directed by a call to DeleteOnClose().
+  // deleted if the instance was constructed with |delete_on_close|.
   ~MiniFile();
 
   MiniFile(const MiniFile&) = delete;
@@ -35,17 +36,28 @@ class MiniFile {
   // Returns true if this object has a path and a handle to an open file.
   bool IsValid() const;
 
-  // Marks the file for deletion when the handle is closed via Close() or the
-  // instance's destructor. This state follows the handle when moved.
-  bool DeleteOnClose();
-
-  // Closes the handle and clears the path. Following this, IsValid() will
-  // return false.
+  // Drops write permission on the file handle so that other parties that
+  // require no writers may open the file. In particular, the Windows loader
+  // opens files for execution with shared read/delete access, as do the
+  // extraction operations in Chrome's mini_installer.exe and setup.exe. These
+  // would fail with sharing violations if mini_installer were to hold files
+  // open with write permissions. Returns false on failure, in which case the
+  // instance is no longer valid.  The file will have been deleted if the
+  // instance was created with DeleteOnClose.
+  bool DropWritePermission();
+
+  // Closes the handle and clears the path. The file will be deleted if the
+  // instance was constructed with |delete_on_close|. Following this, IsValid()
+  // will return false.
   void Close();
 
   // Returns a new handle to the file, or INVALID_HANDLE_VALUE on error.
   HANDLE DuplicateHandle() const;
 
+  // Opens the file for read access, disallowing writers (as if Create followed
+  // by DropWritePermission).
+  bool Open(const PathString& path);
+
   // Returns the path to the open file, or a pointer to an empty string if
   // IsValid() is false.
   const wchar_t* path() const { return path_.get(); }
@@ -61,6 +73,10 @@ class MiniFile {
 
   // A handle to the open file, or INVALID_HANDLE_VALUE.
   HANDLE handle_ = INVALID_HANDLE_VALUE;
+
+  // Zero or FILE_FLAG_DELETE_ON_CLOSE, according to how the instance was
+  // constructed.
+  const DWORD delete_on_close_flag_;
 };
 
 }  // namespace mini_installer

chrome/installer/mini_installer/mini_file_test.cc

@@ -8,16 +8,25 @@
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "chrome/installer/mini_installer/path_string.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace mini_installer {
 
-class MiniFileTest : public ::testing::Test {
+// A test harness for MiniFile. The MiniFile::DeleteOnClose parameter is passed
+// to the test instance's constructor.
+class MiniFileTest : public ::testing::TestWithParam<MiniFile::DeleteOnClose> {
  protected:
   void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); }
 
   void TearDown() override { EXPECT_TRUE(temp_dir_.Delete()); }
 
+  static MiniFile::DeleteOnClose delete_on_close() { return GetParam(); }
+
+  static bool should_delete_on_close() {
+    return delete_on_close() != MiniFile::DeleteOnClose::kNo;
+  }
+
   const base::FilePath& temp_dir() const { return temp_dir_.GetPath(); }
 
  private:
@@ -25,68 +34,73 @@ class MiniFileTest : public ::testing::Test {
 };
 
 // Create should create a file.
-TEST_F(MiniFileTest, Create) {
+TEST_P(MiniFileTest, Create) {
   const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
 
-  MiniFile file;
+  MiniFile file(delete_on_close());
   ASSERT_TRUE(file.Create(file_path.value().c_str()));
-  ASSERT_TRUE(base::PathExists(file_path));
+  EXPECT_TRUE(base::PathExists(file_path));
 }
 
 // Created files should be deletable by others and should vanish when closed.
-TEST_F(MiniFileTest, CreateDeleteIsShared) {
+TEST_P(MiniFileTest, CreateDeleteIsShared) {
   const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
 
-  MiniFile file;
+  MiniFile file(delete_on_close());
   ASSERT_TRUE(file.Create(file_path.value().c_str()));
   // DeleteFile uses POSIX semantics, so the file appears to vanish immediately.
   ASSERT_TRUE(base::DeleteFile(file_path));
   file.Close();
-  ASSERT_FALSE(base::PathExists(file_path));
+  EXPECT_FALSE(base::PathExists(file_path));
 }
 
-// DeleteOnClose should work as advertised.
-TEST_F(MiniFileTest, DeleteOnClose) {
+// Tests that a file can be opened without shared write access after write
+// permissions are dropped.
+TEST_P(MiniFileTest, DropWritePermission) {
   const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
 
-  MiniFile file;
+  MiniFile file(delete_on_close());
   ASSERT_TRUE(file.Create(file_path.value().c_str()));
-  ASSERT_TRUE(file.DeleteOnClose());
-
-  // The file can no longer be opened now that it has been marked for deletion.
-  // Attempts to do so will fail with ERROR_ACCESS_DENIED. Under the covers, the
-  // NT status code is STATUS_DELETE_PENDING. Since base::PathExists will return
-  // false in this case, confirm the file's existence by trying to open it.
-  base::File the_file(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ |
-                                     base::File::FLAG_SHARE_DELETE);
-  ASSERT_FALSE(the_file.IsValid());
-  ASSERT_EQ(the_file.error_details(), base::File::FILE_ERROR_ACCESS_DENIED);
-
-  file.Close();
-  ASSERT_FALSE(base::PathExists(file_path));
+  ASSERT_FALSE(base::File(file_path, base::File::FLAG_OPEN |
+                                         base::File::FLAG_READ |
+                                         base::File::FLAG_EXCLUSIVE_WRITE |
+                                         base::File::FLAG_SHARE_DELETE)
+                   .IsValid());
+  ASSERT_TRUE(file.DropWritePermission());
+  EXPECT_TRUE(base::File(file_path, base::File::FLAG_OPEN |
+                                        base::File::FLAG_READ |
+                                        base::File::FLAG_EXCLUSIVE_WRITE |
+                                        base::File::FLAG_SHARE_DELETE)
+                  .IsValid());
 }
 
 // Close should really close.
-TEST_F(MiniFileTest, Close) {
+TEST_P(MiniFileTest, CreateThenClose) {
   const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
 
-  MiniFile file;
+  MiniFile file(delete_on_close());
   ASSERT_TRUE(file.Create(file_path.value().c_str()));
   file.Close();
   EXPECT_FALSE(file.IsValid());
   EXPECT_EQ(*file.path(), 0);
-  ASSERT_TRUE(base::PathExists(file_path));
-  base::File f(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ |
-                              base::File::FLAG_EXCLUSIVE_READ |
-                              base::File::FLAG_EXCLUSIVE_WRITE);
-  ASSERT_TRUE(f.IsValid());
+
+  ASSERT_NE(base::PathExists(file_path), should_delete_on_close());
+  if (!should_delete_on_close()) {
+    // If closing should not have deleted the file, it should now be possible to
+    // open it with exclusive access.
+    EXPECT_TRUE(base::File(file_path, base::File::FLAG_OPEN |
+                                          base::File::FLAG_READ |
+                                          base::File::FLAG_EXCLUSIVE_READ |
+                                          base::File::FLAG_EXCLUSIVE_WRITE)
+                    .IsValid());
+  }
 }
 
 // DuplicateHandle should work as advertized.
-TEST_F(MiniFileTest, DuplicateHandle) {
+TEST_P(MiniFileTest, DuplicateHandle) {
   const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
 
-  MiniFile file;
+  MiniFile file(delete_on_close());
   ASSERT_TRUE(file.Create(file_path.value().c_str()));
   HANDLE dup = file.DuplicateHandle();
   ASSERT_NE(dup, INVALID_HANDLE_VALUE);
@@ -104,10 +118,55 @@ TEST_F(MiniFileTest, DuplicateHandle) {
   ::CloseHandle(std::exchange(dup, INVALID_HANDLE_VALUE));
 }
 
-TEST_F(MiniFileTest, Path) {
+// Open should provide access to a file just like Create, but for a file that
+// already exists.
+TEST_P(MiniFileTest, Open) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  ASSERT_TRUE(
+      base::File(file_path, base::File::FLAG_CREATE | base::File::FLAG_WRITE)
+          .IsValid());
+  ASSERT_TRUE(base::PathExists(file_path));
+
+  PathString path_string;
+  ASSERT_TRUE(path_string.assign(file_path.value().c_str()));
+  MiniFile file(delete_on_close());
+  ASSERT_TRUE(file.Open(path_string));
+}
+
+// Close should really close.
+TEST_P(MiniFileTest, OpenThenClose) {
   const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
 
-  MiniFile file;
+  ASSERT_TRUE(
+      base::File(file_path, base::File::FLAG_CREATE | base::File::FLAG_WRITE)
+          .IsValid());
+  ASSERT_TRUE(base::PathExists(file_path));
+
+  MiniFile file(delete_on_close());
+  PathString path_string;
+  ASSERT_TRUE(path_string.assign(file_path.value().c_str()));
+  ASSERT_TRUE(file.Open(path_string));
+  file.Close();
+  EXPECT_FALSE(file.IsValid());
+  EXPECT_EQ(*file.path(), 0);
+
+  ASSERT_NE(base::PathExists(file_path), should_delete_on_close());
+  if (!should_delete_on_close()) {
+    // If closing should not have deleted the file, it should now be possible to
+    // open it with exclusive access.
+    EXPECT_TRUE(base::File(file_path, base::File::FLAG_OPEN |
+                                          base::File::FLAG_READ |
+                                          base::File::FLAG_EXCLUSIVE_READ |
+                                          base::File::FLAG_EXCLUSIVE_WRITE)
+                    .IsValid());
+  }
+}
+
+TEST_P(MiniFileTest, Path) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file(delete_on_close());
   EXPECT_EQ(*file.path(), 0);
 
   ASSERT_TRUE(file.Create(file_path.value().c_str()));
@@ -117,10 +176,10 @@ TEST_F(MiniFileTest, Path) {
   EXPECT_EQ(*file.path(), 0);
 }
 
-TEST_F(MiniFileTest, GetHandleUnsafe) {
+TEST_P(MiniFileTest, GetHandleUnsafe) {
   const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
 
-  MiniFile file;
+  MiniFile file(delete_on_close());
   EXPECT_EQ(file.GetHandleUnsafe(), INVALID_HANDLE_VALUE);
 
   ASSERT_TRUE(file.Create(file_path.value().c_str()));
@@ -130,4 +189,11 @@ TEST_F(MiniFileTest, GetHandleUnsafe) {
   EXPECT_EQ(file.GetHandleUnsafe(), INVALID_HANDLE_VALUE);
 }
 
+INSTANTIATE_TEST_SUITE_P(DoNotDeleteOnClose,
+                         MiniFileTest,
+                         ::testing::Values(MiniFile::DeleteOnClose::kNo));
+INSTANTIATE_TEST_SUITE_P(DeleteOnClose,
+                         MiniFileTest,
+                         ::testing::Values(MiniFile::DeleteOnClose::kYes));
+
 }  // namespace mini_installer

chrome/installer/mini_installer/mini_installer.cc

@@ -36,32 +36,41 @@
 #include <stdlib.h>
 
 #include <initializer_list>
+#include <utility>
 
 #include "build/branding_buildflags.h"
 #include "chrome/installer/mini_installer/appid.h"
 #include "chrome/installer/mini_installer/configuration.h"
 #include "chrome/installer/mini_installer/decompress.h"
+#include "chrome/installer/mini_installer/mini_file.h"
 #include "chrome/installer/mini_installer/mini_installer_constants.h"
 #include "chrome/installer/mini_installer/pe_resource.h"
 #include "chrome/installer/mini_installer/regkey.h"
 
 namespace mini_installer {
 
-typedef StackString<MAX_PATH> PathString;
+namespace {
 
 // This structure passes data back and forth for the processing
 // of resource callbacks.
 struct Context {
   // Input to the call back method. Specifies the dir to save resources.
   const wchar_t* base_path;
-  // First output from call back method. Full path of Chrome archive.
-  PathString* chrome_resource_path;
-  // Second output from call back method. Full path of Setup archive/exe.
-  PathString* setup_resource_path;
+  // First output from call back method; the Chrome archive.
+  MiniFile& archive;
+  // Second output from call back method; the Chrome installer.
+  MiniFile& setup;
   // A Windows error code corresponding to an extraction error.
   DWORD error_code;
 };
 
+MiniFile::DeleteOnClose GetDeleteOnCloseOption(
+    const Configuration& configuration) {
+  return configuration.should_delete_extracted_files()
+             ? MiniFile::DeleteOnClose::kYes
+             : MiniFile::DeleteOnClose::kNo;
+}
+
 // TODO(grt): Frame this in terms of whether or not the brand supports
 // integration with Omaha, where Google Update is the Google-specific fork of
 // the open-source Omaha project.
@@ -169,6 +178,8 @@ ProcessExitResult GetSetupExePathForAppGuid(bool system_level,
   return ProcessExitResult(SUCCESS_EXIT_CODE);
 }
 
+}  // namespace
+
 // Gets the path to setup.exe of the previous version. The overall path is found
 // in the Uninstall string in the registry. A previous version number specified
 // in |configuration| is used if available. |size| is measured in wchar_t units.
@@ -182,6 +193,8 @@ ProcessExitResult GetPreviousSetupExePath(const Configuration& configuration,
       configuration.previous_version(), path, size);
 }
 
+namespace {
+
 // Calls CreateProcess with good default parameters and waits for the process to
 // terminate returning the process exit code. In case of CreateProcess failure,
 // returns a results object with the provided codes as follows:
@@ -235,6 +248,8 @@ ProcessExitResult RunProcessAndWait(const wchar_t* exe_path,
   return ProcessExitResult(exit_code);
 }
 
+}  // namespace
+
 void AppendCommandLineFlags(const wchar_t* command_line,
                             CommandString* buffer) {
   // The program name (the first argument parsed by CommandLineToArgvW) is
@@ -277,13 +292,15 @@ void AppendCommandLineFlags(const wchar_t* command_line,
   buffer->append(command_line);
 }
 
+namespace {
+
 // Processes a resource of type |type| in |module| on behalf of a call to
 // EnumResourceNames. On each call, |name| contains the name of a resource. A
 // TRUE return value continues the enumeration, whereas FALSE stops it. This
 // function extracts the first resource starting with "chrome" and/or "setup",
 // populating |context| (which must be a pointer to a Context struct) with the
-// path(s) of the extracted file(s). Enumeration stops early in case of error,
-// which includes any unexpected resources or duplicate matching resources.
+// the extracted file(s). Enumeration stops early in case of error, which
+// includes any unexpected resources or duplicate matching resources.
 // |context|'s |error_code| member may be populated with a Windows error code
 // corresponding to an error condition.
 BOOL CALLBACK OnResourceFound(HMODULE module,
@@ -306,20 +323,16 @@ BOOL CALLBACK OnResourceFound(HMODULE module,
   if (!full_path.assign(context.base_path) || !full_path.append(name))
     return FALSE;  // Break: failed to form the output path.
 
-  if (StrStartsWith(name, kChromeArchivePrefix) &&
-      context.chrome_resource_path->empty()) {
-    if (!resource.WriteToDisk(full_path.get())) {
+  if (StrStartsWith(name, kChromeArchivePrefix) && !context.archive.IsValid()) {
+    if (!resource.WriteToDisk(full_path.get(), context.archive)) {
       context.error_code = ::GetLastError();
       return FALSE;  // Break: failed to write resource.
     }
-    context.chrome_resource_path->assign(full_path);
-  } else if (StrStartsWith(name, kSetupPrefix) &&
-             context.setup_resource_path->empty()) {
-    if (!resource.WriteToDisk(full_path.get())) {
+  } else if (StrStartsWith(name, kSetupPrefix) && !context.setup.IsValid()) {
+    if (!resource.WriteToDisk(full_path.get(), context.setup)) {
       context.error_code = ::GetLastError();
       return FALSE;  // Break: failed to write resource.
     }
-    context.setup_resource_path->assign(full_path);
   } else {
     // Break: unexpected resource names or multiple {chrome,setup}* resources
     // are unexpected.
@@ -330,6 +343,41 @@ BOOL CALLBACK OnResourceFound(HMODULE module,
 }
 
 #if defined(COMPONENT_BUILD)
+// Deletes all files in |base_path| (which must have a trailing path separator).
+void DeleteAllFilesInDir(const wchar_t* base_path) {
+  PathString path;
+  if (!path.assign(base_path))
+    return;
+
+  const size_t base_path_length = path.length();
+  if (!path.append(L"*"))
+    return;
+
+  WIN32_FIND_DATA find_data = {};
+  HANDLE find_handle =
+      ::FindFirstFileEx(path.get(), FindExInfoStandard, &find_data,
+                        FindExSearchNameMatch, /*lpSearchFilter=*/nullptr,
+                        /*dwAdditionalFlags=*/0);
+  if (find_handle == INVALID_HANDLE_VALUE)
+    return;
+
+  do {
+    if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+      continue;
+
+    path.truncate_at(base_path_length);
+    if (!path.append(*find_data.cAlternateFileName
+                         ? find_data.cAlternateFileName
+                         : find_data.cFileName)) {
+      continue;
+    }
+
+    ::DeleteFile(path.get());
+  } while (::FindNextFile(find_handle, &find_data));
+
+  ::FindClose(find_handle);
+}
+
 // An EnumResNameProc callback that writes the resource |name| to disk in the
 // directory |base_path_ptr| (which must end with a path separator).
 BOOL CALLBACK WriteResourceToDirectory(HMODULE module,
@@ -340,8 +388,10 @@ BOOL CALLBACK WriteResourceToDirectory(HMODULE module,
   PathString full_path;
 
   PEResource resource(name, type, module);
+  MiniFile file(MiniFile::DeleteOnClose::kNo);
   return (resource.IsValid() && full_path.assign(base_path) &&
-          full_path.append(name) && resource.WriteToDisk(full_path.get()));
+          full_path.append(name) &&
+          resource.WriteToDisk(full_path.get(), file));
 }
 #endif
 
@@ -360,8 +410,8 @@ BOOL CALLBACK WriteResourceToDirectory(HMODULE module,
 ProcessExitResult UnpackBinaryResources(const Configuration& configuration,
                                         HMODULE module,
                                         const wchar_t* base_path,
-                                        PathString* archive_path,
-                                        PathString* setup_path) {
+                                        MiniFile& archive,
+                                        MiniFile& setup) {
   // Generate the setup.exe path where we patch/uncompress setup resource.
   PathString setup_dest_path;
   if (!setup_dest_path.assign(base_path) || !setup_dest_path.append(kSetupExe))
@@ -369,19 +419,14 @@ ProcessExitResult UnpackBinaryResources(const Configuration& configuration,
 
   // Prepare the input to OnResourceFound method that needs a location where
   // it will write all the resources.
-  Context context = {
-      base_path,
-      archive_path,
-      setup_path,
-      ERROR_SUCCESS,
-  };
+  Context context = {base_path, archive, setup, ERROR_SUCCESS};
 
   // Get the resources of type 'B7' (7zip archive).
   // We need a chrome archive to do the installation. So if there
   // is a problem in fetching B7 resource, just return an error.
   if (!::EnumResourceNames(module, kLZMAResourceType, OnResourceFound,
                            reinterpret_cast<LONG_PTR>(&context)) ||
-      archive_path->empty()) {
+      !archive.IsValid()) {
     const DWORD enum_error = ::GetLastError();
     return ProcessExitResult(UNABLE_TO_EXTRACT_CHROME_ARCHIVE,
                              enum_error == ERROR_RESOURCE_ENUM_USER_STOP
@@ -389,45 +434,59 @@ ProcessExitResult UnpackBinaryResources(const Configuration& configuration,
                                  : enum_error);
   }
 
-  ProcessExitResult exit_code = ProcessExitResult(SUCCESS_EXIT_CODE);
+  if (!archive.DropWritePermission())
+    return {DROP_WRITE_ON_EXTRACTED_ARCHIVE_FAILED, ::GetLastError()};
 
   // If we found setup 'B7' resource (used for differential updates), handle
   // it.  Note that this is only for Chrome; Chromium installs are always
   // "full" installs.
-  if (!setup_path->empty()) {
+  if (setup.IsValid()) {
+    if (!setup.DropWritePermission())
+      return {DROP_WRITE_ON_EXTRACTED_SETUP_PATCH_FAILED, ::GetLastError()};
+
     CommandString cmd_line;
     PathString exe_path;
     // Get the path to setup.exe first.
-    exit_code = GetPreviousSetupExePath(configuration, exe_path.get(),
-                                        exe_path.capacity());
-    if (exit_code.IsSuccess()) {
-      if (!cmd_line.append(L"\"") || !cmd_line.append(exe_path.get()) ||
-          !cmd_line.append(L"\" --") || !cmd_line.append(kCmdUpdateSetupExe) ||
-          !cmd_line.append(L"=\"") || !cmd_line.append(setup_path->get()) ||
-          !cmd_line.append(L"\" --") || !cmd_line.append(kCmdNewSetupExe) ||
-          !cmd_line.append(L"=\"") || !cmd_line.append(setup_dest_path.get()) ||
-          !cmd_line.append(L"\"")) {
-        exit_code = ProcessExitResult(COMMAND_STRING_OVERFLOW);
-      }
+    auto exit_code = GetPreviousSetupExePath(configuration, exe_path.get(),
+                                             exe_path.capacity());
+    if (!exit_code.IsSuccess())
+      return exit_code;
+
+    if (!cmd_line.append(L"\"") || !cmd_line.append(exe_path.get()) ||
+        !cmd_line.append(L"\" --") || !cmd_line.append(kCmdUpdateSetupExe) ||
+        !cmd_line.append(L"=\"") || !cmd_line.append(setup.path()) ||
+        !cmd_line.append(L"\" --") || !cmd_line.append(kCmdNewSetupExe) ||
+        !cmd_line.append(L"=\"") || !cmd_line.append(setup_dest_path.get()) ||
+        !cmd_line.append(L"\"")) {
+      return ProcessExitResult(COMMAND_STRING_OVERFLOW);
     }
 
     // Get any command line option specified for mini_installer and pass them
     // on to setup.exe.
     AppendCommandLineFlags(configuration.command_line(), &cmd_line);
 
-    if (exit_code.IsSuccess()) {
-      exit_code = RunProcessAndWait(
-          exe_path.get(), cmd_line.get(), SETUP_PATCH_FAILED_FILE_NOT_FOUND,
-          SETUP_PATCH_FAILED_PATH_NOT_FOUND,
-          SETUP_PATCH_FAILED_COULD_NOT_CREATE_PROCESS);
-    }
+    exit_code = RunProcessAndWait(exe_path.get(), cmd_line.get(),
+                                  SETUP_PATCH_FAILED_FILE_NOT_FOUND,
+                                  SETUP_PATCH_FAILED_PATH_NOT_FOUND,
+                                  SETUP_PATCH_FAILED_COULD_NOT_CREATE_PROCESS);
+
+    // The setup patch file is no longer needed.
+    setup.Close();
 
     if (!exit_code.IsSuccess())
-      DeleteFile(setup_path->get());
-    else
-      setup_path->assign(setup_dest_path);
+      return exit_code;
 
-    return exit_code;
+    // Open the destination file (with delete-on-close) so that it will be
+    // deleted at process exit.
+    if (!setup.Open(setup_dest_path)) {
+      // Try to delete the file if it could not be opened.
+      const DWORD open_error = ::GetLastError();
+      if (configuration.should_delete_extracted_files())
+        ::DeleteFile(setup_dest_path.get());
+      return {UNABLE_TO_OPEN_PATCHED_SETUP, open_error};
+    }
+
+    return ProcessExitResult(SUCCESS_EXIT_CODE);
   }
 
   // setup.exe wasn't sent as 'B7', lets see if it was sent as 'BL'
@@ -435,7 +494,8 @@ ProcessExitResult UnpackBinaryResources(const Configuration& configuration,
   context.error_code = ERROR_SUCCESS;
   if (!::EnumResourceNames(module, kLZCResourceType, OnResourceFound,
                            reinterpret_cast<LONG_PTR>(&context)) ||
-      setup_path->empty()) {
+      !setup.IsValid()) {
+    // Neither setup_patch.packed.7z nor setup.ex_ could be extracted.
     const DWORD enum_error = ::GetLastError();
     return ProcessExitResult(UNABLE_TO_EXTRACT_SETUP,
                              enum_error == ERROR_RESOURCE_ENUM_USER_STOP
@@ -443,27 +503,29 @@ ProcessExitResult UnpackBinaryResources(const Configuration& configuration,
                                  : enum_error);
   }
 
+  if (!setup.DropWritePermission())
+    return {DROP_WRITE_ON_EXTRACTED_SETUP_FAILED, ::GetLastError()};
+
   // Uncompress LZ compressed resource. Setup is packed with 'MSCF'
   // as opposed to old DOS way of 'SZDD'. Hence we don't use LZCopy.
-  bool success =
-      mini_installer::Expand(setup_path->get(), setup_dest_path.get());
-  ::DeleteFile(setup_path->get());
-  if (success)
-    setup_path->assign(setup_dest_path);
-  else
-    exit_code = ProcessExitResult(UNABLE_TO_EXTRACT_SETUP_EXE);
+  MiniFile setup_dest(GetDeleteOnCloseOption(configuration));
+  mini_installer::Expand(setup.path(), setup_dest_path.get(), setup_dest);
+  setup = std::move(setup_dest);
+
+  if (!setup.IsValid())
+    return ProcessExitResult(UNABLE_TO_EXTRACT_SETUP_EXE);
+  if (!setup.DropWritePermission())
+    return {DROP_WRITE_ON_EXPANDED_SETUP_FAILED, ::GetLastError()};
 
 #if defined(COMPONENT_BUILD)
-  if (exit_code.IsSuccess()) {
-    // Extract the modules in component build required by setup.exe.
-    if (!::EnumResourceNames(module, kBinResourceType, WriteResourceToDirectory,
-                             reinterpret_cast<LONG_PTR>(base_path))) {
-      return ProcessExitResult(UNABLE_TO_EXTRACT_SETUP, ::GetLastError());
-    }
+  // Extract the modules in component build required by setup.exe.
+  if (!::EnumResourceNames(module, kBinResourceType, WriteResourceToDirectory,
+                           reinterpret_cast<LONG_PTR>(base_path))) {
+    return ProcessExitResult(UNABLE_TO_EXTRACT_SETUP, ::GetLastError());
   }
 #endif
 
-  return exit_code;
+  return ProcessExitResult(SUCCESS_EXIT_CODE);
 }
 
 // Executes setup.exe, waits for it to finish and returns the exit code.
@@ -525,16 +587,6 @@ ProcessExitResult RunSetup(const Configuration& configuration,
                            RUN_SETUP_FAILED_COULD_NOT_CREATE_PROCESS);
 }
 
-// Deletes given files and working dir.
-void DeleteExtractedFiles(const wchar_t* base_path,
-                          const wchar_t* archive_path,
-                          const wchar_t* setup_path) {
-  ::DeleteFile(archive_path);
-  ::DeleteFile(setup_path);
-  // Delete the temp dir (if it is empty, otherwise fail).
-  ::RemoveDirectory(base_path);
-}
-
 // Returns true if the supplied path supports ACLs.
 bool IsAclSupportedForPath(const wchar_t* path) {
   PathString volume;
@@ -829,6 +881,8 @@ bool ProcessNonInstallOperations(const Configuration& configuration,
   }
 }
 
+}  // namespace
+
 ProcessExitResult WMain(HMODULE module) {
   // Always start with deleting potential leftovers from previous installations.
   // This can make the difference between success and failure.  We've seen
@@ -867,10 +921,10 @@ ProcessExitResult WMain(HMODULE module) {
   SetInstallerFlags(configuration);
 #endif
 
-  PathString archive_path;
-  PathString setup_path;
+  MiniFile archive(GetDeleteOnCloseOption(configuration));
+  MiniFile setup(GetDeleteOnCloseOption(configuration));
   exit_code = UnpackBinaryResources(configuration, module, base_path.get(),
-                                    &archive_path, &setup_path);
+                                    archive, setup);
 
   // While unpacking the binaries, we paged in a whole bunch of memory that
   // we don't need anymore.  Let's give it back to the pool before running
@@ -878,10 +932,22 @@ ProcessExitResult WMain(HMODULE module) {
   ::SetProcessWorkingSetSize(::GetCurrentProcess(), (SIZE_T)-1, (SIZE_T)-1);
 
   if (exit_code.IsSuccess())
-    exit_code = RunSetup(configuration, archive_path.get(), setup_path.get());
+    exit_code = RunSetup(configuration, archive.path(), setup.path());
 
+  // Closing the files will delete them if should_delete_extracted_files is
+  // true (the normal case).
+  archive.Close();
+  setup.Close();
+
+#if defined(COMPONENT_BUILD)
+  // |base_path| is full of component DLLs in this case, which are not held
+  // open for delete-on-close. Manually delete everything in the directory.
   if (configuration.should_delete_extracted_files())
-    DeleteExtractedFiles(base_path.get(), archive_path.get(), setup_path.get());
+    DeleteAllFilesInDir(base_path.get());
+#endif
+
+  // Delete the temp dir (if it is empty, otherwise fail).
+  ::RemoveDirectory(base_path.get());
 
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)
   WriteInstallResults(configuration, exit_code);

chrome/installer/mini_installer/pe_resource.cc

@@ -8,6 +8,8 @@
 
 #include "chrome/installer/mini_installer/mini_file.h"
 
+namespace mini_installer {
+
 PEResource::PEResource(HRSRC resource, HMODULE module)
     : resource_(resource), module_(module) {}
 
@@ -24,42 +26,43 @@ size_t PEResource::Size() {
   return ::SizeofResource(module_, resource_);
 }
 
-bool PEResource::WriteToDisk(const wchar_t* full_path) {
+bool PEResource::WriteToDisk(const wchar_t* full_path, MiniFile& file) {
   // Resource handles are not real HGLOBALs so do not attempt to close them.
   // Resources are freed when the containing module is unloaded.
   HGLOBAL data_handle = ::LoadResource(module_, resource_);
-  if (nullptr == data_handle)
+  if (!data_handle)
     return false;
 
   const char* data = reinterpret_cast<const char*>(::LockResource(data_handle));
-  if (nullptr == data)
+  if (!data)
     return false;
 
-  mini_installer::MiniFile file;
   if (!file.Create(full_path))
     return false;
 
   // Don't write all of the data at once because this can lead to kernel
   // address-space exhaustion on 32-bit Windows (see https://crbug.com/1001022
   // for details).
-  constexpr size_t kMaxWriteAmount = 8 * 1024 * 1024;
-  const size_t resource_size = Size();
-  for (size_t total_written = 0; total_written < resource_size; /**/) {
-    const size_t write_amount =
+  constexpr DWORD kMaxWriteAmount = 8 * 1024 * 1024;
+  const DWORD resource_size = Size();
+  for (DWORD total_written = 0; total_written < resource_size; /**/) {
+    const DWORD write_amount =
         std::min(kMaxWriteAmount, resource_size - total_written);
     DWORD written = 0;
-    if (!::WriteFile(file.GetHandleUnsafe(), data + total_written,
-                     static_cast<DWORD>(write_amount), &written, nullptr)) {
+    if (!::WriteFile(file.GetHandleUnsafe(), data + total_written, write_amount,
+                     &written, nullptr)) {
       const auto write_error = ::GetLastError();
 
       // Delete the file since the write failed.
-      file.DeleteOnClose();
       file.Close();
 
       ::SetLastError(write_error);
       return false;
     }
-    total_written += write_amount;
+    total_written += written;
   }
+
   return true;
 }
+
+}  // namespace mini_installer

chrome/installer/mini_installer/pe_resource.h

@@ -9,6 +9,10 @@
 
 #include <stddef.h>
 
+namespace mini_installer {
+
+class MiniFile;
+
 // This class models a windows PE resource. It does not pretend to be a full
 // API wrapper and it is just concerned with loading it to memory and writing
 // it to disk. Each resource is unique only in the context of a loaded module,
@@ -30,14 +34,17 @@ class PEResource {
   // not valid.
   size_t Size();
 
-  // Creates a file in 'path' with a copy of the resource. If the resource can
-  // not be loaded into memory or if it cannot be written to disk it returns
-  // false.
-  bool WriteToDisk(const wchar_t* path);
+  // Writes the resource to the file |path|. Returns true on success, in which
+  // case |file| holds an open handle to the destination file. |file| will be
+  // opened with exclusive write access and shared read and delete access, and
+  // will be marked as delete-on-close.
+  bool WriteToDisk(const wchar_t* path, MiniFile& file);
 
  private:
   HRSRC resource_;
   HMODULE module_;
 };
 
+}  // namespace mini_installer
+
 #endif  // CHROME_INSTALLER_MINI_INSTALLER_PE_RESOURCE_H_

chrome/installer/mini_installer/pe_resource_test.cc

+// Copyright 2020 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/installer/mini_installer/pe_resource.h"
+
+#include <windows.h>
+
+#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "chrome/installer/mini_installer/mini_file.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mini_installer {
+
+TEST(PEResourceTest, WriteToDisk) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath manifest_path =
+      temp_dir.GetPath().Append(FILE_PATH_LITERAL("manifest"));
+
+  PEResource manifest(MAKEINTRESOURCE(1), RT_MANIFEST,
+                      ::GetModuleHandle(nullptr));
+  ASSERT_TRUE(manifest.IsValid());
+
+  MiniFile manifest_file(MiniFile::DeleteOnClose::kYes);
+  ASSERT_TRUE(
+      manifest.WriteToDisk(manifest_path.value().c_str(), manifest_file));
+
+  ASSERT_TRUE(manifest_file.IsValid());
+  EXPECT_PRED1(base::PathExists, manifest_path);
+
+  manifest_file.Close();
+  EXPECT_FALSE(base::PathExists(manifest_path));
+
+  EXPECT_TRUE(temp_dir.Delete());
+}
+
+}  // namespace mini_installer