[io] Fix a bug where relative symlinks to directories did not work on…

… Windows. Also modified `File::GetType` on Windows to correctly report the type of broken links as notFound. Windows fixes: co19/LibTest/io/Link/createSync_A04_t08 co19/LibTest/io/Link/createSync_A04_t10 co19/LibTest/io/Link/createSync_A04_t12 co19/LibTest/io/Link/createSync_A04_t14 co19/LibTest/io/Link/createSync_A04_t16 co19/LibTest/io/Link/createSync_A06_t01 co19/LibTest/io/Link/createSync_A06_t03 co19/LibTest/io/Link/createSync_A06_t06 co19/LibTest/io/Link/createSync_A06_t08 co19/LibTest/io/Link/create_A04_t08 co19/LibTest/io/Link/create_A04_t10 co19/LibTest/io/Link/create_A04_t12 co19/LibTest/io/Link/create_A04_t14 co19/LibTest/io/Link/create_A04_t16 co19/LibTest/io/Link/create_A06_t01 co19/LibTest/io/Link/create_A06_t03 co19/LibTest/io/Link/create_A06_t06 co19/LibTest/io/Link/create_A06_t08 co19/LibTest/io/Link/resolveSymbolicLinksSync_A01_t01 co19/LibTest/io/Link/resolveSymbolicLinks_A01_t01 Bug:#53848 Bug:#45981 Change-Id: I3d156f38540089d8adb12dbb79d0477330d9eb07 Tested: updated unit tests plus fixes existing tests Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/335940 Commit-Queue: Brian Quinlan <[email protected]> Reviewed-by: Siva Annamalai <[email protected]>
dart-lang · Feb 21, 2024 · b457490 · b457490
1 parent d67f159
commit b457490
Show file tree

Hide file tree

