From b4574904f9cd0d6f7147950c02ee2447569d0be9 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Wed, 21 Feb 2024 20:20:02 +0000 Subject: [PATCH] [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:https://github.com/dart-lang/sdk/issues/53848 Bug:https://github.com/dart-lang/sdk/issues/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 Reviewed-by: Siva Annamalai --- runtime/bin/BUILD.gn | 3 + runtime/bin/file_win.cc | 78 +++++++++++++++----- tests/standalone/io/file_long_path_test.dart | 50 ++++++++----- tests/standalone/io/link_async_test.dart | 31 +++++++- tests/standalone/io/link_test.dart | 26 +++++++ 5 files changed, 150 insertions(+), 38 deletions(-) diff --git a/runtime/bin/BUILD.gn b/runtime/bin/BUILD.gn index 5ca79ec53eb4..e7741df819c5 100644 --- 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 index c0b2438fe248..d948ebd41a5d 100644 --- a/runtime/bin/file_win.cc +++ b/runtime/bin/file_win.cc @@ -12,6 +12,7 @@ #include // NOLINT #include // NOLINT #include // NOLINT +#include // NOLINT #include // NOLINT #undef StrDup // defined in Shlwapi.h as StrDupW #include // NOLINT @@ -589,10 +590,6 @@ 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) { @@ -600,11 +597,62 @@ bool File::CreateLink(Namespace* namespc, 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,9 +1168,8 @@ 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( @@ -1130,17 +1177,12 @@ File::Type File::GetType(Namespace* namespc, 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) @@ -1148,12 +1190,12 @@ File::Type File::GetType(Namespace* namespc, : 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 index 454e633d7857..2ba7c8e0178d 100644 --- 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 index e23e812f7de7..6e39dcb9fd60 100644 --- 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 index cb0700b8243f..96d5a3f28c43 100644 --- 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(); }