Directory.CreateDirectory: create missing parents using default UnixFileMode. (#74002)

* Directory.CreateDirectory: create missing parents using default UnixFileMode.

* Simplify TarHelpers.CreateDirectory.

* PR feedback.

* AssertFileModeEquals: don't skip on Android.

* Always overwite directory metadata.

* TarHelpers.Windows: remove CreateDirectory overwriteMetadata.

* Fix Assert indentation.

Author: tmds

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

@@ -287,12 +287,12 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o
 
             if (EntryType == TarEntryType.Directory)
             {
-                TarHelpers.CreateDirectory(fileDestinationPath, Mode, overwrite, pendingModes);
+                TarHelpers.CreateDirectory(fileDestinationPath, Mode, pendingModes);
             }
             else
             {
                 // If it is a file, create containing directory.
-                TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, overwrite, pendingModes);
+                TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, pendingModes);
                 ExtractToFileInternal(fileDestinationPath, linkTargetPath, overwrite);
             }
         }
@@ -309,13 +309,13 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b
 
             if (EntryType == TarEntryType.Directory)
             {
-                TarHelpers.CreateDirectory(fileDestinationPath, Mode, overwrite, pendingModes);
+                TarHelpers.CreateDirectory(fileDestinationPath, Mode, pendingModes);
                 return Task.CompletedTask;
             }
             else
             {
                 // If it is a file, create containing directory.
-                TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, overwrite, pendingModes);
+                TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, pendingModes);
                 return ExtractToFileInternalAsync(fileDestinationPath, linkTargetPath, overwrite, cancellationToken);
             }
         }

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Unix.cs

@@ -47,57 +47,41 @@ public int Compare (string? x, string? y)
 
         private static UnixFileMode UMask => s_umask.Value;
 
-        /*
-            Tar files are usually ordered: parent directories come before their child entries.
-
-            They may be unordered. In that case we need to create parent directories before
-            we know the proper permissions for these directories.
-
-            We create these directories with restrictive permissions. If we encounter an entry for
-            the directory later, we store the mode to apply it later.
-
-            If the archive doesn't have an entry for the parent directory, we use the default mask.
-
-            The pending modes to be applied are tracked through a reverse-sorted dictionary.
-            The reverse order is needed to apply permissions to children before their parent.
-            Otherwise we may apply a restrictive mask to the parent, that prevents us from
-            changing a child.
-        */
-
+        // Use a reverse-sorted dictionary to apply permission to children before their parents.
+        // Otherwise we may apply a restrictive mask to the parent, that prevents us from changing a child.
         internal static SortedDictionary<string, UnixFileMode>? CreatePendingModesDictionary()
             => new SortedDictionary<string, UnixFileMode>(s_reverseStringComparer);
 
-        internal static void CreateDirectory(string fullPath, UnixFileMode? mode, bool overwriteMetadata, SortedDictionary<string, UnixFileMode>? pendingModes)
+        internal static void CreateDirectory(string fullPath, UnixFileMode? mode, SortedDictionary<string, UnixFileMode>? pendingModes)
         {
-            // Restrictive mask for creating the missing parent directories while extracting.
+            // Minimal permissions required for extracting.
             const UnixFileMode ExtractPermissions = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute;
 
             Debug.Assert(pendingModes is not null);
 
             if (Directory.Exists(fullPath))
             {
-                // Apply permissions to an existing directory when we're overwriting metadata
-                // or the directory was created as a missing parent (stored in pendingModes).
+                // Apply permissions to an existing directory.
                 if (mode.HasValue)
                 {
+                    // Ensure we have sufficient permissions to extract in the directory.
                     bool hasExtractPermissions = (mode.Value & ExtractPermissions) == ExtractPermissions;
                     if (hasExtractPermissions)
                     {
-                        bool removed = pendingModes.Remove(fullPath);
-                        if (overwriteMetadata || removed)
-                        {
-                            UnixFileMode umask = UMask;
-                            File.SetUnixFileMode(fullPath, mode.Value & ~umask);
-                        }
+                        pendingModes.Remove(fullPath);
+
+                        UnixFileMode umask = UMask;
+                        File.SetUnixFileMode(fullPath, mode.Value & ~umask);
                     }
-                    else if (overwriteMetadata || pendingModes.ContainsKey(fullPath))
+                    else
                     {
-                            pendingModes[fullPath] = mode.Value;
+                        pendingModes[fullPath] = mode.Value;
                     }
                 }
                 return;
             }
 
+            // If there are missing parents, Directory.CreateDirectory will create them using default permissions.
             if (mode.HasValue)
             {
                 // Ensure we have sufficient permissions to extract in the directory.
@@ -106,28 +90,13 @@ internal static void CreateDirectory(string fullPath, UnixFileMode? mode, bool o
                     pendingModes[fullPath] = mode.Value;
                     mode = ExtractPermissions;
                 }
-            }
-            else
-            {
-                pendingModes.Add(fullPath, DefaultDirectoryMode);
-                mode = ExtractPermissions;
-            }
 
-            string parentDir = Path.GetDirectoryName(fullPath)!;
-            string rootDir = Path.GetPathRoot(parentDir)!;
-            bool hasMissingParents = false;
-            for (string dir = parentDir; dir != rootDir && !Directory.Exists(dir); dir = Path.GetDirectoryName(dir)!)
-            {
-                pendingModes.Add(dir, DefaultDirectoryMode);
-                hasMissingParents = true;
+                Directory.CreateDirectory(fullPath, mode.Value);
             }
-
-            if (hasMissingParents)
+            else
             {
-                Directory.CreateDirectory(parentDir, ExtractPermissions);
+                Directory.CreateDirectory(fullPath);
             }
-
-            Directory.CreateDirectory(fullPath, mode.Value);
         }
 
         internal static void SetPendingModes(SortedDictionary<string, UnixFileMode>? pendingModes)

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Windows.cs