Showing 5 changed files with 150 additions and 38 deletions.
diff --git a/runtime/bin/BUILD.gn b/runtime/bin/BUILD.gn
@@ -228,6 +228,7 @@ template("build_gen_snapshot") {
       libs = [
         "iphlpapi.lib",
         "ws2_32.lib",
+        "Pathcch.lib",
         "Rpcrt4.lib",
         "shlwapi.lib",
         "winmm.lib",
@@ -814,6 +815,7 @@ template("dart_executable") {
         # CoTaskMemAlloc up with `DynamicLibrary.process()`.
         "ole32.lib",
         "iphlpapi.lib",
+        "Pathcch.lib",
         "psapi.lib",
         "ws2_32.lib",
         "Rpcrt4.lib",
@@ -1032,6 +1034,7 @@ executable("run_vm_tests") {
       # CoTaskMemAlloc up with `DynamicLibrary.process()`.
       "ole32.lib",
       "iphlpapi.lib",
+      "Pathcch.lib",
       "psapi.lib",
       "ws2_32.lib",
       "Rpcrt4.lib",

diff --git a/runtime/bin/file_win.cc b/runtime/bin/file_win.cc
@@ -12,6 +12,7 @@
 #include <Shlwapi.h>  // NOLINT
 #include <fcntl.h>     // NOLINT
 #include <io.h>        // NOLINT
+#include <pathcch.h>   // NOLINT
 #include <winioctl.h>  // NOLINT
 #undef StrDup          // defined in Shlwapi.h as StrDupW
 #include <stdio.h>     // NOLINT
@@ -589,22 +590,69 @@ typedef struct _REPARSE_DATA_BUFFER {
   };
 } REPARSE_DATA_BUFFER, *PREPARSE_DATA_BUFFER;
 
-static constexpr int kReparseDataHeaderSize =
-    sizeof(ULONG) + 2 * sizeof(USHORT);
-static constexpr int kMountPointHeaderSize = 4 * sizeof(USHORT);
-
 bool File::CreateLink(Namespace* namespc,
                       const char* utf8_name,
                       const char* utf8_target) {
   Utf8ToWideScope name(PrefixLongFilePath(utf8_name));
   Utf8ToWideScope target(PrefixLongFilePath(utf8_target));
   DWORD flags = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE;
 
-  File::Type type = File::GetType(namespc, utf8_target, true);
+  File::Type type;
+  if (File::IsAbsolutePath(utf8_target)) {
+    type = File::GetType(namespc, utf8_target, true);
+  } else {
+    // The path of `target` is relative to `name`.
+    //
+    // To determine if `target` is a file or directory, we need to calculate
+    // either its absolute path or its path relative to the current working
+    // directory.
+    //
+    // For example:
+    //
+    // name=           C:\A\B\Link      ..\..\Link      ..\..\Link
+    // target=         MyFile           MyFile          ..\Dir\MyFile
+    // --------------------------------------------------------------------
+    // target_path=    C:\A\B\MyFile    ..\..\MyFile    ..\..\..\Dir\MyFile
+    //
+    // The transformation steps are:
+    // 1. target_path := name                           ..\..\Link
+    // 2. target_path := remove_file(target_path)       ..\..\
+    // 3. target_path := combine(target_path, target)   ..\..\..\Dir\MyFile
+
+    // 1. target_path := name
+    intptr_t target_path_max_length = name.length() + target.length();
+    Wchart target_path(target_path_max_length);
+    wcscpy_s(target_path.buf(), target_path_max_length, name.wide());
+
+    // 2. target_path := remove_file(target_path)
+    HRESULT remove_result =
+        PathCchRemoveFileSpec(target_path.buf(), target_path_max_length);
+    if (remove_result == S_FALSE) {
+      // If the file component could not be removed, then `name` is
+      // top-level, like "C:\" or "/". Attempts to create files at those paths
+      // will fail with ERROR_ACCESS_DENIED.
+      SetLastError(ERROR_ACCESS_DENIED);
+      return false;
+    } else if (remove_result != S_OK) {
+      SetLastError(remove_result);
+      return false;
+    }
+
+    // 3. target_path := combine(target_path, target)
+    HRESULT combine_result = PathCchCombineEx(
+        target_path.buf(), target_path_max_length, target_path.buf(),
+        target.wide(), PATHCCH_ALLOW_LONG_PATHS);
+    if (combine_result != S_OK) {
+      SetLastError(combine_result);
+      return false;
+    }
+
+    type = PathIsDirectoryW(target_path.buf()) ? kIsDirectory : kIsFile;
+  }
+
   if (type == kIsDirectory) {
     flags |= SYMBOLIC_LINK_FLAG_DIRECTORY;
   }
-
   int create_status = CreateSymbolicLinkW(name.wide(), target.wide(), flags);
 
   // If running on a Windows 10 build older than 14972, an invalid parameter
@@ -1120,40 +1168,34 @@ File::Type File::GetType(Namespace* namespc,
   // Convert to wchar_t string.
   Utf8ToWideScope name(prefixed_path);
   DWORD attributes = GetFileAttributesW(name.wide());
-  File::Type result = kIsFile;
   if (attributes == INVALID_FILE_ATTRIBUTES) {
-    result = kDoesNotExist;
+    return kDoesNotExist;
   } else if ((attributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
     if (follow_links) {
       HANDLE target_handle = CreateFileW(
           name.wide(), 0,
           FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr,
           OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr);
       if (target_handle == INVALID_HANDLE_VALUE) {
-        DWORD last_error = GetLastError();
-        if ((last_error == ERROR_FILE_NOT_FOUND) ||
-            (last_error == ERROR_PATH_NOT_FOUND)) {
-          return kDoesNotExist;
-        }
-        result = kIsLink;
+        return kDoesNotExist;
       } else {
         BY_HANDLE_FILE_INFORMATION info;
         if (!GetFileInformationByHandle(target_handle, &info)) {
           CloseHandle(target_handle);
-          return File::kIsLink;
+          return File::kDoesNotExist;
         }
         CloseHandle(target_handle);
         return ((info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0)
                    ? kIsDirectory
                    : kIsFile;
       }
     } else {
-      result = kIsLink;
+      return kIsLink;
     }
   } else if ((attributes & FILE_ATTRIBUTE_DIRECTORY) != 0) {
-    result = kIsDirectory;
+    return kIsDirectory;
   }
-  return result;
+  return kIsFile;
 }
 
 File::Identical File::AreIdentical(Namespace* namespc_1,

diff --git a/tests/standalone/io/file_long_path_test.dart b/tests/standalone/io/file_long_path_test.dart
@@ -109,31 +109,44 @@ void testFileStat(String dir) {
 }
 
 void testCreateLinkToDir(String dir) {
-  final path = '${dir}\\a_long_path_linkname';
-  Expect.isTrue(path.length > maxPath);
-  var target = '$dir\\a_long_path_target';
-  final link = Link(path)..createSync(target);
+  final linkPath = '$dir\\a_long_path_linkname';
+  final renamedPath = '$dir\\a_long_renamed_path_linkname';
 
-  final dest = Directory(target)..createSync();
-  Expect.isTrue(dest.existsSync());
+  Expect.isTrue(linkPath.length > maxPath);
+  Expect.isTrue(renamedPath.length > maxPath);
+
+  var targetDirectory1 = '$dir\\a_long_directory_target1';
+  var targetDirectory2 = '$dir\\a_long_directory_target2';
 
+  Directory(targetDirectory1).createSync();
+  Directory(targetDirectory2).createSync();
+
+  // Create link
+  final link = Link(linkPath)..createSync(targetDirectory1);
   Expect.isTrue(link.existsSync());
-  Expect.isTrue(link.targetSync().contains('a_long_path_target'));
+  final resolvedCreatePath = link.resolveSymbolicLinksSync();
+  Expect.isTrue(
+      FileSystemEntity.identicalSync(targetDirectory1, resolvedCreatePath),
+      '${link.path} should resolve to $targetDirectory1 but resolved to $resolvedCreatePath');
 
   // Rename link
-  var renamedLink = link.renameSync('${dir}\\a_renamed_long_path_link');
+  var renamedLink = link.renameSync(renamedPath);
   Expect.isTrue(renamedLink.existsSync());
   Expect.isFalse(link.existsSync());
-  Expect.isTrue(renamedLink.targetSync().contains('a_long_path_target'));
+  final resolvedRenamePath = renamedLink.resolveSymbolicLinksSync();
+  Expect.isTrue(
+      FileSystemEntity.identicalSync(targetDirectory1, resolvedRenamePath),
+      '${link.path} should resolve to $targetDirectory1 but resolved to $resolvedRenamePath');
 
   // Update link target
-  target = '$dir\\an_updated_target';
-  final renamedDest = Directory(target)..createSync();
-  renamedLink.updateSync(target);
-  Expect.isTrue(renamedLink.targetSync().contains('an_updated_target'));
-
-  dest.deleteSync();
-  renamedDest.deleteSync();
+  renamedLink.updateSync(targetDirectory2);
+  final resolvedUpdatedPath = renamedLink.resolveSymbolicLinksSync();
+  Expect.isTrue(
+      FileSystemEntity.identicalSync(targetDirectory2, resolvedUpdatedPath),
+      '${link.path} should resolve to $targetDirectory2 but resolved to $resolvedRenamePath');
+
+  Directory(targetDirectory1).deleteSync();
+  Directory(targetDirectory2).deleteSync();
   renamedLink.deleteSync();
 }
 
@@ -147,8 +160,9 @@ void testCreateLinkToFile(String dir) {
   Expect.isTrue(dest.existsSync());
 
   Expect.isTrue(link.existsSync());
-  Expect.isTrue(link.targetSync().contains('a_long_path_target'));
-  Expect.isTrue(link.resolveSymbolicLinksSync().contains('a_long_path_target'));
+  final resolvedPath = link.resolveSymbolicLinksSync();
+  Expect.isTrue(FileSystemEntity.identicalSync(target, resolvedPath),
+      '${link.path} should resolve to $target but resolved to $resolvedPath');
 
   // Rename link
   var renamedLink = link.renameSync('${dir}\\a_renamed_long_path_link');

diff --git a/tests/standalone/io/link_async_test.dart b/tests/standalone/io/link_async_test.dart
@@ -280,7 +280,22 @@ Future testRelativeLinks(_) {
   });
 }
 
-Future testBrokenLinkTypeSync(_) async {
+Future testRelativeLinkToDirectoryNotRelativeToCurrentWorkingDirectory(
+    _) async {
+  final tempDirectory = await Directory.systemTemp.createTemp('dart_link');
+  final dir2 = await Directory(join(tempDirectory.path, 'dir1', 'dir2'))
+      .create(recursive: true);
+
+  final link =
+      await Link(join(tempDirectory.path, 'link')).create(join('dir1', 'dir2'));
+
+  String resolvedDir2Path = await link.resolveSymbolicLinks();
+  Expect.isTrue(await FileSystemEntity.identical(dir2.path, resolvedDir2Path));
+
+  await tempDirectory.delete(recursive: true);
+}
+
+Future testBrokenLinkType(_) async {
   String base = (await Directory.systemTemp.createTemp('dart_link')).path;
   String link = join(base, 'link');
   await Link(link).create('does not exist');
@@ -292,12 +307,24 @@ Future testBrokenLinkTypeSync(_) async {
       await FileSystemEntity.type(link, followLinks: true));
 }
 
+Future testTopLevelLink(_) async {
+  if (!Platform.isWindows) return;
+  try {
+    await Link(r"C:\").create('the target does not matter');
+    Expect.fail("expected FileSystemException");
+  } on FileSystemException catch (e) {
+    Expect.equals(5, e.osError!.errorCode); // ERROR_ACCESS_DENIED
+  }
+}
+
 main() {
   asyncStart();
   testCreate()
       .then(testCreateLoopingLink)
       .then(testRename)
       .then(testRelativeLinks)
-      .then(testBrokenLinkTypeSync)
+      .then(testRelativeLinkToDirectoryNotRelativeToCurrentWorkingDirectory)
+      .then(testBrokenLinkType)
+      .then(testTopLevelLink)
       .then((_) => asyncEnd());
 }
diff --git a/tests/standalone/io/link_test.dart b/tests/standalone/io/link_test.dart
@@ -275,6 +275,20 @@ testRelativeLinksSync() {
   tempDirectory.deleteSync(recursive: true);
 }
 
+void testRelativeLinkToDirectoryNotRelativeToCurrentWorkingDirectory() {
+  final tempDirectory = Directory.systemTemp.createTempSync('dart_link');
+  final dir2 = Directory(join(tempDirectory.path, 'dir1', 'dir2'))
+    ..createSync(recursive: true);
+
+  final link = Link(join(tempDirectory.path, 'link'))
+    ..createSync(join('dir1', 'dir2'));
+
+  String resolvedDir2Path = link.resolveSymbolicLinksSync();
+  Expect.isTrue(FileSystemEntity.identicalSync(dir2.path, resolvedDir2Path));
+
+  tempDirectory.deleteSync(recursive: true);
+}
+
 testIsDir() async {
   // Only run on Platforms that supports file watcher
   if (!Platform.isWindows && !Platform.isLinux && !Platform.isMacOS) return;
@@ -326,12 +340,24 @@ testBrokenLinkTypeSync() {
       FileSystemEntity.typeSync(link, followLinks: true));
 }
 
+void testTopLevelLinkSync() {
+  if (!Platform.isWindows) return;
+  try {
+    Link(r"C:\").createSync('the target does not matter');
+    Expect.fail("expected FileSystemException");
+  } on FileSystemException catch (e) {
+    Expect.equals(5, e.osError!.errorCode); // ERROR_ACCESS_DENIED
+  }
+}
+
 main() {
   testCreateSync();
   testCreateLoopingLink();
   testRenameSync();
   testLinkErrorSync();
   testRelativeLinksSync();
+  testRelativeLinkToDirectoryNotRelativeToCurrentWorkingDirectory();
   testIsDir();
   testBrokenLinkTypeSync();
+  testTopLevelLinkSync();
 }