From a0fdc256404ee92455297ebcfe598f64ac1b7526 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 23 Nov 2021 15:51:37 +0100 Subject: [PATCH] FileSystem.Unix: Directory.Delete: remove per item syscall. (#59520) * 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. --- .../System.IO.FileSystem/tests/Directory/Delete.cs | 29 ++++++ .../src/System/IO/FileSystem.Unix.cs | 115 ++++++++++++--------- 2 files changed, 97 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 b264ec4..1e12662 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -290,5 +290,34 @@ namespace System.IO.Tests } 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"); + } } } 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 e5fd857..3367fd0 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; @@ -429,69 +430,66 @@ namespace System.IO public static void RemoveDirectory(string fullPath, bool recursive) { - var di = new DirectoryInfo(fullPath); - if (!di.Exists) + // 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)) { - throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); + Debug.Assert(recursive); + + 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; - } + 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); - if (recursive) - { - try + foreach ((string childPath, bool isDirectory) in fse) { - 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) + RemoveEmptyDirectory(fullPath); + } + + private static bool RemoveEmptyDirectory(string fullPath, bool topLevel = false, bool throwWhenNotEmpty = true) + { + if (Interop.Sys.RmDir(fullPath) < 0) { Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo(); switch (errorInfo.Error) @@ -500,17 +498,40 @@ namespace System.IO 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) + // When we're recursing, don't throw for items that go missing. + if (!topLevel) { - return; + 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)) + { + throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); + } + + 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. -- 2.7.4