Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove validation to stat call for symlinks since is a breaking change #57551

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Remove validation to stat call for symlinks since is a breaking change #57551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a90126b
Remove validation to stat call for symlinks since is a breaking chang…
jozkee Aug 17, 2021
d705eb7
Undo try-catch workaround in PhysicalFileProvider
jozkee Aug 17, 2021
e6c8835
Fix tests that were failing due to changes
jozkee Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,72 +52,6 @@ protected override void AssertLinkExists(FileSystemInfo link)
}
}

[Theory]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests were validating that we throw when a self-referencing symlink is found in the recursion, while that is true for windows, that wasn't the case for Unix in 5.0, and it was going to be a breaking change caused by the same validation that I'm removing, so I updated the tests to ensure what is expected matches with 5.0

[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
Open in desktop
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
Open in desktop
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