Make CopyDirectory() not copy the read only bit on Windows.

base/file_util_win.cc

 // Copyright (c) 2012 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 "base/file_util.h"
 
 #include <windows.h>
 #include <psapi.h>
 #include <shellapi.h>
 #include <shlobj.h>
 #include <time.h>
 
 #include <algorithm>
 #include <limits>
 #include <string>
 
+#include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/process/process_handle.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
 
 namespace base {
 
 namespace {
 
 const DWORD kFileShareAll =
     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
 
-bool ShellCopy(const FilePath& from_path,
-               const FilePath& to_path,
-               bool recursive) {
-  // WinXP SHFileOperation doesn't like trailing separators.
-  FilePath stripped_from = from_path.StripTrailingSeparators();
-  FilePath stripped_to = to_path.StripTrailingSeparators();
-
-  ThreadRestrictions::AssertIOAllowed();
-
-  // NOTE: I suspect we could support longer paths, but that would involve
-  // analyzing all our usage of files.
-  if (stripped_from.value().length() >= MAX_PATH ||
-      stripped_to.value().length() >= MAX_PATH) {
-    return false;
-  }
-
-  // SHFILEOPSTRUCT wants the path to be terminated with two NULLs,
-  // so we have to use wcscpy because wcscpy_s writes non-NULLs
-  // into the rest of the buffer.
-  wchar_t double_terminated_path_from[MAX_PATH + 1] = {0};
-  wchar_t double_terminated_path_to[MAX_PATH + 1] = {0};
-#pragma warning(suppress:4996)  // don't complain about wcscpy deprecation
-  wcscpy(double_terminated_path_from, stripped_from.value().c_str());
-#pragma warning(suppress:4996)  // don't complain about wcscpy deprecation
-  wcscpy(double_terminated_path_to, stripped_to.value().c_str());
-
-  SHFILEOPSTRUCT file_operation = {0};
-  file_operation.wFunc = FO_COPY;
-  file_operation.pFrom = double_terminated_path_from;
-  file_operation.pTo = double_terminated_path_to;
-  file_operation.fFlags = FOF_NOERRORUI | FOF_SILENT | FOF_NOCONFIRMATION |
-                          FOF_NOCONFIRMMKDIR;
-  if (!recursive)
-    file_operation.fFlags |= FOF_NORECURSION | FOF_FILESONLY;
-
-  return (SHFileOperation(&file_operation) == 0);
-}
-
 }  // namespace
 
 FilePath MakeAbsoluteFilePath(const FilePath& input) {
   ThreadRestrictions::AssertIOAllowed();
   wchar_t file_path[MAX_PATH];
   if (!_wfullpath(file_path, input.value().c_str(), MAX_PATH))
     return FilePath();
   return FilePath(file_path);
 }
 
@@ skipping 72 matching lines @@
                     REPLACEFILE_IGNORE_MERGE_ERRORS, NULL, NULL)) {
     return true;
   }
   if (error)
     *error = File::OSErrorToFileError(GetLastError());
   return false;
 }
 
 bool CopyDirectory(const FilePath& from_path, const FilePath& to_path,
                    bool recursive) {
+  // NOTE(maruel): Previous version of this function used to call
+  // SHFileOperation().  This used to copy the file attributes and extended
+  // attributes, OLE structured storage, NTFS file system alternate data
+  // streams, SECURITY_DESCRIPTOR. In practice, this is not what we want, we
+  // want the containing directory to propagate its SECURITY_DESCRIPTOR.
   ThreadRestrictions::AssertIOAllowed();
 
-  if (recursive)
-    return ShellCopy(from_path, to_path, true);
-
-  // The following code assumes that from path is a directory.
-  DCHECK(DirectoryExists(from_path));
-
-  // Instead of creating a new directory, we copy the old one to include the
-  // security information of the folder as part of the copy.
-  if (!PathExists(to_path)) {
-    // Except that Vista fails to do that, and instead do a recursive copy if
-    // the target directory doesn't exist.
-    if (base::win::GetVersion() >= base::win::VERSION_VISTA)
-      CreateDirectory(to_path);
-    else
-      ShellCopy(from_path, to_path, false);
+  // NOTE: I suspect we could support longer paths, but that would involve
+  // analyzing all our usage of files.
+  if (from_path.value().length() >= MAX_PATH ||
+      to_path.value().length() >= MAX_PATH) {
+    return false;
   }
 
-  FilePath directory = from_path.Append(L"*.*");
-  return ShellCopy(directory, to_path, false);
+  // This function does not properly handle destinations within the source.
+  FilePath real_to_path = to_path;
+  if (PathExists(real_to_path)) {
+    real_to_path = MakeAbsoluteFilePath(real_to_path);
+    if (real_to_path.empty())
+      return false;
+  } else {
+    real_to_path = MakeAbsoluteFilePath(real_to_path.DirName());
+    if (real_to_path.empty())
+      return false;
+  }
+  FilePath real_from_path = MakeAbsoluteFilePath(from_path);
+  if (real_from_path.empty())
+    return false;
+  if (real_to_path.value().size() >= real_from_path.value().size() &&
+      real_to_path.value().compare(0, real_from_path.value().size(),
+                                   real_from_path.value()) == 0) {
+    return false;
+  }
+
+  int traverse_type = FileEnumerator::FILES;
+  if (recursive)
+    traverse_type |= FileEnumerator::DIRECTORIES;
+  FileEnumerator traversal(from_path, recursive, traverse_type);
+
+  if (!PathExists(from_path)) {
+    DLOG(ERROR) << "CopyDirectory() couldn't stat source directory: "
+                << from_path.value().c_str();
+    return false;
+  }
+  // TODO(maruel): This is not necessary anymore.
+  DCHECK(recursive || DirectoryExists(from_path));
+
+  FilePath current = from_path;
+  bool from_is_dir = DirectoryExists(from_path);
+  bool success = true;
+  FilePath from_path_base = from_path;
+  if (recursive && DirectoryExists(to_path)) {
+    // If the destination already exists and is a directory, then the
+    // top level of source needs to be copied.
+    from_path_base = from_path.DirName();
+  }
+
+  while (success && !current.empty()) {
+    // current is the source path, including from_path, so append
+    // the suffix after from_path to to_path to create the target_path.
+    FilePath target_path(to_path);
+    if (from_path_base != current) {
+      if (!from_path_base.AppendRelativePath(current, &target_path)) {
+        success = false;
+        break;
+      }
+    }
+
+    if (from_is_dir) {
+      if (!DirectoryExists(target_path) &&
+          !::CreateDirectory(target_path.value().c_str(), NULL)) {
+        DLOG(ERROR) << "CopyDirectory() couldn't create directory: "
+                    << target_path.value().c_str();
+        success = false;
+      }
+    } else if (!internal::CopyFileUnsafe(current, target_path)) {
+      DLOG(ERROR) << "CopyDirectory() couldn't create file: "
+                  << target_path.value().c_str();
+      success = false;
+    }
+
+    current = traversal.Next();
+    if (!current.empty())
+      from_is_dir = traversal.GetInfo().IsDirectory();
+  }
+
+  return success;
 }
 
 bool PathExists(const FilePath& path) {
   ThreadRestrictions::AssertIOAllowed();
   return (GetFileAttributes(path.value().c_str()) != INVALID_FILE_ATTRIBUTES);
 }
 
 bool PathIsWritable(const FilePath& path) {
   ThreadRestrictions::AssertIOAllowed();
   HANDLE dir =
@@ skipping 515 matching lines @@
   if (!ret) {
     // Leave a clue about what went wrong so that it can be (at least) picked
     // up by a PLOG entry.
     ::SetLastError(last_error);
   }
 
   return ret;
 }
 
 bool CopyFileUnsafe(const FilePath& from_path, const FilePath& to_path) {
+  // NOTE(maruel): Previous version of this function used to call CopyFile().
+  // This used to copy the file attributes and extended attributes, OLE
+  // structured storage, NTFS file system alternate data streams,

[rvargas (doing something else), 2014/01/28 22:40:37]

Having base::CopyFile() drop alternate data streams is a very surprising API
IMO.

While I understand that this works for tests, I don't think we should tailor the
behavior of what seems to be general purpose base code to the requirements of
test code.

CopyFile should do what copying a file through the shell does on each platform.
If POSIX doesn't support alternate data streams that behavior should not be
extended to a platform that supports them just for the sake of unified behavior.

[M-A Ruel, 2014/01/30 14:12:14]

Please state a single occurrence where copying the ADS or ACL is useful. I'm not
only doing it for tests and consistency but also because it makes sense. Here's
a few examples;

- content/browser/download/save_file_manager.cc uses CopyFile() in
SaveFileManager::SaveLocalFile() but it doesn't make sense to keep the ACL when
"downloading" a local file.

- chrome/browser/extensions/convert_user_script.cc
ConvertUserScriptToExtension() uses CopyFile() to copy the file into a temp
path. This means the file will keep the ACL of whatever imported extension will
keep the ACL of the source file. It doesn't make sense either.

- content/browser/download/base_file_posix.cc explicitly zap out file mode.
- and content/browser/download/base_file_win.cc doesn't use base::CopyFile(), it
calls SHFileOperation() directly.

- chrome/utility/importer/ie_importer_win.cc is the only place where an ADS is
actively used and *only* for reading.

- cloud_print/virtual_driver/win/install/setup.cc uses CopyFile to copy its
monitor dll name into a sensitive path. It doesn't make sense to keep the file
"user writeable" with the potentially insecure original security descriptor.

- chrome/browser/extensions/sandboxed_unpacker.cc uses CopyFile to unpack an
extension into a temporary directory (which in theories will have only access by
the current user). It doesn't make sense to copy a potentially wide acl in
there.

The exact list of files can be gathered with:
git gs "CopyFile(" | egrep -v "_(unittest|browsertest|apitest)\." | grep -v
"\.py"

+  // SECURITY_DESCRIPTOR. In practice, this is not what we want, we want the
+  // containing directory to propagate its SECURITY_DESCRIPTOR.
   ThreadRestrictions::AssertIOAllowed();
 
   // NOTE: I suspect we could support longer paths, but that would involve
   // analyzing all our usage of files.
   if (from_path.value().length() >= MAX_PATH ||
       to_path.value().length() >= MAX_PATH) {
     return false;
   }
+  base::win::ScopedHandle from_handle(
+      ::CreateFile(from_path.value().c_str(),
+                   GENERIC_READ,
+                   kFileShareAll,
+                   NULL,
+                   OPEN_EXISTING,
+                   FILE_FLAG_SEQUENTIAL_SCAN,
+                   NULL));
+  if (!from_handle)
+    return false;
+  base::win::ScopedHandle to_handle(
+      ::CreateFile(to_path.value().c_str(),
+                   GENERIC_WRITE,
+                   0,
+                   NULL,
+                   CREATE_ALWAYS,
+                   FILE_FLAG_SEQUENTIAL_SCAN,
+                   NULL));
+  if (!to_handle)
+    return false;
 
-  // Unlike the posix implementation that copies the file manually and discards
-  // the ACL bits, CopyFile() copies the complete SECURITY_DESCRIPTOR and access
-  // bits, which is usually not what we want. We can't do much about the
-  // SECURITY_DESCRIPTOR but at least remove the read only bit.
-  const wchar_t* dest = to_path.value().c_str();
-  if (!::CopyFile(from_path.value().c_str(), dest, false)) {
-    // Copy failed.
-    return false;
+  const DWORD kBufferSize = 64 * 1024;
+  scoped_ptr<uint8_t[]> buffer(new uint8_t[kBufferSize]);
+  bool result = false;
+  while (true) {
+    DWORD size = 0;
+    if (!::ReadFile(from_handle, buffer.get(), kBufferSize, &size, NULL))
+      break;
+    if (size == 0) {
+      result = true;
+      break;
+    }
+    if (!::WriteFile(to_handle, buffer.get(), size, &size, NULL))
+      break;
   }
-  DWORD attrs = GetFileAttributes(dest);
-  if (attrs == INVALID_FILE_ATTRIBUTES) {
-    return false;
+  // If failed, try to delete the destination file.
+  if (!result) {
+    to_handle.Close();
+    DeleteFile(to_path, false);
   }
-  if (attrs & FILE_ATTRIBUTE_READONLY) {
-    SetFileAttributes(dest, attrs & ~FILE_ATTRIBUTE_READONLY);
-  }
-  return true;
+  return result;
 }
 
 bool CopyAndDeleteDirectory(const FilePath& from_path,
                             const FilePath& to_path) {
   ThreadRestrictions::AssertIOAllowed();
   if (CopyDirectory(from_path, to_path, true)) {
     if (DeleteFile(from_path, true))
       return true;
 
     // Like Move, this function is not transactional, so we just
     // leave the copied bits behind if deleting from_path fails.
     // If to_path exists previously then we have already overwritten
     // it by now, we don't get better off by deleting the new bits.
   }
   return false;
 }
 
 }  // namespace internal
 }  // namespace base