Fix CopyTreeWorkItem::IsFileInUse for non-elevated use.

This function has historically been opening a file for exclusive r/w/d
with FILE_ALL_ACCESS. FILE_ALL_ACCESS is nearly almost never, ever the
right choice when opening a file. This is especially true when opening a
file from an ordinary non-elevated user session, since it attempts to
gain things like WRITE_DAC and WRITE_OWNER and will fail if it's not
possible to gain those access rights.

A better way to determine if a file is "in use" (and what we really mean
is "is an executable that is running") is simply to try to write to
it. This is because CreateProcess opens its target with only exclusive
write mode (read and execute are shared for obvious reasons).

This change also eliminates the exact copy of this function, which
included the incorrect formatting from the original, that was used in
the unittest.

R=gab@chromium.org

BUG: 474401
Change-Id: I0488e2a5333f74353f46ae6c87795e2833a30b61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1905948
Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
Auto-Submit: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713858}

chrome/installer/util/copy_tree_work_item.cc

@@ -126,14 +126,25 @@ void CopyTreeWorkItem::RollbackImpl() {
   }
 }
 
+// static
 bool CopyTreeWorkItem::IsFileInUse(const base::FilePath& path) {
   if (!base::PathExists(path))
     return false;
 
-  HANDLE handle = ::CreateFile(path.value().c_str(), FILE_ALL_ACCESS,
-                               NULL, NULL, OPEN_EXISTING, NULL, NULL);
-  if (handle  == INVALID_HANDLE_VALUE)
+  // A running executable is open with exclusive write access, so attempting to
+  // write to it will fail with a sharing violation. A more precise method would
+  // be to open the file with DELETE access and attempt to set the delete
+  // disposition on the handle. This would fail if the file was mapped into a
+  // process's address space, but succeed otherwise. This seems like overkill,
+  // however.
+  HANDLE handle =
+      ::CreateFile(path.value().c_str(), FILE_WRITE_DATA,
+                   FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                   nullptr, OPEN_EXISTING, 0, nullptr);
+  if (handle == INVALID_HANDLE_VALUE) {
+    DPCHECK(::GetLastError() == ERROR_SHARING_VIOLATION);
     return true;
+  }
 
   CloseHandle(handle);
   return false;

chrome/installer/util/copy_tree_work_item.h

@@ -46,7 +46,7 @@ class CopyTreeWorkItem : public WorkItem {
   void RollbackImpl() override;
 
   // Checks if the path specified is in use (and hence can not be deleted)
-  bool IsFileInUse(const base::FilePath& path);
+  static bool IsFileInUse(const base::FilePath& path);
 
   // Source path to copy files from.
   base::FilePath source_path_;

chrome/installer/util/copy_tree_work_item_unittest.cc

@@ -44,19 +44,6 @@ void CreateTextFile(const std::wstring& filename,
   file.close();
 }
 
-bool IsFileInUse(const base::FilePath& path) {
-  if (!base::PathExists(path))
-    return false;
-
-  HANDLE handle = ::CreateFile(path.value().c_str(), FILE_ALL_ACCESS,
-                               NULL, NULL, OPEN_EXISTING, NULL, NULL);
-  if (handle  == INVALID_HANDLE_VALUE)
-    return true;
-
-  CloseHandle(handle);
-  return false;
-}
-
 // Simple function to read text from a file.
 std::wstring ReadTextFile(const std::wstring& filename) {
   WCHAR contents[64];
@@ -398,6 +385,7 @@ TEST_F(CopyTreeWorkItemTest, NewNameAndCopyTest) {
   alternate_to = alternate_to.AppendASCII("Alternate_To");
   base::CopyFile(exe_full_path, file_name_to);
   ASSERT_TRUE(base::PathExists(file_name_to));
+  ASSERT_FALSE(CopyTreeWorkItem::IsFileInUse(file_name_to));
 
   VLOG(1) << "copy ourself from " << exe_full_path.value()
           << " to " << file_name_to.value();
@@ -446,10 +434,10 @@ TEST_F(CopyTreeWorkItemTest, NewNameAndCopyTest) {
   work_item.reset(WorkItem::CreateCopyTreeWorkItem(
       file_name_from, file_name_to, temp_dir_.GetPath(),
       WorkItem::NEW_NAME_IF_IN_USE, alternate_to));
-  if (IsFileInUse(file_name_to))
+  if (CopyTreeWorkItem::IsFileInUse(file_name_to))
     base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(2));
   // If file is still in use, the rest of the test will fail.
-  ASSERT_FALSE(IsFileInUse(file_name_to));
+  ASSERT_FALSE(CopyTreeWorkItem::IsFileInUse(file_name_to));
   EXPECT_TRUE(work_item->Do());
 
   // Get the path of backup file