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

Jump to bottom

Add internal junction support to link APIs #57996

Merged
merged 19 commits into from
Aug 27, 2021
Merged

Add internal junction support to link APIs #57996

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8e7069a
Add mount point support to link APIs.
carlossanlop Aug 24, 2021
11960b4
Add junction and virtual drive tests.
carlossanlop Aug 24, 2021
e59f2b2
Move PrintName comment outside of if else of reparseTag check.
carlossanlop Aug 24, 2021
2923c9e
Add Windows platform specific attribute to junction and virtual drive…
carlossanlop Aug 24, 2021
b6cf857
Revert FILE_NAME_OPENED to FILE_NAME_NORMALIZED
carlossanlop Aug 24, 2021
3ef33c2
Revert addition of FILE_NAME_OPENED const.
carlossanlop Aug 24, 2021
bc12ecf
Remove unnecessary enumeration junction test.
carlossanlop Aug 25, 2021
8957765
Rename GetNewCwdPath to ChangeCurrentDirectory
carlossanlop Aug 25, 2021
ce2aa86
Make Junction_ResolveLinkTarget a theory and test both resolveFinalTa…
carlossanlop Aug 25, 2021
999cddc
Shorter name for targetPath string. Typo in comment. Fix Debug.Assert.
carlossanlop Aug 25, 2021
42fb3a0
Clarify test comment. Change PlatformDetection for OperatingSystem ch…
carlossanlop Aug 25, 2021
ca1f2b2
Cleaner unit tests for virtual drive, add indirection test
carlossanlop Aug 26, 2021
6f6d368
Skip virtual drive tests in Windows Nano (subst not available). Small…
carlossanlop Aug 26, 2021
ec8000b
Simplify Junctions tests, add indirection test
carlossanlop Aug 26, 2021
7df549e
Address test suggestions.
carlossanlop Aug 26, 2021
be9e582
Revert MountHelper.CreateSymbolicLink changes. Unrelated, and will be…
carlossanlop Aug 27, 2021
d33bda3
Add dwReserved0 check for mount points in GetFinalLinkTarget.
carlossanlop Aug 27, 2021
244246b
Use Yoda we don't.
carlossanlop Aug 27, 2021
bd7e9cc
Fix CI issues
jozkee Aug 27, 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
29 changes: 17 additions & 12 deletions src/libraries/Common/src/Interop/Windows/Kernel32/Interop.REPARSE_DATA_BUFFER.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,29 @@ internal static partial class Kernel32
internal const uint SYMLINK_FLAG_RELATIVE = 1;

// https://msdn.microsoft.com/library/windows/hardware/ff552012.aspx
// We don't need all the struct fields; omitting the rest.
[StructLayout(LayoutKind.Sequential)]
internal unsafe struct REPARSE_DATA_BUFFER
internal unsafe struct SymbolicLinkReparseBuffer
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
internal uint ReparseTag;
internal ushort ReparseDataLength;
internal ushort Reserved;
internal SymbolicLinkReparseBuffer ReparseBufferSymbolicLink;
internal ushort SubstituteNameOffset;
internal ushort SubstituteNameLength;
internal ushort PrintNameOffset;
internal ushort PrintNameLength;
internal uint Flags;
}

[StructLayout(LayoutKind.Sequential)]
internal struct SymbolicLinkReparseBuffer
{
internal ushort SubstituteNameOffset;
internal ushort SubstituteNameLength;
internal ushort PrintNameOffset;
internal ushort PrintNameLength;
internal uint Flags;
}
[StructLayout(LayoutKind.Sequential)]
internal struct MountPointReparseBuffer
{
public uint ReparseTag;
public ushort ReparseDataLength;
public ushort Reserved;
public ushort SubstituteNameOffset;
public ushort SubstituteNameLength;
public ushort PrintNameOffset;
public ushort PrintNameLength;
}
}
}
5 changes: 2 additions & 3 deletions src/libraries/System.IO.FileSystem/tests/Base/SymbolicLinks/BaseSymbolicLinks.FileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,11 +460,10 @@ private void ResolveLinkTarget_ReturnFinalTarget(string link1Path, string link1T
Assert.Equal(filePath, finalTarget.FullName);
}

