Fix longpath directory removal #15467
Conversation
| var directoryInfo = new System.IO.DirectoryInfo(providerPath); | ||
| if (directoryInfo.Exists | ||
| && InternalSymbolicLinkLinkCodeMethods.IsReparsePointLikeSymlink(directoryInfo)) |
There was a problem hiding this comment.
Here di is never null and we can remove the null check but we also have no need to do extra Exist check because providerPath is based on resolvedPath.
Are there really side effects from the tests? |
|
I think it is impossible to have reparse points for UNC paths (I mean source path, UNC path is possible as target for symbolic links). If it is true we can do: private const string ExtendedDevicePathPrefix = @"\\?\";
private const string UncPathPrefix = @"\\";
private const string UncDevicePrefixToInsert = @"?\UNC\";
private const string UncExtendedPathPrefix = @"\\?\UNC\";
private const string DevicePathPrefix = @"\\.\";
// \\?\, \\.\, \??\
private const int DevicePrefixLength = 4;
private static string EnsureExtendedPrefix(string path)
{
if (IsPartiallyQualified(path) || IsDevice(path))
return path;
// Given \\server\share in longpath becomes \\?\UNC\server\share
if (path.StartsWith(UncPathPrefix, StringComparison.OrdinalIgnoreCase))
return path.Insert(2, UncDevicePrefixToInsert);
return ExtendedDevicePathPrefix + path;
}
private static bool IsDevice(string path)
{
return IsExtended(path)
||
(
path.Length >= DevicePrefixLength
&
8000
;& IsDirectorySeparator(path[0])
&& IsDirectorySeparator(path[1])
&& (path[2] == '.' || path[2] == '?')
&& IsDirectorySeparator(path[3])
);
}
private static bool IsExtended(string path)
{
return path.Length >= DevicePrefixLength
&& path[0] == '\\'
&& (path[1] == '\\' || path[1] == '?')
&& path[2] == '?'
&& path[3] == '\\';
}
internal static bool IsReparsePointLikeSymlink(FileSystemInfo fileInfo)
{
#if UNIX
// Reparse point on Unix is a symlink.
return IsReparsePoint(fileInfo);
#else
if (InternalTestHooks.OneDriveTestOn && fileInfo.Name == InternalTestHooks.OneDriveTestSymlinkName)
{
return !InternalTestHooks.OneDriveTestRecurseOn;
}
WIN32_FIND_DATA data = default;
string fullPath = Path.TrimEndingDirectorySeparator(fileInfo.FullName);
if (fullPath.Length > MAX_PATH)
{
fullPath = EnsureExtendedPrefix(fullPath);
}
using (var handle = FindFirstFileEx(fullPath, FINDEX_INFO_LEVELS.FindExInfoBasic, ref data, FINDEX_SEARCH_OPS.FindExSearchNameMatch, IntPtr.Zero, 0))
...If there is a reparse point at the end of this long path - the scenario was never covered by tests. |
|
|
||
| if (fullPath.Length > MAX_PATH) | ||
| { | ||
| fullPath = "\\\\?\\" + fullPath; |
There was a problem hiding this comment.
There are 2 things we need to check here before we can add the prefix.
- The prefix isn't already present - I'm not sure if
FileSystemInfo.FullNamepreserves the\\?\if it was first created with that prefix - When dealing with a UNC path we need to do
\\?\UNC\{server}\{share}, essentially replace the first\\with\\?\UNC
I'm also not sure if pwsh/.NET is meant to support the \\.\ DosDevice prefix as well as \\?\, if so then it also needs to check for that prefix.
Finally I think that the prefix should always be added rather than when the path exceeds MAX_PATH. This ensures the code always runs the same no matter what path is being used and any future bugs that might occur occur in all situations (thus gets picked up more easily/in betas) rather than just in the edge cases.
It's still possible to have a reparse point over a UNC path. How the client interprets a symlink does differ but that doesn't really matter in this case. The code is just checking to see whether it needs to go into the target and delete the contents or just the reparse point. But by adding the |
Reparse point is a feature local NTFS - we can have UNC as target but not as source. In the context we check the source. |
|
It might be a feature of local NTFS but the same attributes are exposed over SMB. So you can enumerate a network path over SMB that contains a OneDrive link which appear as a reparse point but still needs those further checks that the function does. My comment merely tries to confirm that the reparse check can still occur beforehand to reduce the extra Win32 call if the item isn’t a reparse point. |
|
For history. I found PR where the test was added. ce4c35b#diff-08b9d853f19b7b63dfb6b0fa7b634db55f2a20aeed01ffb5855b3e5b0da3431b There we also added EnsureLongPathPrefixIfNeeded() with simple normalization. It is 6.0 Beta.3 time. I played with mount points over network and this works as @jborean93 pointed (thanks!). So I think we should use already tested .Net code I updated my proposal above. Now we have an issue in .Net Runtime repository where we are discussing new API for symbolic links and probably reparse points too. Perhaps we get this in .Net 6.0. I hope the new API allows us to remove all p/invokes in PowerShell (in the area).
|
|
Closing in favour of #15546. |
PR Summary
Fixes #15466.
Adds longpath support back for
Remove-Itemand tries to move the tests for OneDrive paths so that they don't cause side effects for existing tests.PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).