From 188bfd8a752a13061e56d7539579c92ab4e5c85c Mon Sep 17 00:00:00 2001 From: =?utf8?q?David=20Cant=C3=BA?= Date: Mon, 13 Sep 2021 09:44:10 -0700 Subject: [PATCH] Address Windows recommendations for Link APIs (#58592) (#58926) * NT prefix shall not be treated as a relative path * Remove file-folder-match validation * Use SubstituteName instead of PrintName since the latter is just for display and can be misleading * Fix MS.IO.Redist * Fix whitespace in Strings.resx * Address suggestions --- .../SymbolicLinks/BaseSymbolicLinks.FileSystem.cs | 93 ++++------------------ .../BaseSymbolicLinks.FileSystemInfo.cs | 15 +--- .../tests/Base/SymbolicLinks/BaseSymbolicLinks.cs | 92 +++++++++++++++++++++ .../tests/Junctions.Windows.cs | 42 +++++++++- .../src/Resources/Strings.resx | 3 - .../src/System/IO/FileSystem.Unix.cs | 10 --- .../src/System/IO/FileSystem.Windows.cs | 64 ++++++++------- .../src/System/IO/PathInternal.Windows.cs | 2 + 8 files changed, 186 insertions(+), 135 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystem.cs b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystem.cs index e3b9027..ffecf93 100644 --- a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystem.cs +++ b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystem.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections.Generic; using System.IO.Enumeration; using System.Linq; using Xunit; @@ -158,7 +157,7 @@ namespace System.IO.Tests } [Theory] - [MemberData(nameof(ResolveLinkTarget_PathToTarget_Data))] + [MemberData(nameof(SymbolicLink_ResolveLinkTarget_PathToTarget_Data))] public void ResolveLinkTarget_Succeeds(string pathToTarget, bool returnFinalTarget) { string linkPath = GetRandomLinkPath(); @@ -345,32 +344,6 @@ namespace System.IO.Tests } [Fact] - public void CreateSymbolicLink_WrongTargetType_Throws() - { - // dirLink -> file - // fileLink -> dir - - string targetPath = GetRandomFilePath(); - CreateFileOrDirectory(targetPath, createOpposite: true); // The underlying file system entry needs to be different - Assert.Throws(() => CreateSymbolicLink(GetRandomFilePath(), targetPath)); - } - - [Fact] - public void CreateSymbolicLink_WrongTargetType_Indirect_Throws() - { - // link-2 (dir) -> link-1 (file) -> file - // link-2 (file) -> link-1 (dir) -> dir - string targetPath = GetRandomFilePath(); - string firstLinkPath = GetRandomFilePath(); - string secondLinkPath = GetRandomFilePath(); - - CreateFileOrDirectory(targetPath, createOpposite: true); - CreateSymbolicLink_Opposite(firstLinkPath, targetPath); - - Assert.Throws(() => CreateSymbolicLink(secondLinkPath, firstLinkPath)); - } - - [Fact] public void CreateSymbolicLink_CorrectTargetType_Indirect_Succeeds() { // link-2 (file) -> link-1 (file) -> file @@ -429,14 +402,14 @@ namespace System.IO.Tests Assert.True(link2Info.Exists); Assert.True(link2Info.Attributes.HasFlag(FileAttributes.ReparsePoint)); AssertIsCorrectTypeAndDirectoryAttribute(link2Info); - Assert.Equal(link2Target, link2Info.LinkTarget); + AssertPathEquals_RelativeSegments(link2Target, link2Info.LinkTarget); // link1 to link2 FileSystemInfo link1Info = CreateSymbolicLink(link1Path, link1Target); Assert.True(link1Info.Exists); Assert.True(link1Info.Attributes.HasFlag(FileAttributes.ReparsePoint)); AssertIsCorrectTypeAndDirectoryAttribute(link1Info); - Assert.Equal(link1Target, link1Info.LinkTarget); + AssertPathEquals_RelativeSegments(link1Target, link1Info.LinkTarget); // link1: do not follow symlinks FileSystemInfo link1TargetInfo = ResolveLinkTarget(link1Path, returnFinalTarget: false); @@ -444,7 +417,7 @@ namespace System.IO.Tests AssertIsCorrectTypeAndDirectoryAttribute(link1TargetInfo); Assert.True(link1TargetInfo.Attributes.HasFlag(FileAttributes.ReparsePoint)); Assert.Equal(link2Path, link1TargetInfo.FullName); - Assert.Equal(link2Target, link1TargetInfo.LinkTarget); + AssertPathEquals_RelativeSegments(link2Target, link1TargetInfo.LinkTarget); // link2: do not follow symlinks FileSystemInfo link2TargetInfo = ResolveLinkTarget(link2Path, returnFinalTarget: false); @@ -460,6 +433,19 @@ namespace System.IO.Tests AssertIsCorrectTypeAndDirectoryAttribute(finalTarget); Assert.False(finalTarget.Attributes.HasFlag(FileAttributes.ReparsePoint)); Assert.Equal(filePath, finalTarget.FullName); + + void AssertPathEquals_RelativeSegments(string expected, string actual) + { +#if WINDOWS + // DeviceIoControl canonicalizes the target path i.e: removes redundant segments. + int rootLength = PathInternal.GetRootLength(expected); + if (rootLength > 0) + { + expected = PathInternal.RemoveRelativeSegments(expected, rootLength); + } +#endif + Assert.Equal(expected, actual); + } } // Must call inside a remote executor @@ -511,50 +497,5 @@ namespace System.IO.Tests (entry.Attributes & FileAttributes.ReparsePoint) != 0 }.FirstOrDefault(); } - - public static IEnumerable ResolveLinkTarget_PathToTarget_Data - { - get - { - foreach (string path in PathToTargetData) - { - yield return new object[] { path, false }; - yield return new object[] { path, true }; - } - } - } - - internal static IEnumerable PathToTargetData - { - get - { - if (OperatingSystem.IsWindows()) - { - //Non-rooted relative - yield return "foo"; - yield return @".\foo"; - yield return @"..\foo"; - // Rooted relative - yield return @"\foo"; - // Rooted absolute - yield return Path.Combine(Path.GetTempPath(), "foo"); - // Extended DOS - yield return Path.Combine(@"\\?\", Path.GetTempPath(), "foo"); - // UNC - yield return @"\\LOCALHOST\share\path"; - } - else - { - //Non-rooted relative - yield return "foo"; - yield return "./foo"; - yield return "../foo"; - // Rooted relative - yield return "/foo"; - // Rooted absolute - Path.Combine(Path.GetTempPath(), "foo"); - } - } - } } } diff --git a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystemInfo.cs b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystemInfo.cs index e828642..05e18f1 100644 --- a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystemInfo.cs +++ b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystemInfo.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections.Generic; -using System.Diagnostics; using Xunit; namespace System.IO.Tests @@ -54,7 +52,7 @@ namespace System.IO.Tests } [Theory] - [MemberData(nameof(LinkTarget_PathToTarget_Data))] + [MemberData(nameof(SymbolicLink_LinkTarget_PathToTarget_Data))] public void LinkTarget_Succeeds(string pathToTarget) { FileSystemInfo linkInfo = CreateSymbolicLink(GetRandomLinkPath(), pathToTarget); @@ -86,16 +84,5 @@ namespace System.IO.Tests Assert.Equal(newPathToTarget, linkInfo.LinkTarget); Assert.Equal(newLinkInfo.LinkTarget, linkInfo.LinkTarget); } - - public static IEnumerable LinkTarget_PathToTarget_Data - { - get - { - foreach (string path in PathToTargetData) - { - yield return new object[] { path }; - } - } - } } } diff --git a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs index 6e5637d..d506e7e 100644 --- a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs +++ b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; +using System.Linq; using Xunit; namespace System.IO.Tests @@ -45,5 +47,95 @@ namespace System.IO.Tests Directory.SetCurrentDirectory(tempCwd); return tempCwd; } + + public static IEnumerable SymbolicLink_LinkTarget_PathToTarget_Data + { + get + { + foreach (string path in PathToTargetData.Union(PathToTargetUncData)) + { + yield return new object[] { path }; + } + } + } + + public static IEnumerable SymbolicLink_ResolveLinkTarget_PathToTarget_Data + { + get + { + foreach (string path in PathToTargetData.Union(PathToTargetUncData)) + { + yield return new object[] { path, false }; + yield return new object[] { path, true }; + } + } + } + + // Junctions doesn't support remote shares. + public static IEnumerable Junction_LinkTarget_PathToTarget_Data + { + get + { + foreach (string path in PathToTargetData) + { + yield return new object[] { path }; + } + } + } + + public static IEnumerable Junction_ResolveLinkTarget_PathToTarget_Data + { + get + { + foreach (string path in PathToTargetData) + { + yield return new object[] { path, false }; + yield return new object[] { path, true }; + } + } + } + + internal static IEnumerable PathToTargetData + { + get + { + if (OperatingSystem.IsWindows()) + { + //Non-rooted relative + yield return "foo"; + yield return @".\foo"; + yield return @"..\foo"; + // Rooted relative + yield return @"\foo"; + // Rooted absolute + yield return Path.Combine(Path.GetTempPath(), "foo"); + // Extended DOS + yield return Path.Combine(@"\\?\", Path.GetTempPath(), "foo"); + } + else + { + //Non-rooted relative + yield return "foo"; + yield return "./foo"; + yield return "../foo"; + // Rooted relative + yield return "/foo"; + // Rooted absolute + Path.Combine(Path.GetTempPath(), "foo"); + } + } + } + + internal static IEnumerable PathToTargetUncData + { + get + { + if (OperatingSystem.IsWindows()) + { + // UNC/Remote Share + yield return @"\\LOCALHOST\share\path"; + } + } + } } } diff --git a/src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs b/src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs index f4f2150..c280008 100644 --- a/src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections.Generic; -using System.Linq; using Xunit; namespace System.IO.Tests @@ -10,7 +8,7 @@ namespace System.IO.Tests [PlatformSpecific(TestPlatforms.Windows)] public class Junctions : BaseSymbolicLinks { - protected DirectoryInfo CreateJunction(string junctionPath, string targetPath) + private DirectoryInfo CreateJunction(string junctionPath, string targetPath) { Assert.True(MountHelper.CreateJunction(junctionPath, targetPath)); DirectoryInfo junctionInfo = new(junctionPath); @@ -67,5 +65,43 @@ namespace System.IO.Tests Assert.Equal(expectedTargetPath, targetFromDirectoryInfo.FullName); Assert.Equal(expectedTargetPath, targetFromDirectory.FullName); } + + [Theory] + [MemberData(nameof(Junction_ResolveLinkTarget_PathToTarget_Data))] + public void Junction_ResolveLinkTarget_Succeeds(string pathToTarget, bool returnFinalTarget) + { + string linkPath = GetRandomLinkPath(); + FileSystemInfo linkInfo = CreateJunction(linkPath, pathToTarget); + + // Junctions are always created with absolute targets, even if a relative path is passed. + string expectedTarget = Path.GetFullPath(pathToTarget); + + Assert.True(linkInfo.Exists); + Assert.IsType(linkInfo); + Assert.True(linkInfo.Attributes.HasFlag(FileAttributes.Directory)); + Assert.Equal(expectedTarget, linkInfo.LinkTarget); + + FileSystemInfo? targetFromDirectoryInfo = linkInfo.ResolveLinkTarget(returnFinalTarget); + FileSystemInfo? targetFromDirectory = Directory.ResolveLinkTarget(linkPath, returnFinalTarget); + + Assert.NotNull(targetFromDirectoryInfo); + Assert.NotNull(targetFromDirectory); + + Assert.False(targetFromDirectoryInfo.Exists); + Assert.False(targetFromDirectory.Exists); + + + Assert.Equal(expectedTarget, targetFromDirectoryInfo.FullName); + Assert.Equal(expectedTarget, targetFromDirectory.FullName); + } + + [Theory] + [MemberData(nameof(Junction_LinkTarget_PathToTarget_Data))] + public void Junction_LinkTarget_Succeeds(string pathToTarget) + { + FileSystemInfo linkInfo = CreateJunction(GetRandomLinkPath(), pathToTarget); + Assert.True(linkInfo.Exists); + Assert.Equal(Path.GetFullPath(pathToTarget), linkInfo.LinkTarget); + } } } diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 6cd99ca..1e16e6b 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -2659,9 +2659,6 @@ BindHandle for ThreadPool failed on this handle. - - The link's file system entry type is inconsistent with that of its target: {0} - The file '{0}' already exists. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index aab00ed..fc4b2fe 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -571,16 +571,6 @@ namespace System.IO internal static void CreateSymbolicLink(string path, string pathToTarget, bool isDirectory) { string pathToTargetFullPath = PathInternal.GetLinkTargetFullPath(path, pathToTarget); - - // Fail if the target exists but is not consistent with the expected filesystem entry type - if (Interop.Sys.Stat(pathToTargetFullPath, out Interop.Sys.FileStatus targetInfo) == 0) - { - if (isDirectory != ((targetInfo.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR)) - { - throw new IOException(SR.Format(SR.IO_InconsistentLinkType, path)); - } - } - Interop.CheckIo(Interop.Sys.SymLink(pathToTarget, path), path, isDirectory); } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs index 223117e..a233347 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs @@ -415,16 +415,6 @@ namespace System.IO internal static void CreateSymbolicLink(string path, string pathToTarget, bool isDirectory) { string pathToTargetFullPath = PathInternal.GetLinkTargetFullPath(path, pathToTarget); - - Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA data = default; - int errorCode = FillAttributeInfo(pathToTargetFullPath, ref data, returnErrorOnNotFound: true); - if (errorCode == Interop.Errors.ERROR_SUCCESS && - data.dwFileAttributes != -1 && - isDirectory != ((data.dwFileAttributes & Interop.Kernel32.FileAttributes.FILE_ATTRIBUTE_DIRECTORY) != 0)) - { - throw new IOException(SR.Format(SR.IO_InconsistentLinkType, path)); - } - Interop.Kernel32.CreateSymbolicLink(path, pathToTarget, isDirectory); } @@ -502,42 +492,52 @@ namespace System.IO success = MemoryMarshal.TryRead(bufferSpan, out Interop.Kernel32.SymbolicLinkReparseBuffer rbSymlink); Debug.Assert(success); - // We use PrintName(Offset|Length) instead of SubstituteName(Offset|Length) given that we don't want to return - // an NT path when the link wasn't created with such NT path. - // Unlike SubstituteName and GetFinalPathNameByHandle(), PrintName doesn't start with a prefix. - // Another nuance is that SubstituteName does not contain redundant path segments while PrintName does. - // PrintName can ONLY return a NT path if the link was created explicitly targeting a file/folder in such way. - // e.g: mklink /D linkName \??\C:\path\to\target. + // We always use SubstituteName(Offset|Length) instead of PrintName(Offset|Length), + // the latter is just the display name of the reparse point and it can show something completely unrelated to the target. if (rbSymlink.ReparseTag == Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_SYMLINK) { - int printNameOffset = sizeof(Interop.Kernel32.SymbolicLinkReparseBuffer) + rbSymlink.PrintNameOffset; - int printNameLength = rbSymlink.PrintNameLength; + int offset = sizeof(Interop.Kernel32.SymbolicLinkReparseBuffer) + rbSymlink.SubstituteNameOffset; + int length = rbSymlink.SubstituteNameLength; - Span targetPath = MemoryMarshal.Cast(bufferSpan.Slice(printNameOffset, printNameLength)); - Debug.Assert((rbSymlink.Flags & Interop.Kernel32.SYMLINK_FLAG_RELATIVE) == 0 || !PathInternal.IsExtended(targetPath)); + Span targetPath = MemoryMarshal.Cast(bufferSpan.Slice(offset, length)); - if (returnFullPath && (rbSymlink.Flags & Interop.Kernel32.SYMLINK_FLAG_RELATIVE) != 0) + bool isRelative = (rbSymlink.Flags & Interop.Kernel32.SYMLINK_FLAG_RELATIVE) != 0; + if (!isRelative) + { + // Absolute target is in NT format and we need to clean it up before return it to the user. + if (targetPath.StartsWith(PathInternal.UncNTPathPrefix.AsSpan())) + { + // We need to prepend the Win32 equivalent of UNC NT prefix. + return Path.Join(PathInternal.UncPathPrefix.AsSpan(), targetPath.Slice(PathInternal.UncNTPathPrefix.Length)); + } + + return GetTargetPathWithoutNTPrefix(targetPath); + } + else if (returnFullPath) { - // Target path is relative and is for ResolveLinkTarget(), we need to append the link directory. return Path.Join(Path.GetDirectoryName(linkPath.AsSpan()), targetPath); } - - return targetPath.ToString(); + else + { + return targetPath.ToString(); + } } else if (rbSymlink.ReparseTag == Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_MOUNT_POINT) { success = MemoryMarshal.TryRead(bufferSpan, out Interop.Kernel32.MountPointReparseBuffer rbMountPoint); Debug.Assert(success); - int printNameOffset = sizeof(Interop.Kernel32.MountPointReparseBuffer) + rbMountPoint.PrintNameOffset; - int printNameLength = rbMountPoint.PrintNameLength; + int offset = sizeof(Interop.Kernel32.MountPointReparseBuffer) + rbMountPoint.SubstituteNameOffset; + int length = rbMountPoint.SubstituteNameLength; - Span targetPath = MemoryMarshal.Cast(bufferSpan.Slice(printNameOffset, printNameLength)); + Span targetPath = MemoryMarshal.Cast(bufferSpan.Slice(offset, length)); - // Unlike symlinks, mount point paths cannot be relative + // Unlike symbolic links, mount point paths cannot be relative. Debug.Assert(!PathInternal.IsPartiallyQualified(targetPath)); - return targetPath.ToString(); + // Mount points cannot point to a remote location. + Debug.Assert(!targetPath.StartsWith(PathInternal.UncNTPathPrefix.AsSpan())); + return GetTargetPathWithoutNTPrefix(targetPath); } return null; @@ -546,6 +546,12 @@ namespace System.IO { ArrayPool.Shared.Return(buffer); } + + static string GetTargetPathWithoutNTPrefix(ReadOnlySpan targetPath) + { + Debug.Assert(targetPath.StartsWith(PathInternal.NTPathPrefix.AsSpan())); + return targetPath.Slice(PathInternal.NTPathPrefix.Length).ToString(); + } } private static unsafe string? GetFinalLinkTarget(string linkPath, bool isDirectory) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/PathInternal.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/PathInternal.Windows.cs index b1767b3..48fb353 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/PathInternal.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/PathInternal.Windows.cs @@ -46,10 +46,12 @@ namespace System.IO internal const string DirectorySeparatorCharAsString = "\\"; + internal const string NTPathPrefix = @"\??\"; internal const string ExtendedPathPrefix = @"\\?\"; internal const string UncPathPrefix = @"\\"; internal const string UncExtendedPrefixToInsert = @"?\UNC\"; internal const string UncExtendedPathPrefix = @"\\?\UNC\"; + internal const string UncNTPathPrefix = @"\??\UNC\"; internal const string DevicePathPrefix = @"\\.\"; internal const string ParentDirectoryPrefix = @"..\"; -- 2.7.4