Address Windows recommendations for Link APIs (#58592) (#58926)
authorDavid Cantú <dacantu@microsoft.com>
Mon, 13 Sep 2021 16:44:10 +0000 (09:44 -0700)
committerGitHub <noreply@github.com>
Mon, 13 Sep 2021 16:44:10 +0000 (09:44 -0700)
* 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

src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystem.cs
src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystemInfo.cs
src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs
src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs
src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs
src/libraries/System.Private.CoreLib/src/System/IO/PathInternal.Windows.cs

index e3b9027..ffecf93 100644 (file)
@@ -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<IOException>(() => 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<IOException>(() => 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<object[]> ResolveLinkTarget_PathToTarget_Data
-        {
-            get
-            {
-                foreach (string path in PathToTargetData)
-                {
-                    yield return new object[] { path, false };
-                    yield return new object[] { path, true };
-                }
-            }
-        }
-
-        internal static IEnumerable<string> 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");
-                }
-            }
-        }
     }
 }
index e828642..05e18f1 100644 (file)
@@ -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<object[]> LinkTarget_PathToTarget_Data
-        {
-            get
-            {
-                foreach (string path in PathToTargetData)
-                {
-                    yield return new object[] { path };
-                }
-            }
-        }
     }
 }
index 6e5637d..d506e7e 100644 (file)
@@ -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<object[]> SymbolicLink_LinkTarget_PathToTarget_Data
+        {
+            get
+            {
+                foreach (string path in PathToTargetData.Union(PathToTargetUncData))
+                {
+                    yield return new object[] { path };
+                }
+            }
+        }
+
+        public static IEnumerable<object[]> 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<object[]> Junction_LinkTarget_PathToTarget_Data
+        {
+            get
+            {
+                foreach (string path in PathToTargetData)
+                {
+                    yield return new object[] { path };
+                }
+            }
+        }
+
+        public static IEnumerable<object[]> 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<string> 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<string> PathToTargetUncData
+        {
+            get
+            {
+                if (OperatingSystem.IsWindows())
+                {
+                    // UNC/Remote Share
+                    yield return @"\\LOCALHOST\share\path";
+                }
+            }
+        }
     }
 }
index f4f2150..c280008 100644 (file)
@@ -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<DirectoryInfo>(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);
+        }
     }
 }
index 6cd99ca..1e16e6b 100644 (file)
   <data name="IO_BindHandleFailed" xml:space="preserve">
     <value>BindHandle for ThreadPool failed on this handle.</value>
   </data>
-  <data name="IO_InconsistentLinkType" xml:space="preserve">
-    <value>The link's file system entry type is inconsistent with that of its target: {0}</value>
-  </data>
   <data name="IO_FileExists_Name" xml:space="preserve">
     <value>The file '{0}' already exists.</value>
   </data>
index aab00ed..fc4b2fe 100644 (file)
@@ -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);
         }
 
index 223117e..a233347 100644 (file)
@@ -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<char> targetPath = MemoryMarshal.Cast<byte, char>(bufferSpan.Slice(printNameOffset, printNameLength));
-                    Debug.Assert((rbSymlink.Flags & Interop.Kernel32.SYMLINK_FLAG_RELATIVE) == 0 || !PathInternal.IsExtended(targetPath));
+                    Span<char> targetPath = MemoryMarshal.Cast<byte, char>(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<char> targetPath = MemoryMarshal.Cast<byte, char>(bufferSpan.Slice(printNameOffset, printNameLength));
+                    Span<char> targetPath = MemoryMarshal.Cast<byte, char>(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<byte>.Shared.Return(buffer);
             }
+
+            static string GetTargetPathWithoutNTPrefix(ReadOnlySpan<char> targetPath)
+            {
+                Debug.Assert(targetPath.StartsWith(PathInternal.NTPathPrefix.AsSpan()));
+                return targetPath.Slice(PathInternal.NTPathPrefix.Length).ToString();
+            }
         }
 
         private static unsafe string? GetFinalLinkTarget(string linkPath, bool isDirectory)
index b1767b3..48fb353 100644 (file)
@@ -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 = @"..\";