Introduce MiniFile, a simple wrapper around a file handle.

Resource extraction and file expansion in the mini_installer now use
the MiniFile class to manage output files while they are open. It takes
care of handle lifetime management and provides a robust way to delete
an open file.

This change also fixes two potential file leaks. Specifically, a
destination file created by either PEResource::WriteToDisk or
mini_installer::Expand is now deleted in case of error.

BUG=516207

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

chrome/installer/mini_installer/BUILD.gn

@@ -34,6 +34,8 @@ source_set("lib") {
     "configuration.h",
     "decompress.cc",
     "decompress.h",
+    "mini_file.cc",
+    "mini_file.h",
     "mini_installer.cc",
     "mini_installer.h",
     "mini_installer.rc",
@@ -42,6 +44,7 @@ source_set("lib") {
     "mini_installer_resource.h",
     "mini_string.cc",
     "mini_string.h",
+    "path_string.h",
     "pe_resource.cc",
     "pe_resource.h",
     "regkey.cc",
@@ -70,6 +73,7 @@ source_set("unit_tests") {
   sources = [
     "configuration_test.cc",
     "decompress_test.cc",
+    "mini_file_test.cc",
     "mini_installer_unittest.cc",
     "mini_string_test.cc",
   ]

chrome/installer/mini_installer/decompress.cc

@@ -11,6 +11,8 @@
 #include <stddef.h>
 #include <stdlib.h>
 
+#include "chrome/installer/mini_installer/mini_file.h"
+
 namespace {
 
 // A simple struct to hold data passed to and from FDICopy via its |pvUser|
@@ -19,7 +21,11 @@ struct ExpandContext {
   // The path to the single destination file.
   const wchar_t* const dest_path;
 
-  // Set to true if the file was extracted to |dest_path|.
+  // The destination file; valid once the destination is created.
+  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.
   bool succeeded;
 };
 
@@ -134,11 +140,12 @@ FNFDINOTIFY(Notify) {
 
   switch (fdint) {
     case fdintCOPY_FILE:
+      context.dest_file.Create(context.dest_path);
       // By sheer coincidence, CreateFileW's success/failure results match that
-      // of fdintCOPY_FILE.
-      return reinterpret_cast<INT_PTR>(::CreateFileW(
-          context.dest_path, GENERIC_WRITE, FILE_SHARE_READ, nullptr,
-          CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr));
+      // of fdintCOPY_FILE. The handle given out here is closed either by
+      // FDICopy (in case of error) or below when handling fdintCLOSE_FILE_INFO
+      // (in case of success).
+      return reinterpret_cast<INT_PTR>(context.dest_file.DuplicateHandle());
 
     case fdintCLOSE_FILE_INFO: {
       // Set the file's creation time and file attributes.
@@ -156,6 +163,7 @@ FNFDINOTIFY(Notify) {
       ::SetFileInformationByHandle(reinterpret_cast<HANDLE>(pfdin->hf),
                                    FileBasicInfo, &info, sizeof(info));
 
+      // Close the handle given out above in fdintCOPY_FILE.
       ::CloseHandle(reinterpret_cast<HANDLE>(pfdin->hf));
       context.succeeded = true;
       return -1;  // Break: the one file was extracted.
@@ -266,11 +274,18 @@ bool Expand(const wchar_t* source, const wchar_t* destination) {
   if (!fdi)
     return false;
 
-  ExpandContext context = {destination, /*succeeded=*/false};
+  ExpandContext context = {destination, {}, /*succeeded=*/false};
   g_FDICopy(fdi, source_name_utf8, source_path_utf8, 0, &Notify, nullptr,
             &context);
   g_FDIDestroy(fdi);
-  return context.succeeded;
+  if (context.succeeded)
+    return true;
+
+  // Delete the output file if it was created.
+  if (context.dest_file.IsValid())
+    context.dest_file.DeleteOnClose();
+
+  return false;
 }
 
 }  // namespace mini_installer

chrome/installer/mini_installer/mini_file.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/mini_file.h"
+
+#include <utility>
+
+namespace mini_installer {
+
+MiniFile::MiniFile() = default;
+
+MiniFile::~MiniFile() {
+  Close();
+}
+
+MiniFile& MiniFile::operator=(MiniFile&& other) noexcept {
+  Close();
+  path_.assign(other.path_);
+  other.path_.clear();
+  handle_ = std::exchange(other.handle_, INVALID_HANDLE_VALUE);
+  return *this;
+}
+
+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);
+  if (handle_ != INVALID_HANDLE_VALUE)
+    return true;
+  path_.clear();
+  return false;
+}
+
+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));
+}
+
+void MiniFile::Close() {
+  if (IsValid())
+    ::CloseHandle(std::exchange(handle_, INVALID_HANDLE_VALUE));
+  path_.clear();
+}
+
+HANDLE MiniFile::DuplicateHandle() const {
+  if (!IsValid())
+    return INVALID_HANDLE_VALUE;
+  HANDLE handle = INVALID_HANDLE_VALUE;
+  return ::DuplicateHandle(::GetCurrentProcess(), handle_,
+                           ::GetCurrentProcess(), &handle,
+                           /*dwDesiredAccess=*/0,
+                           /*bInerhitHandle=*/FALSE, DUPLICATE_SAME_ACCESS)
+             ? handle
+             : INVALID_HANDLE_VALUE;
+}
+
+HANDLE MiniFile::GetHandleUnsafe() const {
+  return handle_;
+}
+
+}  // namespace mini_installer

chrome/installer/mini_installer/mini_file.h

+// 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.
+
+#ifndef CHROME_INSTALLER_MINI_INSTALLER_MINI_FILE_H_
+#define CHROME_INSTALLER_MINI_INSTALLER_MINI_FILE_H_
+
+#include <windows.h>
+
+#include "chrome/installer/mini_installer/path_string.h"
+
+namespace mini_installer {
+
+// A simple abstraction over a path to a file and a Windows file handle to it.
+class MiniFile {
+ public:
+  MiniFile();
+
+  // Closes the file if the instance holds a valid handle. The file will be
+  // deleted if directed by a call to DeleteOnClose().
+  ~MiniFile();
+
+  MiniFile(const MiniFile&) = delete;
+  MiniFile(MiniFile&&) = delete;
+  MiniFile& operator=(const MiniFile&) = delete;
+
+  // Postcondition: other.path() will return an empty string and other.IsValid()
+  // will return false.
+  MiniFile& operator=(MiniFile&& other) noexcept;
+
+  // Creates a new file at |path| for exclusive writing. Returns true if the
+  // file was created, in which case IsValid() will return true.
+  bool Create(const wchar_t* path);
+
+  // 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.
+  void Close();
+
+  // Returns a new handle to the file, or INVALID_HANDLE_VALUE on error.
+  HANDLE DuplicateHandle() const;
+
+  // 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(); }
+
+  // Returns the open file handle. The caller must not close it, and must not
+  // refer to it after this instance is closed or destroyed.
+  HANDLE GetHandleUnsafe() const;
+
+ private:
+  // The path by which |handle_| was created or opened, or an empty path if
+  // |handle_| is not valid.
+  PathString path_;
+
+  // A handle to the open file, or INVALID_HANDLE_VALUE.
+  HANDLE handle_ = INVALID_HANDLE_VALUE;
+};
+
+}  // namespace mini_installer
+
+#endif  // CHROME_INSTALLER_MINI_INSTALLER_MINI_FILE_H_

chrome/installer/mini_installer/mini_file_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/mini_file.h"
+
+#include <utility>
+
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mini_installer {
+
+class MiniFileTest : public ::testing::Test {
+ protected:
+  void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); }
+
+  void TearDown() override { EXPECT_TRUE(temp_dir_.Delete()); }
+
+  const base::FilePath& temp_dir() const { return temp_dir_.GetPath(); }
+
+ private:
+  base::ScopedTempDir temp_dir_;
+};
+
+// Create should create a file.
+TEST_F(MiniFileTest, Create) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file;
+  ASSERT_TRUE(file.Create(file_path.value().c_str()));
+  ASSERT_TRUE(base::PathExists(file_path));
+}
+
+// Created files should be deletable by others and should vanish when closed.
+TEST_F(MiniFileTest, CreateDeleteIsShared) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file;
+  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));
+}
+
+// DeleteOnClose should work as advertised.
+TEST_F(MiniFileTest, DeleteOnClose) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file;
+  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));
+}
+
+// Close should really close.
+TEST_F(MiniFileTest, Close) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file;
+  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());
+}
+
+// DuplicateHandle should work as advertized.
+TEST_F(MiniFileTest, DuplicateHandle) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file;
+  ASSERT_TRUE(file.Create(file_path.value().c_str()));
+  HANDLE dup = file.DuplicateHandle();
+  ASSERT_NE(dup, INVALID_HANDLE_VALUE);
+
+  // Check that the two handles reference the same file.
+  BY_HANDLE_FILE_INFORMATION info1 = {};
+  ASSERT_NE(::GetFileInformationByHandle(file.GetHandleUnsafe(), &info1),
+            FALSE);
+  BY_HANDLE_FILE_INFORMATION info2 = {};
+  ASSERT_NE(::GetFileInformationByHandle(dup, &info2), FALSE);
+  EXPECT_EQ(info1.dwVolumeSerialNumber, info2.dwVolumeSerialNumber);
+  EXPECT_EQ(info1.nFileIndexHigh, info2.nFileIndexHigh);
+  EXPECT_EQ(info1.nFileIndexLow, info2.nFileIndexLow);
+
+  ::CloseHandle(std::exchange(dup, INVALID_HANDLE_VALUE));
+}
+
+TEST_F(MiniFileTest, Path) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file;
+  EXPECT_EQ(*file.path(), 0);
+
+  ASSERT_TRUE(file.Create(file_path.value().c_str()));
+  EXPECT_STREQ(file.path(), file_path.value().c_str());
+
+  file.Close();
+  EXPECT_EQ(*file.path(), 0);
+}
+
+TEST_F(MiniFileTest, GetHandleUnsafe) {
+  const base::FilePath file_path = temp_dir().Append(FILE_PATH_LITERAL("HUM"));
+
+  MiniFile file;
+  EXPECT_EQ(file.GetHandleUnsafe(), INVALID_HANDLE_VALUE);
+
+  ASSERT_TRUE(file.Create(file_path.value().c_str()));
+  EXPECT_NE(file.GetHandleUnsafe(), INVALID_HANDLE_VALUE);
+
+  file.Close();
+  EXPECT_EQ(file.GetHandleUnsafe(), INVALID_HANDLE_VALUE);
+}
+
+}  // namespace mini_installer

