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

carlossanlop · 2021-05-14T08:05:16Z

ghost · 2021-05-14T08:05:20Z

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO`
Milestone:	6.0.0

adamsitnik

LGTM, @carlossanlop thank you for adding the description and a lot of tests, it made the reviewing much easier for me!

adamsitnik · 2021-05-14T08:27:44Z

src/libraries/System.IO.FileSystem/tests/Directory/SymbolicLinks.cs

+            DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();
+
+            // Windows differentiates between dir symlinks and file symlinks
+            int expected = PlatformDetection.IsWindows ? 1 : 0;


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;

adamsitnik · 2021-05-14T08:28:55Z

src/libraries/System.IO.FileSystem/tests/Directory/SymbolicLinks.cs

+        public void EnumerateFileSystemEntries_LinksWithCycles_ShouldNotThrow()
+        {
+            DirectoryInfo testDirectory = CreateDirectorySymbolicLinkToItself();
+            Assert.Equal(1, Directory.EnumerateFileSystemEntries(testDirectory.FullName).Count());


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));

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

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.

adamsitnik · 2021-05-14T08:37:42Z

src/libraries/System.IO.FileSystem/tests/Enumeration/SymbolicLinksTests.cs

+                 // Skipping attributes would force a disk hit which enters the cyclic symlink
+                 new EnumerationOptions(){ AttributesToSkip = 0 });
+
+            Assert.Equal(1, enumerable.Count());


Suggested change

Assert.Equal(1, enumerable.Count());

Assert.Single(enumerable);

src/libraries/System.IO.FileSystem/tests/Directory/SymbolicLinks.cs

jozkee

Otherwise LGTM, thanks.

jozkee · 2021-05-14T18:25:20Z

src/libraries/System.IO.FileSystem/tests/Base/BaseSymbolicLinks.cs

+{
+    public abstract class BaseSymbolicLinks : FileSystemTest
+    {
+        protected DirectoryInfo CreateDirectorySymbolicLinkToItself()


?

Suggested change

protected DirectoryInfo CreateDirectorySymbolicLinkToItself()

protected DirectoryInfo CreateDirectoryContainingSymbolicLinkToItself()

Suggested change

protected DirectoryInfo CreateDirectorySymbolicLinkToItself()

protected DirectoryInfo CreateDirectoryContainingSelfReferencingSymbolicLink()

src/libraries/System.IO.FileSystem/tests/Directory/SymbolicLinks.cs

src/libraries/System.IO.FileSystem/tests/DirectoryInfo/SymbolicLinks.cs

jozkee · 2021-05-14T18:58:05Z

src/libraries/System.IO.FileSystem/tests/DirectoryInfo/SymbolicLinks.cs

+                testDirectory.EnumerateDirectories("*", options).Count();
+                testDirectory.GetDirectories("*", options).Count();


Assert how many entries this gets.

jozkee · 2021-05-14T19:08:25Z

@carlossanlop CI issues seem related to the tests you are adding:

[09:34:28] fail: [FAIL] System.IO.Tests.Directory_SymbolicLinks.EnumerateDirectories_LinksWithCycles_ShouldNotThrow
[09:34:28] info: System.PlatformNotSupportedException : System.Diagnostics.Process is not supported on this platform.

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-52749-merge-495ca526023c41e8ab/System.IO.FileSystem.Tests/console.694d2f52.log?sv=2019-07-07&se=2021-06-03T09%3A01%3A35Z&sr=c&sp=rl&sig=%2Bnmb38cHdd1tLVkSo7T8zOvpQGTNIEiMOCkfzWVvZg8%3D

iSazonov · 2021-05-18T04:25:10Z

Could you please update the PR title to "Avoid throwing if a self symbolic link cycle is detected while enumerating" since the fix is only for symbolic link to itself.

carlossanlop · 2021-05-20T01:33:00Z

@jozkee thanks for pointing out the CI failure. We cannot create symbolic links in WASM. Any test that attempts to create symbolic links, must run behind the condition CanCreateSymbolicLinks.

@iSazonov I thought about your suggestion to rename the PR, but instead I thought of adding tests that also verify links to the parent directory. There is only one case that I didn't add: recursively enumerate folders inside a symbolic link that points to its parent folder, because that causes an infinite loop. I don't think this is a bug. We can discuss limiting the recursion levels like @jozkee did in System.Text.Json.

iSazonov · 2021-05-20T03:25:07Z

We can discuss limiting the recursion levels like @jozkee did in System.Text.Json.

This has been mentioned more than once (is there a tracking issue? #52666?) and obviously we will do this - which is why I ask that this PR be renamed to avoid confusion between itself and transition cycles.

…lFact/ConditionalTheory

stephentoub · 2021-05-25T15:16:41Z

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEntry.Unix.cs

@@ -45,15 +45,17 @@ namespace System.IO.Enumeration
            if (isUnknown)
            {
                isSymlink = entry.IsSymbolicLink;
-                isDirectory = entry._status.IsDirectory(entry.FullPath);
+                // Need to fail silently in case we are enumerating


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

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.

carlossanlop and others added 3 commits May 13, 2021 23:09

Fix: Avoid throwing on FileSystemEntry.Initialize

f460cae

Add unit tests that verify cyclic symbolic links

5528774

Fix Windows-specific unit test failures

65ddfc3

carlossanlop added the area-System.IO label May 14, 2021

carlossanlop added this to the 6.0.0 milestone May 14, 2021

carlossanlop requested review from adamsitnik and jozkee May 14, 2021 08:05

carlossanlop self-assigned this May 14, 2021

adamsitnik approved these changes May 14, 2021

View reviewed changes

kasperk81 reviewed May 14, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/Directory/SymbolicLinks.cs Outdated Show resolved Hide resolved

jozkee approved these changes May 14, 2021

View reviewed changes

carlossanlop force-pushed the SymlinkCycle branch from b732bea to 65ddfc3 Compare May 20, 2021 03:14

carlossanlop changed the title ~~Fix: Avoid throwing if a symbolic link cycle is detected while enumerating~~ Fix: Avoid throwing if a symbolic link cycle to itself is detected while enumerating May 20, 2021

carlossanlop added 6 commits May 21, 2021 13:36

Use OperatingSystem.IsWindows()

740cb18

CHange 'Assert.Equal(1, ' to 'Assert.Single('

a3ba144

Rename helper method that creates self referencing symlink

c0100c6

Spacing

d371bd8

ConditionalClass for CanCreateSymbolicLinks

9f0e595

ConditionalClass does not work in browser/ios/android, use Conditiona…

6ef2a60

…lFact/ConditionalTheory

carlossanlop mentioned this pull request May 21, 2021

Allow watch of symbolic links to folders on Unix #52679

Merged

carlossanlop merged commit e329c2d into dotnet:main May 24, 2021

carlossanlop deleted the SymlinkCycle branch May 24, 2021 17:48

carlossanlop mentioned this pull request May 24, 2021

ConditionalClassAttribute throws in cases where ConditionalFact/ConditionalTheory do not throw dotnet/arcade#7439

Open

stephentoub reviewed May 25, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Jun 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

carlossanlop commented May 14, 2021

ghost commented May 14, 2021

adamsitnik left a comment

adamsitnik May 14, 2021

adamsitnik May 14, 2021

carlossanlop May 19, 2021

carlossanlop May 19, 2021

adamsitnik May 14, 2021

jozkee left a comment

jozkee May 14, 2021

jozkee May 19, 2021

jozkee May 14, 2021

jozkee commented May 14, 2021

iSazonov commented May 18, 2021

carlossanlop commented May 20, 2021

iSazonov commented May 20, 2021 •

edited

Loading

stephentoub May 25, 2021

carlossanlop May 25, 2021

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

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

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

	protected DirectoryInfo CreateDirectorySymbolicLinkToItself()
	protected DirectoryInfo CreateDirectoryContainingSymbolicLinkToItself()

		testDirectory.EnumerateDirectories("*", options).Count();
		testDirectory.GetDirectories("*", options).Count();

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

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

Conversation

carlossanlop commented May 14, 2021

ghost commented May 14, 2021

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee commented May 14, 2021

iSazonov commented May 18, 2021

carlossanlop commented May 20, 2021

iSazonov commented May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented May 20, 2021 •

edited

Loading