Add internal junction support to link APIs #57996

carlossanlop · 2021-08-24T04:19:27Z

Fixes Preview 7 inconsistent FileSystemInfo LinkTarget and ResolveLinkTarget results when using virtual drives #57582

In Windows, if a user creates a virtual drive using the subst command, they are actually creating a junction that points to a drive.

By adding junction support to our internal GetImmediateLinkTarget method, if the user creates symbolic links that point to files or directories inside that virtual drive, we can now ensure that the public API ResolveLinkTarget(returnFinalTarget: false) returns a FileSystemInfo in which the FullName points to a path that uses said virtual drive.

Note: When the user calls ResolveLinkTarget(returnFinalTarget: true), we use a different P/Invoke, which returns the absolute normalized path of the symlink's final target. This is expected behavior from Windows. So this code path remains untouched.

Fixes Preview 7 Improper LinkTarget returned for a Directory Junction on Windows #57575

The root cause was the same as above: Both LinkTarget and ResolveLinkTarget(returnFinalTarget: false) call the internal method GetImmediateLinkTarget, which returns null if a reparse point that is not a symlink is encountered. And because ResolveLinkTarget(returnFinalTarget: true) calls a different P/Invoke, it is able to resolve any kind of reparse point, which explains the discrepancy. By adding support for mount points in GetImmediateLinkTarget, we can now return the target properly.

cc @fitdev

ghost · 2021-08-24T04:19:38Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes Preview 7 inconsistent FileSystemInfo LinkTarget and ResolveLinkTarget results when using virtual drives #57582

In Windows, if a user creates a virtual drive using the subst command, they are actually creating a junction that points to a drive.

By adding junction support to our internal GetImmediateLinkTarget method, if the user creates symbolic links that point to files or directories inside that virtual drive, we can now ensure that the public API ResolveLinkTarget(returnFinalTarget: false) returns a FileSystemInfo in which the FullName points to a path that uses said virtual drive.

Note: When the user calls ResolveLinkTarget(returnFinalTarget: true), we use a different P/Invoke, which returns the absolute normalized path of the symlink's final target. This is expected behavior from Windows. So this code path remains untouched.

Fixes Preview 7 Improper LinkTarget returned for a Directory Junction on Windows #57575 (Pending to fix)

We decided that when a FileSystemInfo wraps a junction, calling LinkTarget should return null for 6.0. This is so the user can determine that the file is not a symlink.

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

… test classes.

carlossanlop · 2021-08-24T21:43:53Z

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.REPARSE_DATA_BUFFER.cs

        [StructLayout(LayoutKind.Sequential)]
-        internal unsafe struct REPARSE_DATA_BUFFER
+        internal unsafe struct SymbolicLinkReparseBuffer


I renamed this to match the names that were provided in the DUMMYUNIONNAME section of the official Windows docs.

Also, the structs are not nested. This is how PowerShell had it already: https://github.com/PowerShell/PowerShell/blob/56d22bc3865fca445145fd685d6d6f6d63194701/src/System.Management.Automation/namespaces/FileSystemProvider.cs#L7957-L7985

src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseJunctions.FileSystem.cs

carlossanlop · 2021-08-24T21:47:31Z

src/libraries/System.IO.FileSystem/tests/VirtualDriveSymbolicLinks.Windows.cs

+    // Need to reuse the same virtual drive for all the test methods.
+    // Creating and disposing one virtual drive per class achieves this.


If instead of doing this, I splitted the virtual drive tests into multiple different test classes, then each test class would create its own virtual drive. We have so many symlink tests now, that we would end up depleting the available drive letters.

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

carlossanlop · 2021-08-24T22:05:51Z

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

-                // Only symbolic links are supported at the moment.
-                if ((rdb.ReparseTag & Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_SYMLINK) == 0)
-                {
-                    return null;
-                }


ReparseTag is not a flag, so the bitwise operation was unnecessary. Examples from PowerShell: 1, 2.

The if logic is now reversed, so that we only return null if the link is not a symlink or a mount point.

jozkee

Source changes look mostly good to me; the tests are a bit hard to comprehend and I think we are missing some scenarios.

src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseJunctions.FileSystem.cs

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

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

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

src/libraries/System.IO.FileSystem/tests/VirtualDriveSymbolicLinks.Windows.cs

…rget

…eck.

carlossanlop · 2021-08-26T11:58:57Z

Seems Windows Nano does not have subst available. The CI is failing in that leg.

