Fix FileSystem enumeration with broken symlinks

Today when enumerating directories, we look only for entries with DT_DIR (directory) or DT_REG (regular file); if we encounter something unknown or a symlink, we stat to resolve it and determine whether it's actually an S_IFDIR (directory) or S_IFREG (regular file). There are (at least) two problems with this:

1) The stat to a symlink will fail if the symlink points to a non-existent entry.
2) We end up skipping lots of special files, e.g. FIFOs.

The commit changes our logic to simply special case directories: we treat everything as a file unless we're sure it's a directory. This directly fixes (2), and indirectly fixes (1): our logic for DirectoryExists already ignores any stat errors, returning false if the symlink couldn't be resolved, such that this naturally maps to our desired behavior of broken symlinks being categorized as a file.

Author: stephentoub

src/System.IO.FileSystem/src/System/IO/UnixFileSystem.cs

@@ -318,7 +318,7 @@ public override bool DirectoryExists(string fullPath)
             return DirectoryExists(fullPath, out errno);
         }
 
-        private bool DirectoryExists(string fullPath, out int errno)
+        private static bool DirectoryExists(string fullPath, out int errno)
         {
             return FileExists(fullPath, Interop.libcoreclr.FileTypes.S_IFDIR, out errno);
         }
@@ -329,7 +329,7 @@ public override bool FileExists(string fullPath)
             return FileExists(fullPath, Interop.libcoreclr.FileTypes.S_IFREG, out errno);
         }
 
