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.

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

Fix: Avoid throwing if a symbolic link cycle to itself is detected while enumerating #52749

Merged
merged 9 commits into from
May 24, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,17 @@ internal static FileAttributes Initialize(
if (isUnknown)
{
isSymlink = entry.IsSymbolicLink;
isDirectory = entry._status.IsDirectory(entry.FullPath);
// Need to fail silently in case we are enumerating
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the comment... isn't all of this code about enumerating?

Copy link
Member Author

Choose a reason for hiding this comment

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

Enumeration always goes through this path, yes. I do not know if in the future we will reach FileSystemEntry.Initialize another way.

Next time I touch this code, I can change the comment to // This call needs to fail silently.

isDirectory = entry._status.IsDirectory(entry.FullPath, continueOnError: true);
}
// Same idea as the directory check, just repeated for (and tweaked due to the
// nature of) symlinks.
// Whether we had the dirent structure or not, we treat a symlink to a directory as a directory,
// so we need to reflect that in our isDirectory variable.
else if (isSymlink)
{
isDirectory = entry._status.IsDirectory(entry.FullPath);
// Need to fail silently in case we are enumerating
isDirectory = entry._status.IsDirectory(entry.FullPath, continueOnError: true);
}

entry._status.InitiallyDirectory = isDirectory;
Expand Down
18 changes: 18 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/Base/BaseSymbolicLinks.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Xunit;

namespace System.IO.Tests
{
public abstract class BaseSymbolicLinks : FileSystemTest
{
protected DirectoryInfo CreateDirectorySymbolicLinkToItself()
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
protected DirectoryInfo CreateDirectorySymbolicLinkToItself()
protected DirectoryInfo CreateDirectoryContainingSymbolicLinkToItself()

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected DirectoryInfo CreateDirectorySymbolicLinkToItself()
protected DirectoryInfo CreateDirectoryContainingSelfReferencingSymbolicLink()

{
DirectoryInfo testDirectory = Directory.CreateDirectory(GetTestFilePath());
string pathToLink = Path.Join(testDirectory.FullName, GetTestFileName());
Assert.True(MountHelper.CreateSymbolicLink(pathToLink, pathToLink, isDirectory: true)); // Create a symlink cycle
return testDirectory;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Linq;
using Xunit;

namespace System.IO.Tests
{
public class Directory_SymbolicLinks : BaseSymbolicLinks
{
[Fact]
public void EnumerateDirectories_LinksWithCycles_ShouldNotThrow()
{
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();

// Windows differentiates between dir symlinks and file symlinks
int expected = PlatformDetection.IsWindows ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

very minor nit: since System.IO.FileSystem.Tests does not target older TFMs, you can use OperatingSystem.IsWindows()

Suggested change
int expected = PlatformDetection.IsWindows ? 1 : 0;
int expected = OperatingSystem.IsWindows() ? 1 : 0;

carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(expected, Directory.EnumerateDirectories(testDirectory.FullName).Count());
}

[Fact]
public void EnumerateFiles_LinksWithCycles_ShouldNotThrow()
{
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();

// Windows differentiates between dir symlinks and file symlinks
int expected = PlatformDetection.IsWindows ? 0 : 1;
Assert.Equal(expected, Directory.EnumerateFiles(testDirectory.FullName).Count());
}

[Fact]
public void EnumerateFileSystemEntries_LinksWithCycles_ShouldNotThrow()
{
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(1, Directory.EnumerateFileSystemEntries(testDirectory.FullName).Count());
Copy link
Member

Choose a reason for hiding this comment

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

nit: xUnit analyzer would suggest using Single: https://stackoverflow.com/a/46654308/5852046

Suggested change
Assert.Equal(1, Directory.EnumerateFileSystemEntries(testDirectory.FullName).Count());
Assert.Single(Directory.EnumerateFileSystemEntries(testDirectory.FullName));

Copy link
Member Author

Choose a reason for hiding this comment

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

Strangely, I did not get the suggestion. But I can change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing I need to confirm is if calling Single will force the creation of the objects, like Count() does, so that the disk hit is performed. If it's some kind of lazy count, it won't work.

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Linq;
using Xunit;

namespace System.IO.Tests
{
public class DirectoryInfo_SymbolicLinks : BaseSymbolicLinks
{
[Theory]
[InlineData(false)]
[InlineData(true)]
public void EnumerateDirectories_LinksWithCycles_ThrowsTooManyLevelsOfSymbolicLinks(bool recurse)
{
var options = new EnumerationOptions() { RecurseSubdirectories = recurse };
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();

// Windows avoids accessing the cyclic symlink if we do not recurse
if (PlatformDetection.IsWindows && !recurse)
{
testDirectory.EnumerateDirectories("*", options).Count();
testDirectory.GetDirectories("*", options).Count();
Comment on lines +23 to +24
Copy link
Member

Choose a reason for hiding this comment

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

Assert how many entries this gets.

}
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 = CreateDirectorySymbolicLinkToItself();

// Windows avoids accessing the cyclic symlink if we do not recurse
if (PlatformDetection.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());
jozkee marked this conversation as resolved.
Show resolved Hide resolved
}
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void EnumerateFileSystemInfos_LinksWithCycles_ThrowsTooManyLevelsOfSymbolicLinks(bool recurse)
{
var options = new EnumerationOptions() { RecurseSubdirectories = recurse };
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();

// Windows avoids accessing the cyclic symlink if we do not recurse
if (PlatformDetection.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());
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.IO.Enumeration;
using System.Linq;
using Xunit;

namespace System.IO.Tests.Enumeration
{
public class Enumeration_SymbolicLinksTests : BaseSymbolicLinks
{
[Fact]
public void EnumerateDirectories_LinksWithCycles_ShouldNotThrow()
{
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();

IEnumerable<string> enumerable = new FileSystemEnumerable<string>(
testDirectory.FullName,
(ref FileSystemEntry entry) => entry.ToFullPath(),
// Skipping attributes would force a disk hit which enters the cyclic symlink
new EnumerationOptions(){ AttributesToSkip = 0 })
{
ShouldIncludePredicate = (ref FileSystemEntry entry) => entry.IsDirectory
};

// Windows differentiates between dir symlinks and file symlinks
int expected = PlatformDetection.IsWindows ? 1 : 0;
Assert.Equal(expected, enumerable.Count());
}

[Fact]
public void EnumerateFiles_LinksWithCycles_ShouldNotThrow()
{
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();

IEnumerable<string> enumerable = new FileSystemEnumerable<string>(
testDirectory.FullName,
(ref FileSystemEntry entry) => entry.ToFullPath(),
// Skipping attributes would force a disk hit which enters the cyclic symlink
new EnumerationOptions(){ AttributesToSkip = 0 })
{
ShouldIncludePredicate = (ref FileSystemEntry entry) => !entry.IsDirectory
};

// Windows differentiates between dir symlinks and file symlinks
int expected = PlatformDetection.IsWindows ? 0 : 1;
Assert.Equal(expected, enumerable.Count());
}

[Fact]
public void EnumerateFileSystemEntries_LinksWithCycles_ShouldNotThrow()
{
DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();

IEnumerable<string> enumerable = new FileSystemEnumerable<string>(
testDirectory.FullName,
(ref FileSystemEntry entry) => entry.ToFullPath(),
// Skipping attributes would force a disk hit which enters the cyclic symlink
new EnumerationOptions(){ AttributesToSkip = 0 });

Assert.Equal(1, enumerable.Count());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Assert.Equal(1, enumerable.Count());
Assert.Single(enumerable);

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
<Compile Include="Base\InfoGetSetTimes.cs" />
<Compile Include="Base\AllGetSetAttributes.cs" />
<Compile Include="Base\StaticGetSetTimes.cs" />
<Compile Include="Base\BaseSymbolicLinks.cs" />
<Compile Include="Directory\EnumerableTests.cs" />
<Compile Include="Directory\SymbolicLinks.cs" />
<Compile Include="DirectoryInfo\SymbolicLinks.cs" />
<Compile Include="FileInfo\GetSetAttributesCommon.cs" />
<Compile Include="FileInfo\IsReadOnly.cs" />
<Compile Include="FileInfo\Replace.cs" />
Expand Down Expand Up @@ -46,6 +49,7 @@
<Compile Include="Enumeration\MatchTypesTests.cs" />
<Compile Include="Enumeration\ExampleTests.cs" />
<Compile Include="Enumeration\RemovedDirectoryTests.cs" />
<Compile Include="Enumeration\SymbolicLinksTests.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetsUnix)' == 'true'">
<Compile Include="FileSystemTest.Unix.cs" />
Expand Down