From f7ba49d726592baebed130b2006728518e53713a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Tue, 17 Aug 2021 11:51:46 -0700 Subject: [PATCH] Remove validation to stat call for symlinks since is a breaking change (#57551) * Remove validation to stat call for symlinks since is a breaking change, subsequently remove the symlink cache logic as is no longer needed * Undo try-catch workaround in PhysicalFileProvider * Fix tests that were failing due to changes --- .../src/PollingFileChangeToken.cs | 13 +--- .../Base/SymbolicLinks/BaseSymbolicLinks.cs | 6 ++ .../tests/DirectoryInfo/SymbolicLinks.cs | 66 ---------------- .../tests/Enumeration/SymbolicLinksTests.cs | 76 +++++++++++++++++++ .../src/System/IO/FileStatus.Unix.cs | 41 +++------- 5 files changed, 92 insertions(+), 110 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs b/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs index 6d8f70b47f7d0..ddf2ae07423f0 100644 --- a/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs +++ b/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs @@ -54,18 +54,7 @@ private DateTime GetLastWriteTimeUtc() return DateTime.MinValue; } - DateTime? lastWriteTimeUtc = FileSystemInfoHelper.GetFileLinkTargetLastWriteTimeUtc(_fileInfo); - - if (lastWriteTimeUtc == null) - { - try - { - lastWriteTimeUtc = _fileInfo.LastWriteTimeUtc; - } - catch (IOException) { } // https://github.com/dotnet/runtime/issues/57221 - } - - return lastWriteTimeUtc ?? DateTime.MinValue; + return FileSystemInfoHelper.GetFileLinkTargetLastWriteTimeUtc(_fileInfo) ?? _fileInfo.LastWriteTimeUtc; } /// diff --git a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs index d8869ff158121..f120cdf10eba5 100644 --- a/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs +++ b/src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs @@ -21,6 +21,12 @@ protected DirectoryInfo CreateDirectoryContainingSelfReferencingSymbolicLink() return testDirectory; } + protected DirectoryInfo CreateSelfReferencingSymbolicLink() + { + string path = GetRandomDirPath(); + return (DirectoryInfo)Directory.CreateSymbolicLink(path, path); + } + protected string GetRandomFileName() => GetTestFileName() + ".txt"; protected string GetRandomLinkName() => GetTestFileName() + ".link"; protected string GetRandomDirName() => GetTestFileName() + "_dir"; diff --git a/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/SymbolicLinks.cs b/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/SymbolicLinks.cs index e7ee5d0c6727c..430a1da6e4f84 100644 --- a/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/SymbolicLinks.cs +++ b/src/libraries/System.IO.FileSystem/tests/DirectoryInfo/SymbolicLinks.cs @@ -52,72 +52,6 @@ protected override void AssertLinkExists(FileSystemInfo link) } } - [Theory] - [InlineData(false)] - [InlineData(true)] - public void EnumerateDirectories_LinksWithCycles_ThrowsTooManyLevelsOfSymbolicLinks(bool recurse) - { - var options = new EnumerationOptions() { RecurseSubdirectories = recurse }; - DirectoryInfo testDirectory = CreateDirectoryContainingSelfReferencingSymbolicLink(); - - // Windows avoids accessing the cyclic symlink if we do not recurse - if (OperatingSystem.IsWindows() && !recurse) - { - testDirectory.EnumerateDirectories("*", options).Count(); - testDirectory.GetDirectories("*", options).Count(); - } - else - { - // Internally transforms the FileSystemEntry to a DirectoryInfo, which performs a disk hit on the cyclic symlink - Assert.Throws(() => testDirectory.EnumerateDirectories("*", options).Count()); - Assert.Throws(() => testDirectory.GetDirectories("*", options).Count()); - } - } - - [Theory] - [InlineData(false)] - [InlineData(true)] - public void EnumerateFiles_LinksWithCycles_ThrowsTooManyLevelsOfSymbolicLinks(bool recurse) - { - var options = new EnumerationOptions() { RecurseSubdirectories = recurse }; - DirectoryInfo testDirectory = CreateDirectoryContainingSelfReferencingSymbolicLink(); - - // Windows avoids accessing the cyclic symlink if we do not recurse - if (OperatingSystem.IsWindows() && !recurse) - { - testDirectory.EnumerateFiles("*", options).Count(); - testDirectory.GetFiles("*", options).Count(); - } - else - { - // Internally transforms the FileSystemEntry to a FileInfo, which performs a disk hit on the cyclic symlink - Assert.Throws(() => testDirectory.EnumerateFiles("*", options).Count()); - Assert.Throws(() => testDirectory.GetFiles("*", options).Count()); - } - } - - [Theory] - [InlineData(false)] - [InlineData(true)] - public void EnumerateFileSystemInfos_LinksWithCycles_ThrowsTooManyLevelsOfSymbolicLinks(bool recurse) - { - var options = new EnumerationOptions() { RecurseSubdirectories = recurse }; - DirectoryInfo testDirectory = CreateDirectoryContainingSelfReferencingSymbolicLink(); - - // Windows avoids accessing the cyclic symlink if we do not recurse - if (OperatingSystem.IsWindows() && !recurse) - { - testDirectory.EnumerateFileSystemInfos("*", options).Count(); - testDirectory.GetFileSystemInfos("*", options).Count(); - } - else - { - // Internally transforms the FileSystemEntry to a FileSystemInfo, which performs a disk hit on the cyclic symlink - Assert.Throws(() => testDirectory.EnumerateFileSystemInfos("*", options).Count()); - Assert.Throws(() => testDirectory.GetFileSystemInfos("*", options).Count()); - } - } - [Fact] public void ResolveLinkTarget_Throws_NotExists() => ResolveLinkTarget_Throws_NotExists_Internal(); diff --git a/src/libraries/System.IO.FileSystem/tests/Enumeration/SymbolicLinksTests.cs b/src/libraries/System.IO.FileSystem/tests/Enumeration/SymbolicLinksTests.cs index 43ea97540f38b..8365b85027ce6 100644 --- a/src/libraries/System.IO.FileSystem/tests/Enumeration/SymbolicLinksTests.cs +++ b/src/libraries/System.IO.FileSystem/tests/Enumeration/SymbolicLinksTests.cs @@ -61,5 +61,81 @@ public void EnumerateFileSystemEntries_LinksWithCycles_ShouldNotThrow() Assert.Single(enumerable); } + + [ConditionalTheory(nameof(CanCreateSymbolicLinks))] + [InlineData(false, false)] // OK + [InlineData(false, true)] // throw + [InlineData(true, false)] // throw, OK on Unix + [InlineData(true, true)] // throw + public void EnumerateGet_SelfReferencingLink_Instance(bool recurse, bool linkAsRoot) + { + var options = new EnumerationOptions() { RecurseSubdirectories = recurse }; + + DirectoryInfo testDirectory = linkAsRoot ? + CreateSelfReferencingSymbolicLink() : + CreateDirectoryContainingSelfReferencingSymbolicLink(); + + // Unix doesn't have a problem when it steps in a self-referencing link through the directory recursion. + if ((!recurse || !OperatingSystem.IsWindows()) && !linkAsRoot) + { + testDirectory.EnumerateFileSystemInfos("*", options).Count(); + testDirectory.GetFileSystemInfos("*", options).Count(); + + testDirectory.EnumerateDirectories("*", options).Count(); + testDirectory.GetDirectories("*", options).Count(); + + testDirectory.EnumerateFiles("*", options).Count(); + testDirectory.GetFiles("*", options).Count(); + } + else + { + Assert.Throws(() => testDirectory.EnumerateFileSystemInfos("*", options).Count()); + Assert.Throws(() => testDirectory.GetFileSystemInfos("*", options).Count()); + + Assert.Throws(() => testDirectory.EnumerateDirectories("*", options).Count()); + Assert.Throws(() => testDirectory.GetDirectories("*", options).Count()); + + Assert.Throws(() => testDirectory.EnumerateFiles("*", options).Count()); + Assert.Throws(() => testDirectory.GetFiles("*", options).Count()); + } + } + + [ConditionalTheory(nameof(CanCreateSymbolicLinks))] + [InlineData(false, false)] // OK + [InlineData(false, true)] // throw + [InlineData(true, false)] // throw, OK on Unix + [InlineData(true, true)] // throw + public void EnumerateGet_SelfReferencingLink_Static(bool recurse, bool linkAsRoot) + { + var options = new EnumerationOptions() { RecurseSubdirectories = recurse }; + + DirectoryInfo testDirectory = linkAsRoot ? + CreateSelfReferencingSymbolicLink() : + CreateDirectoryContainingSelfReferencingSymbolicLink(); + + // Unix doesn't have a problem when it steps in a self-referencing link through the directory recursion. + if ((!recurse || !OperatingSystem.IsWindows()) && !linkAsRoot) + { + Directory.EnumerateFileSystemEntries(testDirectory.FullName, "*", options).Count(); + Directory.GetFileSystemEntries(testDirectory.FullName, "*", options).Count(); + + Directory.EnumerateDirectories(testDirectory.FullName, "*", options).Count(); + Directory.GetDirectories(testDirectory.FullName, "*", options).Count(); + + Directory.EnumerateFiles(testDirectory.FullName, "*", options).Count(); + Directory.GetFiles(testDirectory.FullName, "*", options).Count(); + } + else + { + Assert.Throws(() => Directory.EnumerateFileSystemEntries(testDirectory.FullName, "*", options).Count()); + Assert.Throws(() => Directory.GetFileSystemEntries(testDirectory.FullName, "*", options).Count()); + + Assert.Throws(() => Directory.EnumerateDirectories(testDirectory.FullName, "*", options).Count()); + Assert.Throws(() => Directory.GetDirectories(testDirectory.FullName, "*", options).Count()); + + Assert.Throws(() => Directory.EnumerateFiles(testDirectory.FullName, "*", options).Count()); + Assert.Throws(() => Directory.GetFiles(testDirectory.FullName, "*", options).Count()); + } + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs index fa1d8bf5d8736..ff4d8a86cb18c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs @@ -18,15 +18,6 @@ internal struct FileStatus // Positive number: the error code returned by the lstat call private int _initializedFileCache; - // The last cached stat information about the file - // Refresh only collects this if lstat determines the path is a symbolic link - private Interop.Sys.FileStatus _symlinkCache; - - // -1: if the symlink cache isn't initialized - Refresh only changes this value if lstat determines the path is a symbolic link - // 0: if the symlink cache was initialized with no errors - // Positive number: the error code returned by the stat call - private int _initializedSymlinkCache; - // We track intent of creation to know whether or not we want to (1) create a // DirectoryInfo around this status struct or (2) actually are part of a DirectoryInfo. // Set to true during initialization when the DirectoryEntry's INodeType describes a directory @@ -113,7 +104,6 @@ private bool HasSymbolicLinkFlag internal void InvalidateCaches() { _initializedFileCache = -1; - _initializedSymlinkCache = -1; } internal bool IsReadOnly(ReadOnlySpan path, bool continueOnError = false) @@ -340,7 +330,7 @@ internal long GetLength(ReadOnlySpan path, bool continueOnError = false) return _fileCache.Size; } - // Tries to refresh the lstat cache (_fileCache) and, if the file is pointing to a symbolic link, then also the stat cache (_symlinkCache) + // Tries to refresh the lstat cache (_fileCache). // This method should not throw. Instead, we store the results, and we will throw when the user attempts to access any of the properties when there was a failure internal void RefreshCaches(ReadOnlySpan path) { @@ -348,9 +338,8 @@ internal void RefreshCaches(ReadOnlySpan path) path = Path.TrimEndingDirectorySeparator(path); // Retrieve the file cache (lstat) to get the details on the object, without following symlinks. - // If it is a symlink, then subsequently get details on the target of the symlink, - // storing those results separately. We only report failure if the initial - // lstat fails, as a broken symlink should still report info on exists, attributes, etc. + // If it is a symlink, then subsequently get details on the target of the symlink. + // We only report failure if the initial lstat fails, as a broken symlink should still report info on exists, attributes, etc. if (!TryRefreshFileCache(path)) { _exists = false; @@ -361,11 +350,11 @@ internal void RefreshCaches(ReadOnlySpan path) _isDirectory = CacheHasDirectoryFlag(_fileCache); // We also need to check if the main path is a symbolic link, - // in which case, we retrieve the symbolic link data - if (!_isDirectory && HasSymbolicLinkFlag && TryRefreshSymbolicLinkCache(path)) + // in which case, we retrieve the symbolic link's target data + if (!_isDirectory && HasSymbolicLinkFlag && Interop.Sys.Stat(path, out Interop.Sys.FileStatus target) == 0) { // and check again if the symlink path is a directory - _isDirectory = CacheHasDirectoryFlag(_symlinkCache); + _isDirectory = CacheHasDirectoryFlag(target); } #if !TARGET_BROWSER @@ -376,7 +365,7 @@ internal void RefreshCaches(ReadOnlySpan path) _exists = true; - // Checks if the specified cache (lstat=_fileCache, stat=_symlinkCache) has the directory attribute set + // Checks if the specified cache has the directory attribute set // Only call if Refresh has been successfully called at least once, and you're // certain the passed-in cache was successfully retrieved static bool CacheHasDirectoryFlag(Interop.Sys.FileStatus cache) => @@ -384,8 +373,7 @@ static bool CacheHasDirectoryFlag(Interop.Sys.FileStatus cache) => } // Checks if the file cache is set to -1 and refreshes it's value. - // Optionally, if the symlink cache is set to -1 and the file cache determined the path is pointing to a symbolic link, this cache is also refreshed. - // If stat or lstat failed, and continueOnError is set to true, this method will throw. + // If it failed, and continueOnError is set to true, this method will throw. internal void EnsureCachesInitialized(ReadOnlySpan path, bool continueOnError = false) { if (_initializedFileCache == -1) @@ -402,18 +390,10 @@ internal void EnsureCachesInitialized(ReadOnlySpan path, bool continueOnEr // Throws if any of the caches has an error number saved in it private void ThrowOnCacheInitializationError(ReadOnlySpan path) { - int errno; // Lstat should always be initialized by Refresh if (_initializedFileCache != 0) { - errno = _initializedFileCache; - InvalidateCaches(); - throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(errno), new string(path)); - } - // Stat is optionally initialized when Refresh detects object is a symbolic link - else if (_initializedSymlinkCache != 0 && _initializedSymlinkCache != -1) - { - errno = _initializedSymlinkCache; + int errno = _initializedFileCache; InvalidateCaches(); throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(errno), new string(path)); } @@ -422,9 +402,6 @@ private void ThrowOnCacheInitializationError(ReadOnlySpan path) private bool TryRefreshFileCache(ReadOnlySpan path) => VerifyStatCall(Interop.Sys.LStat(path, out _fileCache), out _initializedFileCache); - private bool TryRefreshSymbolicLinkCache(ReadOnlySpan path) => - VerifyStatCall(Interop.Sys.Stat(path, out _symlinkCache), out _initializedSymlinkCache); - // Receives the return value of a stat or lstat call. // If the call is unsuccessful, sets the initialized parameter to a positive number representing the last error info. // If the call is successful, sets the initialized parameter to zero.