// Must call inside a remote executor
protected void CreateSymbolicLink_PathToTarget_RelativeToLinkPath_Internal(bool createOpposite)
{
string tempCwd = GetRandomDirPath();
Directory.CreateDirectory(tempCwd);
Directory.SetCurrentDirectory(tempCwd);
string tempCwd = ChangeCurrentDirectory();

// Create a dummy file or directory in cwd.
string fileOrDirectoryInCwd = GetRandomFileName();
Expand Down
17 changes: 13 additions & 4 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
@@ -1,10 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Buffers;
using System.Diagnostics;
using System.Runtime.InteropServices;
using Microsoft.Win32.SafeHandles;
using Xunit;

namespace System.IO.Tests
Expand Down Expand Up @@ -36,5 +32,18 @@ protected DirectoryInfo CreateSelfReferencingSymbolicLink()
protected string GetRandomDirPath() => Path.Join(ActualTestDirectory.Value, GetRandomDirName());

private Lazy<string> ActualTestDirectory => new Lazy<string>(() => GetTestDirectoryActualCasing());

/// <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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

/// </summary>
/// <returns>The path of the new cwd.</returns>
protected string ChangeCurrentDirectory()
{
string tempCwd = GetRandomDirPath();
Directory.CreateDirectory(tempCwd);
Directory.SetCurrentDirectory(tempCwd);
return tempCwd;
}
}
}
62 changes: 62 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// 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
{
[PlatformSpecific(TestPlatforms.Windows)]
public class Junctions : BaseSymbolicLinks
{
protected DirectoryInfo CreateJunction(string junctionPath, string targetPath)
{
Assert.True(MountHelper.CreateJunction(junctionPath, targetPath));
DirectoryInfo junctionInfo = new(junctionPath);
return junctionInfo;
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void Junction_ResolveLinkTarget(bool returnFinalTarget)
{
string junctionPath = GetRandomLinkPath();
string targetPath = GetRandomDirPath();

Directory.CreateDirectory(targetPath);
DirectoryInfo junctionInfo = CreateJunction(junctionPath, targetPath);

FileSystemInfo? actualTargetInfo = junctionInfo.ResolveLinkTarget(returnFinalTarget);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
Assert.True(actualTargetInfo is DirectoryInfo);
Assert.Equal(targetPath, actualTargetInfo.FullName);
Assert.Equal(targetPath, junctionInfo.LinkTarget);
}

carlossanlop marked this conversation as resolved.
Show resolved Hide resolved

[Theory]
[InlineData(false)]
[InlineData(true)]
public void Junction_ResolveLinkTarget_WithIndirection(bool returnFinalTarget)
{
string firstJunctionPath = GetRandomLinkPath();
string middleJunctionPath = GetRandomLinkPath();
string targetPath = GetRandomDirPath();

Directory.CreateDirectory(targetPath);
_ = CreateJunction(middleJunctionPath, targetPath);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
DirectoryInfo firstJunctionInfo = CreateJunction(firstJunctionPath, middleJunctionPath);

string expectedTargetPath = returnFinalTarget ? targetPath : middleJunctionPath;

FileSystemInfo? actualTargetInfo = firstJunctionInfo.ResolveLinkTarget(returnFinalTarget);

Assert.True(actualTargetInfo is DirectoryInfo);
Assert.Equal(expectedTargetPath, actualTargetInfo.FullName);

// Always the immediate target
Assert.Equal(middleJunctionPath, firstJunctionInfo.LinkTarget);
}
}
}
96 changes: 83 additions & 13 deletions src/libraries/System.IO.FileSystem/tests/PortedCommon/ReparsePointUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
#define DEBUG

using System;
using System.IO;
using System.Text;
using System.Diagnostics;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices;
using System.ComponentModel;
using System.Threading;
using System.Text;
using System.Threading.Tasks;

public static class MountHelper
{
[DllImport("kernel32.dll", EntryPoint = "GetVolumeNameForVolumeMountPointW", CharSet = CharSet.Unicode, BestFitMapping = false, SetLastError = true)]
Expand All @@ -37,25 +37,95 @@ public static bool CreateSymbolicLink(string linkPath, string targetPath, bool i
if (OperatingSystem.IsWindows())
{
symLinkProcess.StartInfo.FileName = "cmd";
symLinkProcess.StartInfo.Arguments = string.Format("/c mklink{0} \"{1}\" \"{2}\"", isDirectory ? " /D" : "", linkPath, targetPath);
string option = isDirectory ? "/D " : "";
symLinkProcess.StartInfo.Arguments = $"/c mklink {option}\"{linkPath}\" \"{targetPath}\"";
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
symLinkProcess.StartInfo.FileName = "/bin/ln";
symLinkProcess.StartInfo.Arguments = string.Format("-s \"{0}\" \"{1}\"", targetPath, linkPath);
symLinkProcess.StartInfo.Arguments = $"-s \"{targetPath}\" \"{linkPath}\"";
}
symLinkProcess.StartInfo.UseShellExecute = false;
symLinkProcess.StartInfo.RedirectStandardOutput = true;
symLinkProcess.Start();
symLinkProcess.WaitForExit();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@carlossanlop carlossanlop Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return (0 == symLinkProcess.ExitCode);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
}

if (symLinkProcess != null)
public static bool CreateJunction(string junctionPath, string targetPath)
{
if (!OperatingSystem.IsWindows())
{
symLinkProcess.WaitForExit();
return (0 == symLinkProcess.ExitCode);
throw new PlatformNotSupportedException();
}
else

Process junctionProcess = new Process();
junctionProcess.StartInfo.FileName = "cmd";
junctionProcess.StartInfo.Arguments = $"/c mklink /J \"{junctionPath}\" \"{targetPath}\"";
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
junctionProcess.StartInfo.UseShellExecute = false;
junctionProcess.StartInfo.RedirectStandardOutput = true;
junctionProcess.Start();
junctionProcess.WaitForExit();
return (0 == junctionProcess.ExitCode);
}

public static char CreateVirtualDrive(string targetDir)
Copy link
Member

Choose a reason for hiding this comment

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

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

@ViktorHofer could it be an issue for the CI?

Copy link
Member

@ViktorHofer ViktorHofer Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Member

@MattGal MattGal Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@MattGal thank you for a very detailed answer!

Copy link
Member Author

@carlossanlop carlossanlop Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
if (!OperatingSystem.IsWindows())
{
throw new PlatformNotSupportedException();
}

char driveLetter = GetRandomDriveLetter();

Process substProcess = new Process();
substProcess.StartInfo.FileName = "subst";
Copy link
Member

Choose a reason for hiding this comment

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

All other functions use cmd, but this one use subst directly. What's the reason why we don't do it this way everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

What if this gets called twice (in parallel) and this executes when the virtual drive was already created?

Copy link
Member Author

@carlossanlop carlossanlop Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK, invoking cmd so that it can invoke an executable is unnecessary. If you're asking because mklink above is doing that, then maybe we could remove the dependency on cmd and invoke it directly. Edit: mklink depends on cmd, it's a command, not an executable. subst is an executable that resides in C:\Windows\System32\subst.exe.

If the subst call ever gets called twice, it would fail with the message Drive already SUBSTed. This is why I decided to only create one drive per test class, add all the virtual drive tests to the same class, and delete the virtual drive on disposal of said class.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was careful to call subst only once for the whole test class, and ensure the reserved virtual drive letter gets unassigned on the Dispose method of said test class.

I'd like to think that the Drive already SUBSTed error message will be accompanied by an exitCode != 0, so the run process catches it. I'll make sure the exitCode != 0 is also caught and causes the exception to be thrown.

substProcess.StartInfo.Arguments = $"{driveLetter}: {targetDir}";
substProcess.StartInfo.UseShellExecute = false;
substProcess.StartInfo.RedirectStandardOutput = true;
substProcess.Start();
substProcess.WaitForExit();
if (!DriveInfo.GetDrives().Any(x => x.Name[0] == driveLetter))
{
throw new InvalidOperationException($"Could not create virtual drive {driveLetter}: with subst");
}
return driveLetter;

char GetRandomDriveLetter()
{
List<char> existingDrives = DriveInfo.GetDrives().Select(x => x.Name[0]).ToList();

// A,B are reserved, C is usually reserved
IEnumerable<int> range = Enumerable.Range('D', 'Z' - 'D');
IEnumerable<char> castRange = range.Select(x => Convert.ToChar(x));
IEnumerable<char> allDrivesLetters = castRange.Except(existingDrives);

if (!allDrivesLetters.Any())
{
throw new ArgumentOutOfRangeException("No drive letters available");
}

return allDrivesLetters.First();
}
}

public static void DeleteVirtualDrive(char driveLetter)
{
if (!OperatingSystem.IsWindows())
{
throw new PlatformNotSupportedException();
}

Process substProcess = new Process();
substProcess.StartInfo.FileName = "subst";
substProcess.StartInfo.Arguments = $"/d {driveLetter}:";
substProcess.StartInfo.UseShellExecute = false;
substProcess.StartInfo.RedirectStandardOutput = true;
substProcess.Start();
substProcess.WaitForExit();
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
if (DriveInfo.GetDrives().Any(x => x.Name[0] == driveLetter))
{
return false;
throw new InvalidOperationException($"Could not delete virtual drive {driveLetter}: with subst");
}
}

Expand Down Expand Up @@ -94,7 +164,7 @@ public static void Unmount(string mountPoint)
}