@@ -13,7 +13,7 @@ internal static partial class TarHelpers
         internal static SortedDictionary<string, UnixFileMode>? CreatePendingModesDictionary()
             => null;
 
-        internal static void CreateDirectory(string fullPath, UnixFileMode? mode, bool overwriteMetadata, SortedDictionary<string, UnixFileMode>? pendingModes)
+        internal static void CreateDirectory(string fullPath, UnixFileMode? mode, SortedDictionary<string, UnixFileMode>? pendingModes)
             => Directory.CreateDirectory(fullPath);
 
         internal static void SetPendingModes(SortedDictionary<string, UnixFileMode>? pendingModes)

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

@@ -216,13 +216,9 @@ public void UnixFileModes(bool overwrite)
             Assert.True(File.Exists(filePath), $"{filePath}' does not exist.");
             AssertFileModeEquals(filePath, TestPermission2);
 
-            // Missing parents are created with DefaultDirectoryMode.
-            // The mode is not set when overwrite == true if there is no entry and the directory exists before extracting.
+            // Missing parents are created with CreateDirectoryDefaultMode.
             Assert.True(Directory.Exists(missingParentPath), $"{missingParentPath}' does not exist.");
-            if (!overwrite)
-            {
-                AssertFileModeEquals(missingParentPath, DefaultDirectoryMode);
-            }
+            AssertFileModeEquals(missingParentPath, CreateDirectoryDefaultMode);
 
             Assert.True(Directory.Exists(missingParentDirPath), $"{missingParentDirPath}' does not exist.");
             AssertFileModeEquals(missingParentDirPath, TestPermission3);

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

@@ -191,8 +191,10 @@ public async Task ExtractArchiveWithEntriesThatStartWithSlashDotPrefix_Async()
             }
         }
 
-        [Fact]
-        public async Task UnixFileModes_Async()
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task UnixFileModes_Async(bool overwrite)
         {
             using TempDirectory source = new TempDirectory();
             using TempDirectory destination = new TempDirectory();
@@ -223,27 +225,36 @@ public async Task UnixFileModes_Async()
                 writer.WriteEntry(outOfOrderDir);
             }
 
-            await TarFile.ExtractToDirectoryAsync(archivePath, destination.Path, overwriteFiles: false);
-
             string dirPath = Path.Join(destination.Path, "dir");
+            string filePath = Path.Join(destination.Path, "file");
+            string missingParentPath = Path.Join(destination.Path, "missing_parent");
+            string missingParentDirPath = Path.Join(missingParentPath, "dir");
+            string outOfOrderDirPath = Path.Join(destination.Path, "out_of_order_parent");
+
+            if (overwrite)
+            {
+                File.OpenWrite(filePath).Dispose();
+                Directory.CreateDirectory(dirPath);
+                Directory.CreateDirectory(missingParentDirPath);
+                Directory.CreateDirectory(outOfOrderDirPath);
+            }
+
+            await TarFile.ExtractToDirectoryAsync(archivePath, destination.Path, overwriteFiles: overwrite);
+
             Assert.True(Directory.Exists(dirPath), $"{dirPath}' does not exist.");
             AssertFileModeEquals(dirPath, TestPermission1);
 
-            string filePath = Path.Join(destination.Path, "file");
             Assert.True(File.Exists(filePath), $"{filePath}' does not exist.");
             AssertFileModeEquals(filePath, TestPermission2);
 
-            // Missing parents are created with DefaultDirectoryMode.
-            string missingParentPath = Path.Join(destination.Path, "missing_parent");
+            // Missing parents are created with CreateDirectoryDefaultMode.
             Assert.True(Directory.Exists(missingParentPath), $"{missingParentPath}' does not exist.");
-            AssertFileModeEquals(missingParentPath, DefaultDirectoryMode);
+            AssertFileModeEquals(missingParentPath, CreateDirectoryDefaultMode);
 
-            string missingParentDirPath = Path.Join(missingParentPath, "dir");
             Assert.True(Directory.Exists(missingParentDirPath), $"{missingParentDirPath}' does not exist.");
             AssertFileModeEquals(missingParentDirPath, TestPermission3);
 
             // Directory modes that are out-of-order are still applied.
