From d8451f1d298416690b74d8b271934eb1642ea5d5 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 23 Sep 2021 09:51:06 +0200 Subject: [PATCH 1/6] FileSystem.Unix: Directory.Delete: remove per item syscall. By recursing using FileSystemEnumerable we know the file type and can omit the stat calls made by DirectoryInfo.Exists. For the top level path, we can omit the call also and handle non-directories when rmdir errno is ENOTDIR. For the recursive case we can avoid recursion when the top level path rmdir succeeds immediately. FileSystemEntry is updated so IsSymbolicLink remembers the file is symbolic link and does not make a syscall for it. --- .../tests/Directory/Delete.cs | 29 +++++ .../src/System/IO/FileSystem.Unix.cs | 111 ++++++++++-------- 2 files changed, 93 insertions(+), 47 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index b264ec4c3b60e2..f6f71a780cf3ec 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -119,6 +119,35 @@ public void DeletingSymLinkDoesntDeleteTarget() Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); } + [ConditionalFact(nameof(CanCreateSymbolicLinks))] + public void RecursiveDeletingDoesntFollowLinks() + { + var target = GetTestFilePath(); + Directory.CreateDirectory(target); + + var fileInTarget = Path.Combine(target, GetTestFileName()); + File.WriteAllText(fileInTarget, ""); + + var linkParent = GetTestFilePath(); + Directory.CreateDirectory(linkParent); + + var linkPath = Path.Combine(linkParent, GetTestFileName()); + Assert.True(MountHelper.CreateSymbolicLink(linkPath, target, isDirectory: true)); + + // Both the symlink and the target exist + Assert.True(Directory.Exists(target), "target should exist"); + Assert.True(Directory.Exists(linkPath), "linkPath should exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + + // Delete the parent folder of the symlink. + Directory.Delete(linkParent, true); + + // Target should still exist + Assert.True(Directory.Exists(target), "target should still exist"); + Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + } + [ConditionalFact(nameof(UsingNewNormalization))] public void ExtendedDirectoryWithSubdirectories() { 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 e23ae9e9e28a8c..4d29f3bd63f90a 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 @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.IO.Enumeration; using System.Text; using Microsoft.Win32.SafeHandles; @@ -433,69 +434,64 @@ public static void MoveDirectory(string sourceFullPath, string destFullPath) public static void RemoveDirectory(string fullPath, bool recursive) { - var di = new DirectoryInfo(fullPath); - if (!di.Exists) + if (!TryRemoveDirectory(fullPath, topLevel: true, throwWhenNotEmpty: !recursive)) { - throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); + RemoveDirectoryRecursive(fullPath); } - RemoveDirectoryInternal(di, recursive, throwOnTopLevelDirectoryNotFound: true); } - private static void RemoveDirectoryInternal(DirectoryInfo directory, bool recursive, bool throwOnTopLevelDirectoryNotFound) + private static void RemoveDirectoryRecursive(string fullPath) { Exception? firstException = null; - if ((directory.Attributes & FileAttributes.ReparsePoint) != 0) + try { - DeleteFile(directory.FullName); - return; - } + foreach ((string childPath, bool isDirectory) in + new FileSystemEnumerable<(string, bool)>( + fullPath, + static (ref FileSystemEntry entry) => + { + // Don't report symlinks to directories as directories. + bool isRealDirectory = !entry.IsSymbolicLink && entry.IsDirectory; - if (recursive) - { - try + return (entry.ToFullPath(), isRealDirectory); + }, + EnumerationOptions.Compatible)) { - foreach (string item in Directory.EnumerateFileSystemEntries(directory.FullName)) + try { - if (!ShouldIgnoreDirectory(Path.GetFileName(item))) + if (isDirectory) { - try - { - var childDirectory = new DirectoryInfo(item); - if (childDirectory.Exists) - { - RemoveDirectoryInternal(childDirectory, recursive, throwOnTopLevelDirectoryNotFound: false); - } - else - { - DeleteFile(item); - } - } - catch (Exception exc) - { - if (firstException != null) - { - firstException = exc; - } - } + RemoveDirectoryRecursive(childPath); + } + else + { + DeleteFile(childPath); } } - } - catch (Exception exc) - { - if (firstException != null) + catch (Exception ex) { - firstException = exc; + firstException ??= ex; } } + } + catch (Exception exc) + { + firstException ??= exc; + } - if (firstException != null) - { - throw firstException; - } + if (firstException != null) + { + throw firstException; } - if (Interop.Sys.RmDir(directory.FullName) < 0) + bool removed = TryRemoveDirectory(fullPath); + Debug.Assert(removed); + } + + private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, bool throwWhenNotEmpty = true) + { + if (Interop.Sys.RmDir(fullPath) < 0) { Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo(); switch (errorInfo.Error) @@ -504,17 +500,38 @@ private static void RemoveDirectoryInternal(DirectoryInfo directory, bool recurs case Interop.Error.EPERM: case Interop.Error.EROFS: case Interop.Error.EISDIR: - throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, directory.FullName)); // match Win32 exception + throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, fullPath)); // match Win32 exception case Interop.Error.ENOENT: - if (!throwOnTopLevelDirectoryNotFound) + if (!topLevel) { - return; + return true; + } + goto default; + case Interop.Error.ENOTDIR: + if (topLevel) + { + if (!DirectoryExists(fullPath)) + { + throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); + } + + // Path is symbolic link to a directory. + DeleteFile(fullPath); + return true; + } + goto default; + case Interop.Error.ENOTEMPTY: + if (!throwWhenNotEmpty) + { + return false; } goto default; default: - throw Interop.GetExceptionForIoErrno(errorInfo, directory.FullName, isDirectory: true); + throw Interop.GetExceptionForIoErrno(errorInfo, fullPath, isDirectory: true); } } + + return true; } /// Determines whether the specified directory name should be ignored. From ead0f4772e1a917ad853920239642be784350892 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 18 Nov 2021 19:35:24 +0100 Subject: [PATCH 2/6] PR feedback --- .../src/System/IO/FileSystem.Unix.cs | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) 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 4d29f3bd63f90a..05b782147a8e3a 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 @@ -434,8 +434,12 @@ public static void MoveDirectory(string sourceFullPath, string destFullPath) public static void RemoveDirectory(string fullPath, bool recursive) { - if (!TryRemoveDirectory(fullPath, topLevel: true, throwWhenNotEmpty: !recursive)) + // Delete the directory. + // If we're recursing, don't throw when it is not empty, and perform a recursive remove. + if (!RemoveEmptyDirectory(fullPath, topLevel: true, throwWhenNotEmpty: !recursive)) { + Debug.Assert(recursive); + RemoveDirectoryRecursive(fullPath); } } @@ -446,17 +450,16 @@ private static void RemoveDirectoryRecursive(string fullPath) try { - foreach ((string childPath, bool isDirectory) in - new FileSystemEnumerable<(string, bool)>( - fullPath, - static (ref FileSystemEntry entry) => - { - // Don't report symlinks to directories as directories. - bool isRealDirectory = !entry.IsSymbolicLink && entry.IsDirectory; - - return (entry.ToFullPath(), isRealDirectory); - }, - EnumerationOptions.Compatible)) + var fse = new FileSystemEnumerable<(string, bool)>(fullPath, + static (ref FileSystemEntry entry) => + { + // Don't report symlinks to directories as directories. + bool isRealDirectory = !entry.IsSymbolicLink && entry.IsDirectory; + return (entry.ToFullPath(), isRealDirectory); + }, + EnumerationOptions.Compatible); + + foreach ((string childPath, bool isDirectory) in fse) { try { @@ -485,11 +488,10 @@ private static void RemoveDirectoryRecursive(string fullPath) throw firstException; } - bool removed = TryRemoveDirectory(fullPath); - Debug.Assert(removed); + RemoveEmptyDirectory(fullPath); } - private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, bool throwWhenNotEmpty = true) + private static bool RemoveEmptyDirectory(string fullPath, bool topLevel = false, bool throwWhenNotEmpty = true) { if (Interop.Sys.RmDir(fullPath) < 0) { @@ -502,12 +504,15 @@ private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, b case Interop.Error.EISDIR: throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, fullPath)); // match Win32 exception case Interop.Error.ENOENT: + // When we're recursing, don't throw for items that go missing. if (!topLevel) { return true; } goto default; case Interop.Error.ENOTDIR: + // When the top-level path is a symlink to a directory, delete the link. + // In other cases, throw because we expect path to be a real directory. if (topLevel) { if (!DirectoryExists(fullPath)) @@ -515,7 +520,6 @@ private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, b throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); } - // Path is symbolic link to a directory. DeleteFile(fullPath); return true; } From 599514a521b1e10433a73b8e442de75aeeff95ba Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 18 Nov 2021 19:46:47 +0100 Subject: [PATCH 3/6] test: use Directory.CreateSymbolicLink --- src/libraries/System.IO.FileSystem/tests/File/Delete.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs index b696838619b0da..2e0f828110a4f9 100644 --- a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs @@ -88,7 +88,7 @@ public void DeletingSymLinkDoesntDeleteTarget() var linkPath = GetTestFilePath(); File.Create(path).Dispose(); - Assert.True(MountHelper.CreateSymbolicLink(linkPath, path, isDirectory: false)); + Assert.NotNull(Directory.CreateSymbolicLink(linkPath, path)); // Both the symlink and the target exist Assert.True(File.Exists(path), "path should exist"); From 63953d709e928e499484d7678d3f8f65226152ef Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 22 Nov 2021 15:24:25 +0100 Subject: [PATCH 4/6] Run test against different Delete APIs --- .../tests/Directory/Delete.cs | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index f6f71a780cf3ec..1e12662b971ca0 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -119,35 +119,6 @@ public void DeletingSymLinkDoesntDeleteTarget() Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); } - [ConditionalFact(nameof(CanCreateSymbolicLinks))] - public void RecursiveDeletingDoesntFollowLinks() - { - var target = GetTestFilePath(); - Directory.CreateDirectory(target); - - var fileInTarget = Path.Combine(target, GetTestFileName()); - File.WriteAllText(fileInTarget, ""); - - var linkParent = GetTestFilePath(); - Directory.CreateDirectory(linkParent); - - var linkPath = Path.Combine(linkParent, GetTestFileName()); - Assert.True(MountHelper.CreateSymbolicLink(linkPath, target, isDirectory: true)); - - // Both the symlink and the target exist - Assert.True(Directory.Exists(target), "target should exist"); - Assert.True(Directory.Exists(linkPath), "linkPath should exist"); - Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); - - // Delete the parent folder of the symlink. - Directory.Delete(linkParent, true); - - // Target should still exist - Assert.True(Directory.Exists(target), "target should still exist"); - Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); - Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); - } - [ConditionalFact(nameof(UsingNewNormalization))] public void ExtendedDirectoryWithSubdirectories() { @@ -319,5 +290,34 @@ public void RecursiveDelete_ShouldThrowIOExceptionIfContainedFileInUse() } Assert.True(testDir.Exists); } + + [ConditionalFact(nameof(CanCreateSymbolicLinks))] + public void RecursiveDeletingDoesntFollowLinks() + { + var target = GetTestFilePath(); + Directory.CreateDirectory(target); + + var fileInTarget = Path.Combine(target, GetTestFileName()); + File.WriteAllText(fileInTarget, ""); + + var linkParent = GetTestFilePath(); + Directory.CreateDirectory(linkParent); + + var linkPath = Path.Combine(linkParent, GetTestFileName()); + Assert.NotNull(Directory.CreateSymbolicLink(linkPath, target)); + + // Both the symlink and the target exist + Assert.True(Directory.Exists(target), "target should exist"); + Assert.True(Directory.Exists(linkPath), "linkPath should exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + + // Delete the parent folder of the symlink. + Delete(linkParent, true); + + // Target should still exist + Assert.True(Directory.Exists(target), "target should still exist"); + Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + } } } From edb54b201d4e3ffc3f10a1011e31b50f494b6034 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 23 Nov 2021 11:11:32 +0100 Subject: [PATCH 5/6] test: update Exists check for Windows --- src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index 1e12662b971ca0..30872f3972775b 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -308,7 +308,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Both the symlink and the target exist Assert.True(Directory.Exists(target), "target should exist"); - Assert.True(Directory.Exists(linkPath), "linkPath should exist"); + Assert.True(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); // Delete the parent folder of the symlink. @@ -316,7 +316,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Target should still exist Assert.True(Directory.Exists(target), "target should still exist"); - Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); + Assert.False(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should no longer exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); } } From 90147934804b022f4be0eb608a67f7f0546a3f58 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 23 Nov 2021 13:43:22 +0100 Subject: [PATCH 6/6] Fix tests --- src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs | 4 ++-- src/libraries/System.IO.FileSystem/tests/File/Delete.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index 30872f3972775b..1e12662b971ca0 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -308,7 +308,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Both the symlink and the target exist Assert.True(Directory.Exists(target), "target should exist"); - Assert.True(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should exist"); + Assert.True(Directory.Exists(linkPath), "linkPath should exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); // Delete the parent folder of the symlink. @@ -316,7 +316,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Target should still exist Assert.True(Directory.Exists(target), "target should still exist"); - Assert.False(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should no longer exist"); + Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); } } diff --git a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs index 2e0f828110a4f9..b696838619b0da 100644 --- a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs @@ -88,7 +88,7 @@ public void DeletingSymLinkDoesntDeleteTarget() var linkPath = GetTestFilePath(); File.Create(path).Dispose(); - Assert.NotNull(Directory.CreateSymbolicLink(linkPath, path)); + Assert.True(MountHelper.CreateSymbolicLink(linkPath, path, isDirectory: false)); // Both the symlink and the target exist Assert.True(File.Exists(path), "path should exist");