-        private bool FileExists(string fullPath, int fileType, out int errno)
+        private static bool FileExists(string fullPath, int fileType, out int errno)
         {
             Interop.libcoreclr.fileinfo fileinfo;
             while (true)
@@ -475,48 +475,53 @@ private IEnumerator<T> Enumerate(Interop.libc.SafeDirHandle dirHandle)
                             string name = Interop.libc.GetDirEntName(curEntry);
 
                             // Get from the dir entry whether the entry is a file or directory.
-                            // If we're not sure from the dir entry itself, stat to the entry.
-                            bool isDir = false, isFile = false;
+                            // We classify everything as a file unless we know it to be a directory,
+                            // e.g. a FIFO will be classified as a file.
+                            bool isDir;
                             switch (Interop.libc.GetDirEntType(curEntry))
                             {
                                 case Interop.libc.DType.DT_DIR:
+                                    // We know it's a directory.
                                     isDir = true;
                                     break;
-                                case Interop.libc.DType.DT_REG:
-                                    isFile = true;
-                                    break;
                                 case Interop.libc.DType.DT_LNK:
                                 case Interop.libc.DType.DT_UNKNOWN:
-                                    string fullPath = Path.Combine(dirPath.FullPath, name);
-                                    Interop.libcoreclr.fileinfo fileinfo;
-                                    while (Interop.CheckIo(Interop.libcoreclr.GetFileInformationFromPath(fullPath, out fileinfo), fullPath)) ;
-                                    isDir = (fileinfo.mode & Interop.libcoreclr.FileTypes.S_IFMT) == Interop.libcoreclr.FileTypes.S_IFDIR;
-                                    isFile = (fileinfo.mode & Interop.libcoreclr.FileTypes.S_IFMT) == Interop.libcoreclr.FileTypes.S_IFREG;
+                                    // It's a symlink or unknown: stat to it to see if we can resolve it to a directory.
+                                    // If we can't (e.g.symlink to a file, broken symlink, etc.), we'll just treat it as a file.
+                                    int errnoIgnored;
+                                    isDir = DirectoryExists(Path.Combine(dirPath.FullPath, name), out errnoIgnored);
+                                    break;
+                                default:
+                                    // Otherwise, treat it as a file.  This includes regular files,
+                                    // FIFOs, etc.
+                                    isDir = false;
                                     break;
                             }
-                            bool matchesSearchPattern =
-                                (isFile || isDir) &&
-                                Interop.libc.fnmatch(_searchPattern, name, Interop.libc.FnmatchFlags.None) == 0;
 
                             // Yield the result if the user has asked for it.  In the case of directories,
                             // always explore it by pushing it onto the stack, regardless of whether
                             // we're returning directories.
-                            if (isDir && !ShouldIgnoreDirectory(name))
+                            if (isDir)
                             {
-                                if (_includeDirectories && matchesSearchPattern)
-                                {
-                                    yield return _translateResult(Path.Combine(dirPath.UserPath, name), /*isDirectory*/true);
-                                }
-                                if (_searchOption == SearchOption.AllDirectories)
+                                if (!ShouldIgnoreDirectory(name))
                                 {
-                                    if (toExplore == null)
+                                    if (_includeDirectories &&
+                                        Interop.libc.fnmatch(_searchPattern, name, Interop.libc.FnmatchFlags.None) == 0)
+                                    {
+                                        yield return _translateResult(Path.Combine(dirPath.UserPath, name), /*isDirectory*/true);
+                                    }
+                                    if (_searchOption == SearchOption.AllDirectories)
                                     {
-                                        toExplore = new Stack<PathPair>();
+                                        if (toExplore == null)
+                                        {
+                                            toExplore = new Stack<PathPair>();
+                                        }
+                                        toExplore.Push(new PathPair(Path.Combine(dirPath.UserPath, name), Path.Combine(dirPath.FullPath, name)));
                                     }
-                                    toExplore.Push(new PathPair(Path.Combine(dirPath.UserPath, name), Path.Combine(dirPath.FullPath, name)));
                                 }
                             }
-                            else if (isFile && _includeFiles && matchesSearchPattern)
+                            else if (_includeFiles &&
+                                     Interop.libc.fnmatch(_searchPattern, name, Interop.libc.FnmatchFlags.None) == 0)
                             {
                                 yield return _translateResult(Path.Combine(dirPath.UserPath, name), /*isDirectory*/false);
                             }

src/System.IO.FileSystem/tests/Directory/GetDirectories_str.cs

@@ -3,11 +3,9 @@
 
 using System;
 using System.IO;
-using System.Collections;
-using System.Globalization;
-using System.Text;
-using System.Threading;
+using System.Linq;
 using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
 using Xunit;
 
 public class Directory_GetDirectories_str
@@ -347,6 +345,33 @@ public static void printerr(String err, [CallerMemberName] string memberName = "
     {
         Console.WriteLine("ERROR: ({0}, {1}, {2}) {3}", memberName, filePath, lineNumber, err);
     }
+
+    [Fact]
+    [PlatformSpecific(PlatformID.AnyUnix)]
+    public void EnumerateWithSymLinkToDirectory()
+    {
+        using (var containingFolder = new TemporaryDirectory())
+        {
+            // Test a symlink to a directory that does and then doesn't exist
+            using (var targetDir = new TemporaryDirectory())
+            {
+                // Create a symlink to a folder that exists
+                string linkPath = Path.Combine(containingFolder.Path, Path.GetRandomFileName());
+                Assert.Equal(0, symlink(targetDir.Path, linkPath));
+                Assert.True(Directory.Exists(linkPath));
+                Assert.Equal(1, Directory.GetDirectories(containingFolder.Path).Count());
+            }
+
+            // The target file is gone and the symlink still exists; since it can't be resolved,
+            // it's treated as a file rather than as a directory.
+            Assert.Equal(0, Directory.GetDirectories(containingFolder.Path).Count());
+            Assert.Equal(1, Directory.GetFiles(containingFolder.Path).Count());
+        }
+    }
+
+    [DllImport("libc", SetLastError = true)]
+    private static extern int symlink(string path1, string path2);
+
 }
 
 

src/System.IO.FileSystem/tests/Directory/GetFiles_str.cs

@@ -4,6 +4,8 @@
 using System;
 using System.IO;
 using System.IO.FileSystem.Tests;
+using System.Linq;
+using System.Runtime.InteropServices;
 using Xunit;
 
 public class Directory_GetFiles_str : FileSystemTest
@@ -156,5 +158,34 @@ public void UnixFilePathWithSpaces()
         }
     }
 
+    [Fact]
+    [PlatformSpecific(PlatformID.AnyUnix)]
+    public void EnumerateWithSymLinkToFile()
+    {
+        using (var containingFolder = new TemporaryDirectory())
+        {
+            string linkPath;
+
+            // Test a symlink to a file that does and then doesn't exist
+            using (var targetFile = new TemporaryFile())
+            {
+                linkPath = Path.Combine(containingFolder.Path, Path.GetRandomFileName());
+                Assert.Equal(0, symlink(targetFile.Path, linkPath));
+                Assert.True(File.Exists(linkPath));
+                Assert.Equal(1, GetFiles(containingFolder.Path).Count());
+            }
+
+            // The symlink still exists even though the target file is gone.
+            Assert.Equal(1, GetFiles(containingFolder.Path).Count());
+
+            // The symlink is gone
+            File.Delete(linkPath);
+            Assert.Equal(0, GetFiles(containingFolder.Path).Count());
+        }
+    }
+
+    [DllImport("libc", SetLastError = true)]
+    private static extern int symlink(string path1, string path2);
+    
     #endregion
 }