Reland "File::Copy avoids direct copying on Windows"

This is a reland of 391d3bc

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: #42119
> Change-Id: I39b6d451f6ace970bc554501148259d33de232c7
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/149667
> Commit-Queue: Zichang Guo <[email protected]>
> Reviewed-by: Zach Anderson <[email protected]>

Bug: #42119
Change-Id: I58c3aa432d3f64bddb1deace4c9a1ceb2f0f5e16
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151035
Commit-Queue: Zichang Guo <[email protected]>
Reviewed-by: Zach Anderson <[email protected]>

Author: zichangg

runtime/bin/file_win.cc

@@ -18,6 +18,7 @@
 #include <sys/utime.h>  // 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<wchar_t*>(
+      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<uint8_t*>(&suffix_bytes))) {
+    PathBuffer buffer;
+    char* dir = reinterpret_cast<char*>(
+        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<char*>(
+      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<wchar_t*>(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) {

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();
 }

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();
 }