-            string outOfOrderDirPath = Path.Join(destination.Path, "out_of_order_parent");
             Assert.True(Directory.Exists(outOfOrderDirPath), $"{outOfOrderDirPath}' does not exist.");
             AssertFileModeEquals(outOfOrderDirPath, TestPermission4);
         }

src/libraries/System.Formats.Tar/tests/TarTestsBase.cs

@@ -17,6 +17,9 @@ public abstract partial class TarTestsBase : FileCleanupTestBase
         protected const UnixFileMode DefaultFileMode = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.GroupRead | UnixFileMode.OtherRead; // 644 in octal, internally used as default
         protected const UnixFileMode DefaultDirectoryMode = DefaultFileMode | UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute; // 755 in octal, internally used as default
 
+        protected readonly UnixFileMode CreateDirectoryDefaultMode; // Mode of directories created using Directory.CreateDirectory(string).
+        protected readonly UnixFileMode UMask;
+
         // Mode assumed for files and directories on Windows.
         protected const UnixFileMode DefaultWindowsMode = DefaultFileMode | UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute; // 755 in octal, internally used as default
 
@@ -115,6 +118,12 @@ public enum TestTarFormat
 
         protected static bool IsNotLinuxBionic => !PlatformDetection.IsLinuxBionic;
 
+        protected TarTestsBase()
+        {
+            CreateDirectoryDefaultMode = Directory.CreateDirectory(GetRandomDirPath()).UnixFileMode; // '0777 & ~umask'
+            UMask = ~CreateDirectoryDefaultMode & (UnixFileMode)Convert.ToInt32("777", 8);
+        }
+
         protected static string GetTestCaseUnarchivedFolderPath(string testCaseName) =>
             Path.Join(Directory.GetCurrentDirectory(), "unarchived", testCaseName);
 
@@ -407,11 +416,20 @@ protected static void AssertEntryModeFromFileSystemEquals(TarEntry entry, UnixFi
             Assert.Equal(fileMode, entry.Mode);
         }
 
-        protected static void AssertFileModeEquals(string path, UnixFileMode mode)
+        protected void AssertFileModeEquals(string path, UnixFileMode archiveMode)
         {
             if (!PlatformDetection.IsWindows)
             {
-                Assert.Equal(mode, File.GetUnixFileMode(path));
+                UnixFileMode expectedMode = archiveMode & ~UMask;
+
+                UnixFileMode actualMode = File.GetUnixFileMode(path);
+                // Ignore SetGroup: it may have been added to preserve group ownership.
+                if ((expectedMode & UnixFileMode.SetGroup) == 0)
+                {
+                    actualMode &= ~UnixFileMode.SetGroup;
+                }
+
+                Assert.Equal(expectedMode, actualMode);
             }
         }
 

src/libraries/System.IO.FileSystem/tests/Directory/CreateDirectory_UnixFileMode.Unix.cs

@@ -39,6 +39,21 @@ public void CreateDoesntChangeExistingMode()
             Assert.Equal(initialMode, sameDir.UnixFileMode);
         }
 
+        [Fact]
+        public void MissingParentsHaveDefaultPermissions()
+        {
+            string parent = GetRandomDirPath();
+            string child = Path.Combine(parent, "child");
+
+            const UnixFileMode childMode = UnixFileMode.UserRead | UnixFileMode.UserExecute;
+            DirectoryInfo childDir = Directory.CreateDirectory(child, childMode);
+
+            Assert.Equal(childMode, childDir.UnixFileMode);
+
+            UnixFileMode defaultPermissions = Directory.CreateDirectory(GetRandomDirPath()).UnixFileMode;
+            Assert.Equal(defaultPermissions, File.GetUnixFileMode(parent));
+        }
+
         [Theory]
         [InlineData((UnixFileMode)(1 << 12), false)]
         [InlineData((UnixFileMode)(1 << 12), true)]

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs

@@ -328,7 +328,7 @@ private static void CreateParentsAndDirectory(string fullPath, UnixFileMode unix
                 }
 
                 ReadOnlySpan<char> mkdirPath = fullPath.AsSpan(0, i);
-                int result = Interop.Sys.MkDir(mkdirPath, (int)unixCreateMode);
+                int result = Interop.Sys.MkDir(mkdirPath, (int)DefaultUnixCreateDirectoryMode);
                 if (result == 0)
                 {
                     break; // Created parent.
@@ -360,7 +360,8 @@ private static void CreateParentsAndDirectory(string fullPath, UnixFileMode unix
             for (i = stackDir.Length - 1; i >= 0; i--)
             {
                 ReadOnlySpan<char> mkdirPath = fullPath.AsSpan(0, stackDir[i]);
-                int result = Interop.Sys.MkDir(mkdirPath, (int)unixCreateMode);
+                UnixFileMode mode = i == 0 ? unixCreateMode : DefaultUnixCreateDirectoryMode;
+                int result = Interop.Sys.MkDir(mkdirPath, (int)mode);
                 if (result < 0)
                 {
                     Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo();