From 42fcc18fccf6f04c0ecfbcfe1312e429573714ca Mon Sep 17 00:00:00 2001 From: Zichang Guo Date: Tue, 7 Jul 2020 20:11:00 +0000 Subject: [PATCH] Reland "File::Copy avoids direct copying on Windows" This is a reland of 391d3bcdd861efde45bd77e9cbdce814cbda3762 The root cause of the failure is that the destination path uses forward slashes as path separator. This fix checks for forward slashes as a potential path separators. If both forward and back slashes exist, the one closer to the end of the path will be used to get the destination directory. If any step fails, it will fall back to original copy. Original change's description: > File::Copy avoids direct copying on Windows > > There is a race condition for copying file on Windows, where CopyFile() > returns success but data has not been populated into destination file. > E.g process is killed or died in the middle. > > This cl will change File::Copy as > 1. Copy file to a temp file in the same directory of destination file. > 2. Rename the file to the target file. > > Bug: https://github.com/dart-lang/sdk/issues/42119 > Change-Id: I39b6d451f6ace970bc554501148259d33de232c7 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/149667 > Commit-Queue: Zichang Guo > Reviewed-by: Zach Anderson Bug: https://github.com/dart-lang/sdk/issues/42119 Change-Id: I58c3aa432d3f64bddb1deace4c9a1ceb2f0f5e16 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151035 Commit-Queue: Zichang Guo Reviewed-by: Zach Anderson --- runtime/bin/file_win.cc | 123 +++++++++++++++++++++- tests/standalone/io/file_copy_test.dart | 32 ++++++ tests/standalone_2/io/file_copy_test.dart | 32 ++++++ 3 files changed, 182 insertions(+), 5 deletions(-) diff --git a/runtime/bin/file_win.cc b/runtime/bin/file_win.cc index 58a9542eeae..80238146ae2 100644 --- a/runtime/bin/file_win.cc +++ b/runtime/bin/file_win.cc @@ -18,6 +18,7 @@ #include // NOLINT #include "bin/builtin.h" +#include "bin/crypto.h" #include "bin/directory.h" #include "bin/namespace.h" #include "bin/utils.h" @@ -517,6 +518,100 @@ bool File::RenameLink(Namespace* namespc, return (move_status != 0); } +static wchar_t* CopyToDartScopeString(wchar_t* string) { + wchar_t* wide_path = reinterpret_cast( + Dart_ScopeAllocate(MAX_PATH * sizeof(wchar_t) + 1)); + wcscpy(wide_path, string); + return wide_path; +} + +static wchar_t* CopyIntoTempFile(const char* src, const char* dest) { + // This function will copy the file to a temp file in the destination + // directory and return the path of temp file. + // Creating temp file name has the same logic as Directory::CreateTemp(), + // which tries with the rng and falls back to a uuid if it failed. + const char* last_back_slash = strrchr(dest, '\\'); + // It is possible the path uses forwardslash as path separator. + const char* last_forward_slash = strrchr(dest, '/'); + const char* last_path_separator = NULL; + if (last_back_slash == NULL && last_forward_slash == NULL) { + return NULL; + } else if (last_forward_slash != NULL && last_forward_slash != NULL) { + // If both types occur in the path, use the one closer to the end. + if (last_back_slash - dest > last_forward_slash - dest) { + last_path_separator = last_back_slash; + } else { + last_path_separator = last_forward_slash; + } + } else { + last_path_separator = + (last_forward_slash == NULL) ? last_back_slash : last_forward_slash; + } + int length_of_parent_dir = last_path_separator - dest + 1; + if (length_of_parent_dir + 8 > MAX_PATH) { + return NULL; + } + uint32_t suffix_bytes = 0; + const int kSuffixSize = sizeof(suffix_bytes); + if (Crypto::GetRandomBytes(kSuffixSize, + reinterpret_cast(&suffix_bytes))) { + PathBuffer buffer; + char* dir = reinterpret_cast( + Dart_ScopeAllocate(1 + sizeof(char) * length_of_parent_dir)); + memmove(dir, dest, length_of_parent_dir); + dir[length_of_parent_dir] = '\0'; + if (!buffer.Add(dir)) { + return NULL; + } + + char suffix[8 + 1]; + Utils::SNPrint(suffix, sizeof(suffix), "%x", suffix_bytes); + Utf8ToWideScope source_path(src); + if (!buffer.Add(suffix)) { + return NULL; + } + if (CopyFileExW(source_path.wide(), buffer.AsStringW(), NULL, NULL, NULL, + 0) != 0) { + return CopyToDartScopeString(buffer.AsStringW()); + } + // If CopyFileExW() fails to copy to a temp file with random hex, fall + // back to copy to a uuid temp file. + } + // UUID has a total of 36 characters in the form of + // xxxxxxxx-xxxx-Mxxx-Nxxx-xxxxxxxxxxxx. + if (length_of_parent_dir + 36 > MAX_PATH) { + return NULL; + } + UUID uuid; + RPC_STATUS status = UuidCreateSequential(&uuid); + if ((status != RPC_S_OK) && (status != RPC_S_UUID_LOCAL_ONLY)) { + return NULL; + } + RPC_WSTR uuid_string; + status = UuidToStringW(&uuid, &uuid_string); + if (status != RPC_S_OK) { + return NULL; + } + PathBuffer buffer; + char* dir = reinterpret_cast( + Dart_ScopeAllocate(1 + sizeof(char) * length_of_parent_dir)); + memmove(dir, dest, length_of_parent_dir); + dir[length_of_parent_dir] = '\0'; + Utf8ToWideScope dest_path(dir); + if (!buffer.AddW(dest_path.wide()) || + !buffer.AddW(reinterpret_cast(uuid_string))) { + return NULL; + } + + RpcStringFreeW(&uuid_string); + Utf8ToWideScope source_path(src); + if (CopyFileExW(source_path.wide(), buffer.AsStringW(), NULL, NULL, NULL, + 0) != 0) { + return CopyToDartScopeString(buffer.AsStringW()); + } + return NULL; +} + bool File::Copy(Namespace* namespc, const char* old_path, const char* new_path) { @@ -525,11 +620,29 @@ bool File::Copy(Namespace* namespc, SetLastError(ERROR_FILE_NOT_FOUND); return false; } - Utf8ToWideScope system_old_path(old_path); - Utf8ToWideScope system_new_path(new_path); - bool success = CopyFileExW(system_old_path.wide(), system_new_path.wide(), - NULL, NULL, NULL, 0) != 0; - return success; + + wchar_t* temp_file = CopyIntoTempFile(old_path, new_path); + if (temp_file == NULL) { + // If temp file creation fails, fall back on doing a direct copy. + Utf8ToWideScope system_old_path(old_path); + Utf8ToWideScope system_new_path(new_path); + return CopyFileExW(system_old_path.wide(), system_new_path.wide(), NULL, + NULL, NULL, 0) != 0; + } + Utf8ToWideScope system_new_dest(new_path); + + // Remove the existing file. Otherwise, renaming will fail. + if (Exists(namespc, new_path)) { + DeleteFileW(system_new_dest.wide()); + } + + if (!MoveFileW(temp_file, system_new_dest.wide())) { + DWORD error = GetLastError(); + DeleteFileW(temp_file); + SetLastError(error); + return false; + } + return true; } int64_t File::LengthFromPath(Namespace* namespc, const char* name) { diff --git a/tests/standalone/io/file_copy_test.dart b/tests/standalone/io/file_copy_test.dart index 2302a4a037b..b1c07238178 100644 --- a/tests/standalone/io/file_copy_test.dart +++ b/tests/standalone/io/file_copy_test.dart @@ -30,6 +30,14 @@ void testCopySync() { Expect.equals(FILE_CONTENT2, file1.readAsStringSync()); Expect.equals(FILE_CONTENT2, file2.readAsStringSync()); + // Check there is no temporary files existing. + var list = tmp.listSync(); + Expect.equals(2, list.length); + for (var file in list) { + final fileName = file.path.toString(); + Expect.isTrue(fileName.contains("file1") || fileName.contains("file2")); + } + // Fail when coping to directory. var dir = new Directory('${tmp.path}/dir')..createSync(); Expect.throws(() => file1.copySync(dir.path)); @@ -38,6 +46,28 @@ void testCopySync() { tmp.deleteSync(recursive: true); } +void testWithForwardSlashes() { + if (Platform.isWindows) { + final tmp = Directory.systemTemp.createTempSync('dart-file-copy'); + + final file1 = File('${tmp.path}/file1'); + file1.writeAsStringSync(FILE_CONTENT1); + Expect.equals(FILE_CONTENT1, file1.readAsStringSync()); + + // Test with a path contains only forward slashes. + final dest = tmp.path.toString().replaceAll("\\", "/"); + final file2 = file1.copySync('${dest}/file2'); + Expect.equals(FILE_CONTENT1, file2.readAsStringSync()); + + // Test with a path mixing both forward and backward slashes. + final file3 = file1.copySync('${dest}\\file3'); + Expect.equals(FILE_CONTENT1, file3.readAsStringSync()); + + // Clean up the directory + tmp.deleteSync(recursive: true); + } +} + void testCopy() { asyncStart(); var tmp = Directory.systemTemp.createTempSync('dart-file-copy'); @@ -76,4 +106,6 @@ void testCopy() { main() { testCopySync(); testCopy(); + // This is Windows only test. + testWithForwardSlashes(); } diff --git a/tests/standalone_2/io/file_copy_test.dart b/tests/standalone_2/io/file_copy_test.dart index 2302a4a037b..b1c07238178 100644 --- a/tests/standalone_2/io/file_copy_test.dart +++ b/tests/standalone_2/io/file_copy_test.dart @@ -30,6 +30,14 @@ void testCopySync() { Expect.equals(FILE_CONTENT2, file1.readAsStringSync()); Expect.equals(FILE_CONTENT2, file2.readAsStringSync()); + // Check there is no temporary files existing. + var list = tmp.listSync(); + Expect.equals(2, list.length); + for (var file in list) { + final fileName = file.path.toString(); + Expect.isTrue(fileName.contains("file1") || fileName.contains("file2")); + } + // Fail when coping to directory. var dir = new Directory('${tmp.path}/dir')..createSync(); Expect.throws(() => file1.copySync(dir.path)); @@ -38,6 +46,28 @@ void testCopySync() { tmp.deleteSync(recursive: true); } +void testWithForwardSlashes() { + if (Platform.isWindows) { + final tmp = Directory.systemTemp.createTempSync('dart-file-copy'); + + final file1 = File('${tmp.path}/file1'); + file1.writeAsStringSync(FILE_CONTENT1); + Expect.equals(FILE_CONTENT1, file1.readAsStringSync()); + + // Test with a path contains only forward slashes. + final dest = tmp.path.toString().replaceAll("\\", "/"); + final file2 = file1.copySync('${dest}/file2'); + Expect.equals(FILE_CONTENT1, file2.readAsStringSync()); + + // Test with a path mixing both forward and backward slashes. + final file3 = file1.copySync('${dest}\\file3'); + Expect.equals(FILE_CONTENT1, file3.readAsStringSync()); + + // Clean up the directory + tmp.deleteSync(recursive: true); + } +} + void testCopy() { asyncStart(); var tmp = Directory.systemTemp.createTempSync('dart-file-copy'); @@ -76,4 +106,6 @@ void testCopy() { main() { testCopySync(); testCopy(); + // This is Windows only test. + testWithForwardSlashes(); }