From 8196f9ab79a8d461c3b546bac59e3b21d13d3867 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 20 Aug 2022 10:57:18 -0700 Subject: [PATCH] [release/7.0-rc1] Directory.CreateDirectory: create missing parents using default UnixFileMode. (#74277) * 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. Co-authored-by: Tom Deseyn --- .../src/System/Formats/Tar/TarEntry.cs | 8 +-- .../src/System/Formats/Tar/TarHelpers.Unix.cs | 63 +++++-------------- .../System/Formats/Tar/TarHelpers.Windows.cs | 2 +- .../TarFile.ExtractToDirectory.File.Tests.cs | 8 +-- ...File.ExtractToDirectoryAsync.File.Tests.cs | 31 ++++++--- .../System.Formats.Tar/tests/TarTestsBase.cs | 22 ++++++- .../CreateDirectory_UnixFileMode.Unix.cs | 15 +++++ .../src/System/IO/FileSystem.Unix.cs | 5 +- 8 files changed, 82 insertions(+), 72 deletions(-) diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs index 00c790762e4b65..27fe4af8701a92 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs +++ b/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); } } diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Unix.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Unix.cs index f684dd78fde1c6..3e0a0b814484d4 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Unix.cs +++ b/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? CreatePendingModesDictionary() => new SortedDictionary(s_reverseStringComparer); - internal static void CreateDirectory(string fullPath, UnixFileMode? mode, bool overwriteMetadata, SortedDictionary? pendingModes) + internal static void CreateDirectory(string fullPath, UnixFileMode? mode, SortedDictionary? 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? pendingModes) diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Windows.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Windows.cs index e00f6476764aba..6569ff237dbf97 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Windows.cs +++ b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Windows.cs @@ -13,7 +13,7 @@ internal static partial class TarHelpers internal static SortedDictionary? CreatePendingModesDictionary() => null; - internal static void CreateDirectory(string fullPath, UnixFileMode? mode, bool overwriteMetadata, SortedDictionary? pendingModes) + internal static void CreateDirectory(string fullPath, UnixFileMode? mode, SortedDictionary? pendingModes) => Directory.CreateDirectory(fullPath); internal static void SetPendingModes(SortedDictionary? pendingModes) diff --git a/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs index f741926cdd8ef1..ee00ccfaf6ccb8 100644 --- a/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs +++ b/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); diff --git a/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectoryAsync.File.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectoryAsync.File.Tests.cs index 01d2457018ce24..905e9b61bfb8e2 100644 --- a/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectoryAsync.File.Tests.cs +++ b/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); } diff --git a/src/libraries/System.Formats.Tar/tests/TarTestsBase.cs b/src/libraries/System.Formats.Tar/tests/TarTestsBase.cs index b167a980acd2b0..04d034ce8764f8 100644 --- a/src/libraries/System.Formats.Tar/tests/TarTestsBase.cs +++ b/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); } } diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/CreateDirectory_UnixFileMode.Unix.cs b/src/libraries/System.IO.FileSystem/tests/Directory/CreateDirectory_UnixFileMode.Unix.cs index 73c8f587bccede..5627974f47420a 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/CreateDirectory_UnixFileMode.Unix.cs +++ b/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)] 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 67237805c3a744..f538524471a918 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 @@ -328,7 +328,7 @@ private static void CreateParentsAndDirectory(string fullPath, UnixFileMode unix } ReadOnlySpan 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 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();