Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(101)

Unified Diff: base/file_util_win.cc

Issue 141273010: Make CopyDirectory() not copy the read only bit on Windows. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: whitespace Created 6 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« base/file_util_posix.cc ('K') | « base/file_util_unittest.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: base/file_util_win.cc
diff --git a/base/file_util_win.cc b/base/file_util_win.cc
index dd6282516e4f0a1888ac56da826476898bd9e758..98b29eb84c8295afc45b98a76caf6cabf4f1cad8 100644
--- a/base/file_util_win.cc
+++ b/base/file_util_win.cc
@@ -14,6 +14,7 @@
#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"
@@ -34,44 +35,6 @@ 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) {
@@ -164,27 +127,92 @@ bool ReplaceFile(const FilePath& from_path,
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();
+ // 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;
+ }
+
+ // This function does not properly handle destinations within the source
grt (UTC plus 2) 2014/01/24 20:50:07 nit: period at end of sentence
M-A Ruel 2014/01/24 21:32:57 Done.
+ FilePath real_to_path = to_path;
+ if (PathExists(real_to_path)) {
grt (UTC plus 2) 2014/01/24 20:50:07 do the existing unittests cover all of these cases
M-A Ruel 2014/01/24 21:32:57 In FileUtilTest, - CopyDirectoryRecursivelyNew - C
+ 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)
grt (UTC plus 2) 2014/01/24 20:50:07 indent to align with 0 on previous line and add br
M-A Ruel 2014/01/24 21:32:57 Done here and in file_util_posix.cc (where I copie
+ return false;
+
+ int traverse_type = FileEnumerator::FILES;
if (recursive)
- return ShellCopy(from_path, to_path, true);
+ traverse_type |= FileEnumerator::DIRECTORIES;
+ FileEnumerator traversal(from_path, recursive, traverse_type);
- // The following code assumes that from path is a directory.
- DCHECK(DirectoryExists(from_path));
+ 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();
+ }
- // 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);
+ 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();
}
- FilePath directory = from_path.Append(L"*.*");
- return ShellCopy(directory, to_path, false);
+ return success;
}
bool PathExists(const FilePath& path) {
@@ -720,6 +748,11 @@ bool MoveUnsafe(const FilePath& from_path, const FilePath& to_path) {
}
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,
+ // 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
@@ -729,23 +762,49 @@ bool CopyFileUnsafe(const FilePath& from_path, const FilePath& to_path) {
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.
+ 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;
- }
- DWORD attrs = GetFileAttributes(dest);
- if (attrs == INVALID_FILE_ATTRIBUTES) {
+ base::win::ScopedHandle to_handle(
+ ::CreateFile(to_path.value().c_str(),
+ GENERIC_WRITE,
+ kFileShareAll,
grt (UTC plus 2) 2014/01/24 20:50:07 kFileShareAll -> 0
M-A Ruel 2014/01/24 21:32:57 Done.
+ NULL,
+ CREATE_ALWAYS,
+ FILE_FLAG_SEQUENTIAL_SCAN,
+ NULL));
+ if (!to_handle)
return false;
+
+ std::vector<char> buffer(64*1024);
M-A Ruel 2014/01/24 19:52:31 The posix version use 32kb but Windows really like
grt (UTC plus 2) 2014/01/24 20:50:07 you don't seem to be using the power of vector (wh
M-A Ruel 2014/01/24 21:32:57 Done, but that still zero-out the memory. :)
grt (UTC plus 2) 2014/01/26 23:55:08 I don't think C++03 requires this. My read of the
+ bool result = true;
grt (UTC plus 2) 2014/01/24 20:50:07 fewer lines of code if result starts as false and
M-A Ruel 2014/01/24 21:32:57 Done.
+ while (result) {
+ DWORD size = 0;
+ if (!::ReadFile(from_handle, &buffer[0], buffer.size(), &size, NULL)) {
+ result = false;
+ break;
+ }
+ if (size == 0) {
grt (UTC plus 2) 2014/01/24 20:50:07 omit braces on single-line conditionals
M-A Ruel 2014/01/24 21:32:57 Done.
+ break;
+ }
+ if (!::WriteFile(to_handle, &buffer[0], size, &size, NULL)) {
+ result = false;
+ break;
+ }
}
- if (attrs & FILE_ATTRIBUTE_READONLY) {
- SetFileAttributes(dest, attrs & ~FILE_ATTRIBUTE_READONLY);
+ // If failed, try to delete the destination file.
+ if (!result) {
+ to_handle.Close();
+ DeleteFile(to_path, false);
}
- return true;
+ return result;
}
bool CopyAndDeleteDirectory(const FilePath& from_path,
« base/file_util_posix.cc ('K') | « base/file_util_unittest.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698