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

Add internal junction support to link APIs #57996

Merged
merged 19 commits into from
Aug 27, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
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.

{
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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,28 @@ private static bool GetStaticNonPublicBooleanPropertyValue(string typeName, stri
public static bool IsIcuGlobalization => ICUVersion > new Version(0,0,0,0);
public static bool IsNlsGlobalization => IsNotInvariantGlobalization && !IsIcuGlobalization;

public static bool IsSubstAvailable
{
get
{
try
{
if (IsWindows)
{
string systemRoot = Environment.GetEnvironmentVariable("SystemRoot");
if (string.IsNullOrWhiteSpace(systemRoot))
{
return false;
}
string system32 = Path.Combine(systemRoot, "System32");
return File.Exists(Path.Combine(system32, "subst.exe"));
}
}
catch { }
return false;
}
}

private static Version GetICUVersion()
{
int version = 0;
Expand Down
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
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;
}
}
}
71 changes: 71 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/Junctions.Windows.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// 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? targetFromDirectoryInfo = junctionInfo.ResolveLinkTarget(returnFinalTarget);
FileSystemInfo? targetFromDirectory = Directory.ResolveLinkTarget(junctionPath, returnFinalTarget);

Assert.True(targetFromDirectoryInfo is DirectoryInfo);
Assert.True(targetFromDirectory is DirectoryInfo);

Assert.Equal(targetPath, junctionInfo.LinkTarget);

carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(targetPath, targetFromDirectoryInfo.FullName);
Assert.Equal(targetPath, targetFromDirectory.FullName);
}

[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);
DirectoryInfo firstJunctionInfo = CreateJunction(firstJunctionPath, middleJunctionPath);

string expectedTargetPath = returnFinalTarget ? targetPath : middleJunctionPath;

FileSystemInfo? targetFromDirectoryInfo = firstJunctionInfo.ResolveLinkTarget(returnFinalTarget);
FileSystemInfo? targetFromDirectory = Directory.ResolveLinkTarget(firstJunctionPath, returnFinalTarget);

Assert.True(targetFromDirectoryInfo is DirectoryInfo);
Assert.True(targetFromDirectory is DirectoryInfo);

// Always the immediate target
Assert.Equal(middleJunctionPath, firstJunctionInfo.LinkTarget);

Assert.Equal(expectedTargetPath, targetFromDirectoryInfo.FullName);
Assert.Equal(expectedTargetPath, targetFromDirectory.FullName);
}
}
}
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 @@ -28,9 +28,7 @@ public static class MountHelper
[DllImport("kernel32.dll", EntryPoint = "DeleteVolumeMountPointW", CharSet = CharSet.Unicode, BestFitMapping = false, SetLastError = true)]
private static extern bool DeleteVolumeMountPoint(string mountPoint);

/// <summary>Creates a symbolic link using command line tools</summary>
/// <param name="linkPath">The existing file</param>
/// <param name="targetPath"></param>
/// <summary>Creates a symbolic link using command line tools.</summary>
public static bool CreateSymbolicLink(string linkPath, string targetPath, bool isDirectory)
{
Process symLinkProcess = new Process();
Expand All @@ -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();
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

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 symLinkProcess.ExitCode == 0;
}

/// <summary>On Windows, creates a junction using command line tools.</summary>
public static bool CreateJunction(string junctionPath, string targetPath)
{
if (!OperatingSystem.IsWindows())
{
symLinkProcess.WaitForExit();
return (0 == symLinkProcess.ExitCode);
throw new PlatformNotSupportedException();
}
else

return RunProcess(CreateProcessStartInfo("cmd", "/c", "mklink", "/J", junctionPath, targetPath));
}

///<summary>
/// On Windows, mounts a folder to an assigned virtual drive letter using the subst command.
/// subst is not available in Windows Nano.
/// </summary>
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

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

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

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 = GetNextAvailableDriveLetter();
bool success = RunProcess(CreateProcessStartInfo("cmd", "/c", SubstPath, $"{driveLetter}:", targetDir));
if (!success || !DriveInfo.GetDrives().Any(x => x.Name[0] == driveLetter))
{
throw new InvalidOperationException($"Could not create virtual drive {driveLetter}: with subst");
}
return driveLetter;

// Finds the next unused drive letter and returns it.
char GetNextAvailableDriveLetter()
{
return false;
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 Mount(string volumeName, string mountPoint)
/// <summary>
/// On Windows, unassigns the specified virtual drive letter from its mounted folder.
/// </summary>
public static void DeleteVirtualDrive(char driveLetter)
{
if (!OperatingSystem.IsWindows())
{
throw new PlatformNotSupportedException();
}

bool success = RunProcess(CreateProcessStartInfo("cmd", "/c", SubstPath, "/d", $"{driveLetter}:"));
if (!success || DriveInfo.GetDrives().Any(x => x.Name[0] == driveLetter))
{
throw new InvalidOperationException($"Could not delete virtual drive {driveLetter}: with subst");
}
}

public static void Mount(string volumeName, string mountPoint)
{
if (volumeName[volumeName.Length - 1] != Path.DirectorySeparatorChar)
volumeName += Path.DirectorySeparatorChar;
if (mountPoint[mountPoint.Length - 1] != Path.DirectorySeparatorChar)
Expand Down Expand Up @@ -93,8 +149,47 @@ public static void Unmount(string mountPoint)
throw new Exception(string.Format("Win32 error: {0}", Marshal.GetLastPInvokeError()));
}

private static ProcessStartInfo CreateProcessStartInfo(string fileName, params string[] arguments)
{
var info = new ProcessStartInfo
{
FileName = fileName,
UseShellExecute = false,
RedirectStandardOutput = true
};

foreach (var argument in arguments)
{
info.ArgumentList.Add(argument);
}

return info;
}

private static bool RunProcess(ProcessStartInfo startInfo)
{
var process = Process.Start(startInfo);
process.WaitForExit();
return process.ExitCode == 0;
}

private static string SubstPath
{
get
{
if (!OperatingSystem.IsWindows())
{
throw new PlatformNotSupportedException();
}

string systemRoot = Environment.GetEnvironmentVariable("SystemRoot") ?? @"C:\Windows";
string system32 = Path.Join(systemRoot, "System32");
return Path.Join(system32, "subst.exe");
}
}

/// For standalone debugging help. Change Main0 to Main
public static void Main0(string[] args)
public static void Main0(string[] args)
{
try
{
Expand Down
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