Address Windows recommendations for Link APIs #58592
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-io Issue Details
|
…display and can be misleading
|
@adamsitnik @carlossanlop PTAL, I've updated the PR in order to fix a potential security threat. |
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs
Outdated
Show resolved
Hide resolved
| } | ||
| else if (returnFullPath) | ||
| { | ||
| return Path.Join(Path.GetDirectoryName(linkPath.AsSpan()), targetPath).ToString(); |
There was a problem hiding this comment.
The ToString() is unnecessary. All the Path.Join overloads return a string.
There was a problem hiding this comment.
Maybe we should have an analyzer to catch calling ToString() on a string..
There was a problem hiding this comment.
Maybe we should have an analyzer to catch calling ToString() on a string..
It has a side effect: it performs a null check. So it can't always be removed. Regardless, I see this in generated code sometimes, where the generator just blindly calls ToString on data regardless of the data's underlying type, and it's pretty harmless in those scenarios.
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs
Outdated
Show resolved
Hide resolved
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1220223811 |
|
@jozkee backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: NT prefix shall not be treated as a relative path
Using index info to reconstruct a base tree...
M src/libraries/Common/src/System/IO/PathInternal.Windows.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Common/src/System/IO/PathInternal.Windows.cs
CONFLICT (content): Merge conflict in src/libraries/Common/src/System/IO/PathInternal.Windows.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 NT prefix shall not be treated as a relative path
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
* NT prefix shall not be treated as a relative path * Remove file-folder-match validation * Use SubstituteName instead of PrintName since the latter is just for display and can be misleading * Fix MS.IO.Redist * Fix whitespace in Strings.resx * Address suggestions
* NT prefix shall not be treated as a relative path * Remove file-folder-match validation * Use SubstituteName instead of PrintName since the latter is just for display and can be misleading * Fix MS.IO.Redist * Fix whitespace in Strings.resx * Address suggestions
ghost
locked as resolved and limited conversation to collaborators
1ce2f30: Even thoughREPARSE_DATA_BUFFER'sPrintNameis more end-user-friendly vsSubstituteName, there's no guarantee that NT prefix (\??\) won't be there, we got recommended that we should take our cautions with that and apply logic to trim the prefix as needed.However, in further investigation I've noticed that a symlink/junction created with NT prefix, that is
mklink foo \??\C:\Users\david\barcannot be resolved by Windows. therefore my approach here is to always return the unadulteredPrintNamein such case. If Windows is unable to resolve such paths, we should defer users to reason about them themselves.I've also found a bug in
DeviceIoControlwhereREPARSE_DATA_BUFFER.Flagswill containSYMLINK_FLAG_RELATIVEfor NT paths (e.g:\??\C:\foo), which is incorrect given that such paths will always be treated as absolute.2a5cda1: Another recommendation from Windows is that we should not run any validation in the link's target when it is being created since all validations should occur at resolution time, therefore, I've removed the "file-must-target-file" validation that we initially wrote.
fbcfe3e UPDATE: After further digging, we should not use PrintName as it can display a different target to the one intended, see: https://www.tiraniddo.dev/2016/02/tracking-down-root-cause-of-windows.html
cc @GrabYourPitchforks.