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, |