/// For standalone debugging help. Change Main0 to Main
public static void Main0(string[] args)
public static void Main0(string[] args)
{
try
{
Expand Down
7 changes: 4 additions & 3 deletions src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
Expand Down Expand Up @@ -80,9 +80,11 @@
<Compile Include="FileSystemTest.Windows.cs" />
<Compile Include="FileStream\ctor_options_as.Windows.cs" />
<Compile Include="FileStream\FileStreamConformanceTests.Windows.cs" />
<Compile Include="Junctions.Windows.cs" />
<Compile Include="RandomAccess\Mixed.Windows.cs" />
<Compile Include="RandomAccess\NoBuffering.Windows.cs" />
<Compile Include="RandomAccess\SectorAlignedMemory.Windows.cs" />
<Compile Include="VirtualDriveSymbolicLinks.Windows.cs" />
<Compile Include="$(CommonPath)Interop\Windows\Interop.BOOL.cs" Link="Common\Interop\Windows\Interop.BOOL.cs" />
<Compile Include="$(CommonPath)Interop\Windows\Interop.Libraries.cs" Link="Common\Interop\Windows\Interop.Libraries.cs" />
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.CreateFile.cs" Link="Common\Interop\Windows\Interop.CreateFile.cs" />
Expand Down Expand Up @@ -211,8 +213,7 @@
<Compile Include="$(CommonTestPath)System\IO\TempFile.cs" Link="Common\System\IO\TempFile.cs" />
<Compile Include="$(CommonTestPath)System\IO\PathFeatures.cs" Link="Common\System\IO\PathFeatures.cs" />
<Content Include="DirectoryInfo\test-dir\dummy.txt" Link="test-dir\dummy.txt" />
<Compile Include="$(CommonPath)System\IO\PathInternal.CaseSensitivity.cs"
Link="Common\System\IO\PathInternal.CaseSensitivity.cs" />
<Compile Include="$(CommonPath)System\IO\PathInternal.CaseSensitivity.cs" Link="Common\System\IO\PathInternal.CaseSensitivity.cs" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(CommonTestPath)StreamConformanceTests\StreamConformanceTests.csproj" />
Expand Down
Loading