I'll have to add the IsNotWindowsNanoServer condition attribute to the virtual drive tests.

… test rename.

adamsitnik

Overall LGTM, but please address the nits that I've found before we merge it. Thank you @carlossanlop !

adamsitnik · 2021-08-26T14:47:42Z

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

+
+        /// <summary>
+        /// Changes the current working directory path to a new temporary directory.
+        /// Important: Make sure to call this inside a remote executor to avoid changing the cwd for all tests in same process.


is there a way to ensure that it's called from Remote Executor?

I'm not sure if we can add such a check here.

it would have to be something like this
Debug.Assert(Environment.StackTrace.Contains("RemoteExecutor"));
or it would be reasonable to add a static flag in RemoteExecutor so you could just test RemoteExecutor.IsInRemoteExecutor.

src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs

src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs

adamsitnik · 2021-08-26T15:06:04Z

src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs

+        return (0 == junctionProcess.ExitCode);
+    }
+
+    public static char CreateVirtualDrive(string targetDir)


I am totally OK with a test creating a temporary drive on my machine.

@ViktorHofer could it be an issue for the CI?

@dotnet/dnceng @MattGal is creating a temporary virtual drive as part of a test which runs on PRs and rolling builds something that we should avoid?

I am totally OK with a test creating a temporary drive on my machine.

We should maybe make sure this is a general consensus with real users who might run your test on their personal machines; the odds of leaking are there.

As for whether it's something to avoid, that depends. TL;DR: Probably OK, but you might need to consider cleanup automation (there's not even a try around this code, if it blows up it just leaves things where it was...) I'll give a few responses since I don't know exactly where this will run.

If running on an Azure Devops Hosted build machine - no issues, these are unique VMs per build

If running on a helix-based buildpool.* machine: my (admittedly limited) understanding of subst is that the virtual drive mappings don't persist across reboots; we always reboot, no issues as long as this is accurate.

If running on a VM-based Helix test machine: it's probably OK because you're the only user creating random drive mappings, but if there's a failure to cleanup and the same machine runs automation doing this repeatedly, as I don't think you can get more than 26 drive letters.

If running on an on-premises, physical Helix test machine (every Windows ARM machine) : scariest possible choice; anything that's actually persistent might exist a long time, so if there's a letter-leak this is where I'd expect trouble.

All that said, I think if you had some sort of try {} finally {} here that cleans up the mapping when you're done, you're Ok.

@MattGal thank you for a very detailed answer!

I think if you had some sort of try {} finally {} here that cleans up the mapping when you're done, you're Ok.

I added a Dispose method to the test class, which clears the mapped drive. And by the way, the test class only creates one mapped drive and reuses it for all of its test methods. precisely because I was seeing the problem of using all the available drive letters. Now I only use one per test class.

I'm not sure what I would add to the finally part of the try catch I added to the Dispose method. Suggestions? Or is it good as it is?

BTW I have no preference, but note for other "somewhat impactful" tests we made them outerloop. That way we rarely impact our contributors, but still are properly covered. Example: the process.start tests that pop up the browser. It's an option but I have no opinion.

src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs

src/libraries/System.IO.FileSystem/tests/VirtualDriveSymbolicLinks.Windows.cs

jozkee

Otherwise; LGTM. Only suggestions are for tests.

src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs

jozkee · 2021-08-26T18:16:09Z

src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs

+        char driveLetter = GetRandomDriveLetter();
+
+        Process substProcess = new Process();
+        substProcess.StartInfo.FileName = "subst";


If the subst call ever gets called twice, it would fail with the message Drive already SUBSTed.

Is there something that catches that error? my concern is that a race condition could occur and subst gets unintentionally executed twice and the CI would fail. Or maybe is not a big deal and we can wait for the CI to actually fail.

jozkee · 2021-08-26T18:25:45Z

src/libraries/System.IO.FileSystem/tests/VirtualDriveSymbolicLinks.Windows.cs

+    // Need to reuse the same virtual drive for all the test methods.
+    // Creating and disposing one virtual drive per class achieves this.
+    [PlatformSpecific(TestPlatforms.Windows)]
+    [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))]


Do you know why this is not supported in Nano?

Windows Nano Server is a stripped down version of Windows. A bunch of components are removed to make it lightweight. The CI error says the subst executable was not found. It was supposed to be located in C:\Windows\System32, which is part of either PATH or some default env variable.

