TarEntry: remove some unneeded checks when extracting symbolic and hard links. (#85378)

* TarEntry: remove some unneeded checks when extracting symbolic and hard links.

* Add test.

* Fix linkTargetPath for hard links.

The LinkName for hard links is a path relative to the archive root.

* Add the trailing separator early on.

* Sanitize the complete Name and LinkName.

* Preserve the drive colon when sanitizing paths on Windows.

* Add some comments.

* PR feedback.

Author: tmds

src/libraries/Common/src/System/IO/Archiving.Utils.Unix.cs

@@ -5,7 +5,9 @@ namespace System.IO
 {
     internal static partial class ArchivingUtils
     {
-        internal static string SanitizeEntryFilePath(string entryPath) => entryPath.Replace('\0', '_');
+#pragma warning disable IDE0060 // preserveDriveRoot is unused.
+        internal static string SanitizeEntryFilePath(string entryPath, bool preserveDriveRoot = false) => entryPath.Replace('\0', '_');
+#pragma warning restore IDE0060
 
         public static unsafe string EntryFromPath(ReadOnlySpan<char> path, bool appendPathSeparator = false)
         {

src/libraries/Common/src/System/IO/Archiving.Utils.Windows.cs

@@ -13,15 +13,23 @@ internal static partial class ArchivingUtils
             "\u0010\u0011\u0012\u0013\u0014\u0015\u0016\u0017\u0018\u0019\u001A\u001B\u001C\u001D\u001E\u001F" +
             "\"*:<>?|");
 
-        internal static string SanitizeEntryFilePath(string entryPath)
+        internal static string SanitizeEntryFilePath(string entryPath, bool preserveDriveRoot = false)
         {
+            // When preserveDriveRoot is set, preserve the colon in 'c:\'.
+            int offset = 0;
+            if (preserveDriveRoot && entryPath.Length >= 3 && entryPath[1] == ':' && Path.IsPathFullyQualified(entryPath))
+            {
+                offset = 3;
+            }
+
             // Find the first illegal character in the entry path.
-            int i = entryPath.AsSpan().IndexOfAny(s_illegalChars);
+            int i = entryPath.AsSpan(offset).IndexOfAny(s_illegalChars);
             if (i < 0)
             {
                 // There weren't any characters to sanitize.  Just return the original string.
                 return entryPath;
             }
+            i += offset;
 
             // We found at least one character that needs to be replaced.
             return string.Create(entryPath.Length, (i, entryPath), static (dest, state) =>

src/libraries/System.Formats.Tar/src/Resources/Strings.resx

@@ -160,12 +160,6 @@
   <data name="TarGnuFormatExpected" xml:space="preserve">
     <value>Entry '{0}' was expected to be in the GNU format, but did not have the expected version data.</value>
   </data>
-  <data name="TarHardLinkTargetNotExists" xml:space="preserve">
-    <value>Cannot create a hard link '{0}' because the specified target file '{1}' does not exist.</value>
-  </data>
-  <data name="TarHardLinkToDirectoryNotAllowed" xml:space="preserve">
-    <value>Cannot create the hard link '{0}' targeting the directory '{1}'.</value>
-  </data>
   <data name="TarInvalidFormat" xml:space="preserve">
     <value>The archive format is invalid: '{0}'</value>
   </data>
@@ -178,9 +172,6 @@
   <data name="TarSizeFieldTooLargeForEntryType" xml:space="preserve">
     <value>The value of the size field for the current entry of type '{0}' is greater than the expected length.</value>
   </data>
-  <data name="TarSymbolicLinkTargetNotExists" xml:space="preserve">
-    <value>Cannot create the symbolic link '{0}' because the specified target '{1}' does not exist.</value>
-  </data>
   <data name="TarUnexpectedMetadataEntry" xml:space="preserve">
     <value>A metadata entry of type '{0}' was unexpectedly found after a metadata entry of type '{1}'.</value>
   </data>

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs

@@ -330,59 +330,65 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b
             Debug.Assert(!string.IsNullOrEmpty(destinationDirectoryPath));
             Debug.Assert(Path.IsPathFullyQualified(destinationDirectoryPath));
 
-            destinationDirectoryPath = Path.TrimEndingDirectorySeparator(destinationDirectoryPath);
-
-            string? fileDestinationPath = GetSanitizedFullPath(destinationDirectoryPath, Name);
+            string name = ArchivingUtils.SanitizeEntryFilePath(Name, preserveDriveRoot: true);
+            string? fileDestinationPath = GetFullDestinationPath(
+                                                destinationDirectoryPath,
+                                                Path.IsPathFullyQualified(name) ? name : Path.Join(Path.GetDirectoryName(destinationDirectoryPath), name));
             if (fileDestinationPath == null)
             {
-                throw new IOException(SR.Format(SR.TarExtractingResultsFileOutside, Name, destinationDirectoryPath));
+                throw new IOException(SR.Format(SR.TarExtractingResultsFileOutside, name, destinationDirectoryPath));
             }
 
             string? linkTargetPath = null;
-            if (EntryType is TarEntryType.SymbolicLink or TarEntryType.HardLink)
-            {
-                if (string.IsNullOrEmpty(LinkName))
+            if (EntryType is TarEntryType.SymbolicLink)
+            {
+                // LinkName is an absolute path, or path relative to the fileDestinationPath directory.
+                // We don't check if the LinkName is empty. In that case, creation of the link will fail because link targets can't be empty.
+                string linkName = ArchivingUtils.SanitizeEntryFilePath(LinkName, preserveDriveRoot: true);
+                string? linkDestination = GetFullDestinationPath(
+                                            destinationDirectoryPath,
+                                            Path.IsPathFullyQualified(linkName) ? linkName : Path.Join(Path.GetDirectoryName(fileDestinationPath), linkName));
+                if (linkDestination is null)
                 {
-                    throw new InvalidDataException(SR.TarEntryHardLinkOrSymlinkLinkNameEmpty);
+                    throw new IOException(SR.Format(SR.TarExtractingResultsLinkOutside, linkName, destinationDirectoryPath));
                 }
-
-                linkTargetPath = GetSanitizedFullPath(destinationDirectoryPath,
-                    Path.IsPathFullyQualified(LinkName) ? LinkName : Path.Join(Path.GetDirectoryName(fileDestinationPath), LinkName));
-
-                if (linkTargetPath == null)
+                // Use the linkName for creating the symbolic link.
+                linkTargetPath = linkName;
+            }
+            else if (EntryType is TarEntryType.HardLink)
+            {
+                // LinkName is path relative to the destinationDirectoryPath.
+                // We don't check if the LinkName is empty. In that case, creation of the link will fail because a hard link can't target a directory.
+                string linkName = ArchivingUtils.SanitizeEntryFilePath(LinkName, preserveDriveRoot: false);
+                string? linkDestination = GetFullDestinationPath(
+                                            destinationDirectoryPath,
+                                            Path.Join(destinationDirectoryPath, linkName));
+                if (linkDestination is null)
                 {
-                    throw new IOException(SR.Format(SR.TarExtractingResultsLinkOutside, LinkName, destinationDirectoryPath));
+                    throw new IOException(SR.Format(SR.TarExtractingResultsLinkOutside, linkName, destinationDirectoryPath));
                 }
-
-                // after TarExtractingResultsLinkOutside validation, preserve the original
-                // symlink target path (to match behavior of other utilities).
-                linkTargetPath = LinkName;
+                // Use the target path for creating the hard link.
+                linkTargetPath = linkDestination;
             }
 
             return (fileDestinationPath, linkTargetPath);
         }
 
-        // If the path can be extracted in the specified destination directory, returns the full path with sanitized file name. Otherwise, returns null.
-        private static string? GetSanitizedFullPath(string destinationDirectoryFullPath, string path)
+        // Returns the full destination path if the path is the destinationDirectory or a subpath. Otherwise, returns null.
+        private static string? GetFullDestinationPath(string destinationDirectoryFullPath, string qualifiedPath)
         {
-            destinationDirectoryFullPath = PathInternal.EnsureTrailingSeparator(destinationDirectoryFullPath);
+            Debug.Assert(Path.IsPathFullyQualified(qualifiedPath), $"{qualifiedPath} is not qualified");
+            Debug.Assert(PathInternal.EndsInDirectorySeparator(destinationDirectoryFullPath), "caller must ensure the path ends with a separator.");
 
-            string fullyQualifiedPath = Path.IsPathFullyQualified(path) ? path : Path.Combine(destinationDirectoryFullPath, path);
-            string normalizedPath = Path.GetFullPath(fullyQualifiedPath); // Removes relative segments
-            string? fileName = Path.GetFileName(normalizedPath);
-            if (string.IsNullOrEmpty(fileName)) // It's a directory
-            {
-                fileName = PathInternal.DirectorySeparatorCharAsString;
-            }
+            string fullPath = Path.GetFullPath(qualifiedPath); // Removes relative segments
 
-            string sanitizedPath = Path.Join(Path.GetDirectoryName(normalizedPath), ArchivingUtils.SanitizeEntryFilePath(fileName));
-            return sanitizedPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison) ? sanitizedPath : null;
+            return fullPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison) ? fullPath : null;
         }
 
         // Extracts the current entry into the filesystem, regardless of the entry type.
         private void ExtractToFileInternal(string filePath, string? linkTargetPath, bool overwrite)
         {
-            VerifyPathsForEntryType(filePath, linkTargetPath, overwrite);
+            VerifyDestinationPath(filePath, overwrite);
 
             if (EntryType is TarEntryType.RegularFile or TarEntryType.V7RegularFile or TarEntryType.ContiguousFile)
             {
@@ -401,7 +407,7 @@ private Task ExtractToFileInternalAsync(string filePath, string? linkTargetPath,
             {
                 return Task.FromCanceled(cancellationToken);
             }
-            VerifyPathsForEntryType(filePath, linkTargetPath, overwrite);
+            VerifyDestinationPath(filePath, overwrite);
 
             if (EntryType is TarEntryType.RegularFile or TarEntryType.V7RegularFile or TarEntryType.ContiguousFile)
             {
@@ -423,7 +429,7 @@ private void CreateNonRegularFile(string filePath, string? linkTargetPath)
                 case TarEntryType.Directory:
                 case TarEntryType.DirectoryList:
                     // Mode must only be used for the leaf directory.
-                    // VerifyPathsForEntryType ensures we're only creating a leaf.
+                    // VerifyDestinationPath ensures we're only creating a leaf.
                     Debug.Assert(Directory.Exists(Path.GetDirectoryName(filePath)));
                     Debug.Assert(!Directory.Exists(filePath));
 
@@ -476,8 +482,8 @@ private void CreateNonRegularFile(string filePath, string? linkTargetPath)
             }
         }
 
-        // Verifies if the specified paths make sense for the current type of entry.
-        private void VerifyPathsForEntryType(string filePath, string? linkTargetPath, bool overwrite)
+        // Verifies there's a writable destination.
+        private static void VerifyDestinationPath(string filePath, bool overwrite)
         {
             string? directoryPath = Path.GetDirectoryName(filePath);
             // If the destination contains a directory segment, need to check that it exists
@@ -503,35 +509,6 @@ private void VerifyPathsForEntryType(string filePath, string? linkTargetPath, bo
                 throw new IOException(SR.Format(SR.IO_AlreadyExists_Name, filePath));
             }
             File.Delete(filePath);
-
-            if (EntryType is TarEntryType.SymbolicLink or TarEntryType.HardLink)
-            {
-                if (!string.IsNullOrEmpty(linkTargetPath))
-                {
-                    string? targetDirectoryPath = Path.GetDirectoryName(linkTargetPath);
-                    // If the destination target contains a directory segment, need to check that it exists
-                    if (!string.IsNullOrEmpty(targetDirectoryPath) && !Path.Exists(targetDirectoryPath))
-                    {
-                        throw new IOException(SR.Format(SR.TarSymbolicLinkTargetNotExists, filePath, linkTargetPath));
-                    }
-
-                    if (EntryType is TarEntryType.HardLink)
-                    {
-                        if (!Path.Exists(linkTargetPath))
-                        {
-                            throw new IOException(SR.Format(SR.TarHardLinkTargetNotExists, filePath, linkTargetPath));
-                        }
-                        else if (Directory.Exists(linkTargetPath))
-                        {
-                            throw new IOException(SR.Format(SR.TarHardLinkToDirectoryNotAllowed, filePath, linkTargetPath));
-                        }
-                    }
-                }
-                else
-                {
-                    throw new InvalidDataException(SR.TarEntryHardLinkOrSymlinkLinkNameEmpty);
-                }
-            }
         }
 
         // Extracts the current entry as a regular file into the specified destination.

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarFile.cs

@@ -185,6 +185,7 @@ public static void ExtractToDirectory(Stream source, string destinationDirectory
 
             // Rely on Path.GetFullPath for validation of paths
             destinationDirectoryName = Path.GetFullPath(destinationDirectoryName);
+            destinationDirectoryName = PathInternal.EnsureTrailingSeparator(destinationDirectoryName);
 
             ExtractToDirectoryInternal(source, destinationDirectoryName, overwriteFiles, leaveOpen: true);
         }
@@ -229,6 +230,7 @@ public static Task ExtractToDirectoryAsync(Stream source, string destinationDire
 
             // Rely on Path.GetFullPath for validation of paths
             destinationDirectoryName = Path.GetFullPath(destinationDirectoryName);
+            destinationDirectoryName = PathInternal.EnsureTrailingSeparator(destinationDirectoryName);
 
             return ExtractToDirectoryInternalAsync(source, destinationDirectoryName, overwriteFiles, leaveOpen: true, cancellationToken);
         }
@@ -257,6 +259,7 @@ public static void ExtractToDirectory(string sourceFileName, string destinationD
             // Rely on Path.GetFullPath for validation of paths
             sourceFileName = Path.GetFullPath(sourceFileName);
             destinationDirectoryName = Path.GetFullPath(destinationDirectoryName);
+            destinationDirectoryName = PathInternal.EnsureTrailingSeparator(destinationDirectoryName);
 
             if (!File.Exists(sourceFileName))
             {
@@ -303,6 +306,7 @@ public static Task ExtractToDirectoryAsync(string sourceFileName, string destina
             // Rely on Path.GetFullPath for validation of paths
             sourceFileName = Path.GetFullPath(sourceFileName);
             destinationDirectoryName = Path.GetFullPath(destinationDirectoryName);
+            destinationDirectoryName = PathInternal.EnsureTrailingSeparator(destinationDirectoryName);
 
             if (!File.Exists(sourceFileName))
             {

src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Roundtrip.cs

@@ -73,7 +73,7 @@ public void SymlinkRelativeTargets_OutsideTheArchive_Fails(string symlinkTargetP
             using FileStream archiveStream = File.OpenRead(destinationArchive);
             Exception exception = Assert.Throws<IOException>(() => TarFile.ExtractToDirectory(archiveStream, destinationDirectoryName, overwriteFiles: true));
 
-            Assert.Equal(SR.Format(SR.TarExtractingResultsLinkOutside, symlinkTargetPath, destinationDirectoryName), exception.Message);
+            Assert.Equal(SR.Format(SR.TarExtractingResultsLinkOutside, symlinkTargetPath, $"{destinationDirectoryName}{Path.DirectorySeparatorChar}"), exception.Message);
         }
     }
 }

src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectoryAsync.File.Roundtrip.cs

@@ -74,7 +74,7 @@ public async Task SymlinkRelativeTargets_OutsideTheArchive_Fails_Async(string sy
             using FileStream archiveStream = File.OpenRead(destinationArchive);
             Exception exception = await Assert.ThrowsAsync<IOException>(() => TarFile.ExtractToDirectoryAsync(archiveStream, destinationDirectoryName, overwriteFiles: true));
 
-            Assert.Equal(SR.Format(SR.TarExtractingResultsLinkOutside, symlinkTargetPath, destinationDirectoryName), exception.Message);
+            Assert.Equal(SR.Format(SR.TarExtractingResultsLinkOutside, symlinkTargetPath, $"{destinationDirectoryName}{Path.DirectorySeparatorChar}"), exception.Message);
         }
     }
 }

src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs

@@ -296,5 +296,34 @@ public void UnixFileModes_RestrictiveParentDir(bool overwrite)
             Assert.True(File.Exists(filePath), $"{filePath}' does not exist.");
             AssertFileModeEquals(filePath, TestPermission1);
         }
+
+        [Fact]
+        public void LinkBeforeTarget()
+        {
+            using TempDirectory source = new TempDirectory();
+            using TempDirectory destination = new TempDirectory();
+
+            string archivePath = Path.Join(source.Path, "archive.tar");
+            using FileStream archiveStream = File.Create(archivePath);
+            using (TarWriter writer = new TarWriter(archiveStream))
+            {
+                PaxTarEntry link = new PaxTarEntry(TarEntryType.SymbolicLink, "link");
+                link.LinkName = "dir/file";
+                writer.WriteEntry(link);
+
+                PaxTarEntry file = new PaxTarEntry(TarEntryType.RegularFile, "dir/file");
+                writer.WriteEntry(file);
+            }
+
+            string filePath = Path.Join(destination.Path, "dir", "file");
+            string linkPath = Path.Join(destination.Path, "link");
+
+            File.WriteAllText(linkPath, "");
+
+            TarFile.ExtractToDirectory(archivePath, destination.Path, overwriteFiles: true);
+
+            Assert.True(File.Exists(filePath), $"{filePath}' does not exist.");
+            Assert.True(File.Exists(linkPath), $"{linkPath}' does not exist.");
+        }
     }
 }

src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.Stream.Tests.cs

@@ -90,7 +90,7 @@ public void Extract_LinkEntry_TargetOutsideDirectory(TarEntryType entryType)
 
             using TempDirectory root = new TempDirectory();
 
-            Assert.Throws<IOException>(() => TarFile.ExtractToDirectory(archive, root.Path, overwriteFiles: false));
+            Assert.ThrowsAny<IOException>(() => TarFile.ExtractToDirectory(archive, root.Path, overwriteFiles: false));
 
             Assert.Equal(0, Directory.GetFileSystemEntries(root.Path).Count());
         }
@@ -123,19 +123,20 @@ private void Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType entry
         {
             using TempDirectory root = new TempDirectory();
 
-            string baseDir = string.IsNullOrEmpty(subfolder) ? root.Path : Path.Join(root.Path, subfolder);
+            string baseDir = root.Path;
             Directory.CreateDirectory(baseDir);
 
             string linkName = "link";
             string targetName = "target";
-            string targetPath = Path.Join(baseDir, targetName);
-
-            File.Create(targetPath).Dispose();
+            string targetPath = string.IsNullOrEmpty(subfolder) ? targetName : Path.Join(subfolder, targetName);
 
             using MemoryStream archive = new MemoryStream();
             using (TarWriter writer = new TarWriter(archive, format, leaveOpen: true))
             {
-                TarEntry entry= InvokeTarEntryCreationConstructor(format, entryType, linkName);
+                TarEntry fileEntry = InvokeTarEntryCreationConstructor(format, TarEntryType.RegularFile, targetPath);
+                writer.WriteEntry(fileEntry);
+
+                TarEntry entry = InvokeTarEntryCreationConstructor(format, entryType, linkName);
                 entry.LinkName = targetPath;
                 writer.WriteEntry(entry);
             }