chrome/installer/mini_installer/path_string.h

+// 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.
+
+#ifndef CHROME_INSTALLER_MINI_INSTALLER_PATH_STRING_H_
+#define CHROME_INSTALLER_MINI_INSTALLER_PATH_STRING_H_
+
+#include <windows.h>
+
+#include "chrome/installer/mini_installer/mini_string.h"
+
+namespace mini_installer {
+
+// A string sufficiently large to hold a Windows file path. Note that starting
+// with Windows 10 1607, applications may opt-in to supporting long paths via
+// the longPathAware element in the app's manifest. Chrome does not do this. See
+// https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#maximum-path-length-limitation
+// for details.
+using PathString = StackString<MAX_PATH>;
+
+}  // namespace mini_installer
+
+#endif  // CHROME_INSTALLER_MINI_INSTALLER_PATH_STRING_H_

chrome/installer/mini_installer/pe_resource.cc

@@ -6,6 +6,8 @@
 
 #include <algorithm>
 
+#include "chrome/installer/mini_installer/mini_file.h"
+
 PEResource::PEResource(HRSRC resource, HMODULE module)
     : resource_(resource), module_(module) {}
 
@@ -33,35 +35,31 @@ bool PEResource::WriteToDisk(const wchar_t* full_path) {
   if (nullptr == data)
     return false;
 
-  const size_t resource_size = Size();
-  HANDLE out_file =
-      ::CreateFile(full_path, DELETE | GENERIC_WRITE, FILE_SHARE_DELETE,
-                   nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
-  if (INVALID_HANDLE_VALUE == out_file)
+  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 =
         std::min(kMaxWriteAmount, resource_size - total_written);
     DWORD written = 0;
-    if (!::WriteFile(out_file, data + total_written,
+    if (!::WriteFile(file.GetHandleUnsafe(), data + total_written,
                      static_cast<DWORD>(write_amount), &written, nullptr)) {
       const auto write_error = ::GetLastError();
 
       // Delete the file since the write failed.
-      FILE_DISPOSITION_INFO disposition = {/*DeleteFile=*/TRUE};
-      ::SetFileInformationByHandle(out_file, FileDispositionInfo, &disposition,
-                                   sizeof(disposition));
-      ::CloseHandle(out_file);
+      file.DeleteOnClose();
+      file.Close();
 
       ::SetLastError(write_error);
       return false;
     }
     total_written += write_amount;
   }
-  return ::CloseHandle(out_file) ? true : false;
+  return true;
 }