Skip to content

Commit

Permalink
Remove validation to stat call for symlinks since is a breaking change (
Browse files Browse the repository at this point in the history 
#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
  • Loading branch information
@jozkee
jozkee authored Aug 17, 2021
1 parent 52ae8c3 commit f7ba49d
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 110 deletions.
13 changes: 1 addition & 12 deletions src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/// <summary>
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
66 changes: 0 additions & 66 deletions src/libraries/System.IO.FileSystem/tests/DirectoryInfo/SymbolicLinks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<IOException>(() => testDirectory.EnumerateDirectories("*", options).Count());
Assert.Throws<IOException>(() => 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<IOException>(() => testDirectory.EnumerateFiles("*", options).Count());
Assert.Throws<IOException>(() => 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<IOException>(() => testDirectory.EnumerateFileSystemInfos("*", options).Count());
Assert.Throws<IOException>(() => testDirectory.GetFileSystemInfos("*", options).Count());
}
}

[Fact]
public void ResolveLinkTarget_Throws_NotExists() =>
ResolveLinkTarget_Throws_NotExists_Internal<DirectoryNotFoundException>();
Expand Down
76 changes: 76 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/Enumeration/SymbolicLinksTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<IOException>(() => testDirectory.EnumerateFileSystemInfos("*", options).Count());
Assert.Throws<IOException>(() => testDirectory.GetFileSystemInfos("*", options).Count());

Assert.Throws<IOException>(() => testDirectory.EnumerateDirectories("*", options).Count());
Assert.Throws<IOException>(() => testDirectory.GetDirectories("*", options).Count());

Assert.Throws<IOException>(() => testDirectory.EnumerateFiles("*", options).Count());
Assert.Throws<IOException>(() => 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<IOException>(() => Directory.EnumerateFileSystemEntries(testDirectory.FullName, "*", options).Count());
Assert.Throws<IOException>(() => Directory.GetFileSystemEntries(testDirectory.FullName, "*", options).Count());

Assert.Throws<IOException>(() => Directory.EnumerateDirectories(testDirectory.FullName, "*", options).Count());
Assert.Throws<IOException>(() => Directory.GetDirectories(testDirectory.FullName, "*", options).Count());

Assert.Throws<IOException>(() => Directory.EnumerateFiles(testDirectory.FullName, "*", options).Count());
Assert.Throws<IOException>(() => Directory.GetFiles(testDirectory.FullName, "*", options).Count());
}
}
}
}
41 changes: 9 additions & 32 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -113,7 +104,6 @@ private bool HasSymbolicLinkFlag
internal void InvalidateCaches()
{
_initializedFileCache = -1;
_initializedSymlinkCache = -1;
}

internal bool IsReadOnly(ReadOnlySpan<char> path, bool continueOnError = false)
Expand Down Expand Up @@ -340,17 +330,16 @@ internal long GetLength(ReadOnlySpan<char> 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<char> path)
{
_isDirectory = false;
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;
Expand All @@ -361,11 +350,11 @@ internal void RefreshCaches(ReadOnlySpan<char> 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
Expand All @@ -376,16 +365,15 @@ internal void RefreshCaches(ReadOnlySpan<char> 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) =>
(cache.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR;
}

// 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<char> path, bool continueOnError = false)
{
if (_initializedFileCache == -1)
Expand All @@ -402,18 +390,10 @@ internal void EnsureCachesInitialized(ReadOnlySpan<char> path, bool continueOnEr
// Throws if any of the caches has an error number saved in it
private void ThrowOnCacheInitializationError(ReadOnlySpan<char> 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));
}
Expand All @@ -422,9 +402,6 @@ private void ThrowOnCacheInitializationError(ReadOnlySpan<char> path)
private bool TryRefreshFileCache(ReadOnlySpan<char> path) =>
VerifyStatCall(Interop.Sys.LStat(path, out _fileCache), out _initializedFileCache);

private bool TryRefreshSymbolicLinkCache(ReadOnlySpan<char> 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.
Expand Down

0 comments on commit f7ba49d

Please sign in to comment.