if subst is not in the PATH for Windows Nano, can't we use the full path to the subst executable?

src/libraries/System.IO.FileSystem/tests/VirtualDriveSymbolicLinks.Windows.cs

jozkee · 2021-08-26T18:37:13Z

src/libraries/System.IO.FileSystem/tests/VirtualDriveSymbolicLinks.Windows.cs

+        // [InlineData(true, true, false, true, false)]   // Target is not in virtual drive
+        [InlineData(true, true, true, false, true)]       // Immediate target expected, target is in virtual drive
+        [InlineData(true, true, true, true, false)]       // Final target expected, target is in virtual drive
+        public void VirtualDrive_SymbolicLinks_WithIndirection(bool isFirstLinkInVirtualDrive, bool isMiddleLinkInVirtualDrive, bool isTargetInVirtualDrive, bool returnFinalTarget, bool isExpectedTargetPathVirtual)


Instead of this method you can add another parameter to the previous test, name it isIndirect or something alike.

I attempted it but the test became way more complicated than it already is. I would rather keep them separate.

src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs

… refactored/removed in the future. Detect if SUBST is available in Windows machine, to bring back Nano.

adamsitnik · 2021-08-27T06:12:40Z

src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs

@@ -48,20 +46,78 @@ public static bool CreateSymbolicLink(string linkPath, string targetPath, bool i
        symLinkProcess.StartInfo.RedirectStandardOutput = true;
        symLinkProcess.Start();

-        if (symLinkProcess != null)
+        symLinkProcess.WaitForExit();


the code above was not modified, but we should rather refactor it as well:

Depeneding on your personal preferences it could be:

public static bool CreateSymbolicLink(string linkPath, string targetPath, bool isDirectory) { ProcessStartInfo startInfo = OperatingSystem.IsWindows() ? CreateStartInfo("cmd", "/c", "mklink", isDirectory ? "/D" : "", linkPath, targetPath) : CreateStartInfo("/bin/ln", "-s", targetPath, linkPath); return RunProcess(startInfo); }

Or

public static bool CreateSymbolicLink(string linkPath, string targetPath, bool isDirectory) => RunProcess(OperatingSystem.IsWindows() ? CreateStartInfo("cmd", "/c", "mklink", isDirectory ? "/D" : "", linkPath, targetPath) : CreateStartInfo("/bin/ln", "-s", targetPath, linkPath));

I also refactored it, initially. I used the code you shared the first time. It caused all the symlink enumeration tests to fail. It was taking me too long to figure out why so I decided to revert it and the tests passed again.
I can take a look again in the related PR for AppExecLinks, if you don't mind.

carlossanlop · 2021-08-27T18:49:00Z

/backport to release/6.0-rc1

github-actions · 2021-08-27T18:49:15Z

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1175308538

carlossanlop added 2 commits August 23, 2021 21:00

Add mount point support to link APIs.

8e7069a

Add junction and virtual drive tests.

11960b4

carlossanlop added the area-System.IO label Aug 24, 2021

carlossanlop added this to the 6.0.0 milestone Aug 24, 2021

carlossanlop requested review from adamsitnik and jozkee August 24, 2021 04:19

carlossanlop self-assigned this Aug 24, 2021

carlossanlop assigned jozkee Aug 24, 2021

runfoapp bot mentioned this pull request Aug 24, 2021

Feed unreliability affecting CI #55449

Closed

carlossanlop added 2 commits August 24, 2021 12:59

Move PrintName comment outside of if else of reparseTag check.

e59f2b2

Add Windows platform specific attribute to junction and virtual drive…

2923c9e

… test classes.

carlossanlop marked this pull request as ready for review August 24, 2021 21:42

carlossanlop commented Aug 24, 2021

View reviewed changes

carlossanlop added 2 commits August 24, 2021 14:55

Revert FILE_NAME_OPENED to FILE_NAME_NORMALIZED

b6cf857

Revert addition of FILE_NAME_OPENED const.

3ef33c2

carlossanlop commented Aug 24, 2021

View reviewed changes

jozkee reviewed Aug 25, 2021

View reviewed changes

carlossanlop added 6 commits August 25, 2021 15:39

Remove unnecessary enumeration junction test.

bc12ecf

Rename GetNewCwdPath to ChangeCurrentDirectory

8957765

Make Junction_ResolveLinkTarget a theory and test both resolveFinalTa…

ce2aa86

…rget

Shorter name for targetPath string. Typo in comment. Fix Debug.Assert.

999cddc

Clarify test comment. Change PlatformDetection for OperatingSystem ch…

42fb3a0

…eck.

Cleaner unit tests for virtual drive, add indirection test

ca1f2b2

carlossanlop added 2 commits August 26, 2021 05:13

Skip virtual drive tests in Windows Nano (subst not available). Small…

6f6d368

… test rename.

Simplify Junctions tests, add indirection test

ec8000b

adamsitnik approved these changes Aug 26, 2021

View reviewed changes

jozkee approved these changes Aug 26, 2021

View reviewed changes

Address test suggestions.

7df549e

jozkee reviewed Aug 26, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs Outdated Show resolved Hide resolved

carlossanlop added 3 commits August 26, 2021 17:23

Revert MountHelper.CreateSymbolicLink changes. Unrelated, and will be…

be9e582

… refactored/removed in the future. Detect if SUBST is available in Windows machine, to bring back Nano.

Add dwReserved0 check for mount points in GetFinalLinkTarget.

d33bda3

Use Yoda we don't.

244246b

carlossanlop mentioned this pull request Aug 27, 2021

Add internal AppExecLink support to link APIs #58233

Closed

karelz mentioned this pull request Aug 27, 2021

System.Net.Security test run killed on Mono Linux #56478

Closed

Fix CI issues

bd7e9cc

adamsitnik reviewed Aug 27, 2021

View reviewed changes

jozkee mentioned this pull request Aug 27, 2021

WIP: Use newly added LinkTarget property PowerShell/PowerShell#15960

Closed

22 tasks

carlossanlop merged commit f7848bc into dotnet:main Aug 27, 2021

carlossanlop deleted the reparsepoint branch August 27, 2021 18:36

github-actions bot mentioned this pull request Aug 27, 2021

[release/6.0-rc1] Add internal junction support to link APIs #58285

Merged

This was referenced Aug 30, 2021

Fix detection of unsupported links #58391

Merged

[release/6.0] Fix detection of unsupported links #58399

Merged

ghost locked as resolved and limited conversation to collaborators Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal junction support to link APIs #57996

Add internal junction support to link APIs #57996

carlossanlop commented Aug 24, 2021 •

edited

Loading

ghost commented Aug 24, 2021

carlossanlop Aug 24, 2021

carlossanlop Aug 24, 2021

carlossanlop Aug 24, 2021

carlossanlop Aug 24, 2021 •

edited

Loading

jozkee left a comment

carlossanlop commented Aug 26, 2021

adamsitnik left a comment

adamsitnik Aug 26, 2021

carlossanlop Aug 26, 2021

danmoseley Aug 28, 2021

adamsitnik Aug 26, 2021

ViktorHofer Aug 26, 2021 •

edited

Loading

MattGal Aug 26, 2021 •

edited

Loading

adamsitnik Aug 26, 2021

carlossanlop Aug 26, 2021 •

edited

Loading

danmoseley Aug 27, 2021

jozkee left a comment

jozkee Aug 26, 2021

jozkee Aug 26, 2021

carlossanlop Aug 26, 2021 •

edited

Loading

jozkee Aug 26, 2021

jozkee Aug 26, 2021

carlossanlop Aug 26, 2021

adamsitnik Aug 27, 2021

carlossanlop Aug 27, 2021 •

edited

Loading

carlossanlop commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

		// Need to reuse the same virtual drive for all the test methods.
		// Creating and disposing one virtual drive per class achieves this.

Add internal junction support to link APIs #57996

Add internal junction support to link APIs #57996

Conversation

carlossanlop commented Aug 24, 2021 • edited Loading

ghost commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

jozkee left a comment

Choose a reason for hiding this comment

carlossanlop commented Aug 26, 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

ViktorHofer Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

MattGal Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Aug 26, 2021 • edited Loading

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

carlossanlop Aug 26, 2021 • edited Loading

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

carlossanlop Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

carlossanlop commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

carlossanlop commented Aug 24, 2021 •

edited

Loading

carlossanlop Aug 24, 2021 •

edited

Loading

ViktorHofer Aug 26, 2021 •

edited

Loading

MattGal Aug 26, 2021 •

edited

Loading

carlossanlop Aug 26, 2021 •

edited

Loading

carlossanlop Aug 26, 2021 •

edited

Loading

carlossanlop Aug 27, 2021 •

edited

Loading