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

[release/6.0] Address Windows recommendations for Link APIs (#58592) #58926

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Sep 10, 2021

Backport of #58592 to release/6.0

Customer Impact

Windows experts gave us advice related to our new Link APIs added to System.IO. This PR addresses the following:

  • 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.
  • For Windows only: We should not use PrintName as there is no guarantee that the SubstituteName and PrintName in the REPARSE_DATA_BUFFER refer to the same file, I've changed the implementation to use SubstituteName.

Testing

Updated/extended unit tests based on above changes.

Risk

Low, we are performing the following:

  • Remove one validation on CreateSymbolicLink
  • Parse an NT path (\??\C:\...) to a "normal" Win32 path (C:\...), however, there's prior art (in PowerShell) to this kind of parsing.

Note

The bot failed to backport this automatically given a previous PR (#57988) that unified the duplicated PathInternal*.cs files.
This PR just adds two constants to one of those files.

cc @carlossanlop

* 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
@jozkee jozkee added the Servicing-consider Issue for next servicing release review label Sep 10, 2021
@jozkee jozkee added this to the 6.0.0 milestone Sep 10, 2021
@jozkee jozkee self-assigned this Sep 10, 2021
@ghost
Copy link

ghost commented Sep 10, 2021

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

Issue Details

Backport of #58592 to release/6.0

Customer Impact

Windows experts gave us advice related to our new Link APIs added to System.IO. This PR addresses the following:

  • 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.
  • For Windows only: We should not use PrintName as there is no guarantee that the SubstituteName and PrintName in the REPARSE_DATA_BUFFER refer to the same file, I've changed the implementation to use SubstituteName.

Testing

Updated/extended unit tests based on above changes.

Risk

Low, we are performing the following:

  • Remove one validation on CreateSymbolicLink
  • Parse an NT path (\??\C:\...) to a "normal" Win32 path (C:\...), however, there's prior art (in PowerShell) to this kind of parsing.

Note

The bot failed to backport this automatically given a previous PR (#57988) that unified the duplicated PathInternal*.cs files.
This PR just adds two constants to one of those files.

cc @carlossanlop

Author: Jozkee
Assignees: Jozkee
Labels:

Servicing-consider, area-System.IO

Milestone: 6.0.0

@danmoseley
Copy link
Member

Approved. Avoids potential failures with the API we're adding. Based on guidance from OS and prior art in Powershell. Limited and well understood changes.

Good template!

@Anipik Anipik merged commit 188bfd8 into dotnet:release/6.0 Sep 13, 2021
@jozkee jozkee deleted the link_recomendations_6.0 branch September 13, 2021 18:24
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO Servicing-consider Issue for next servicing release review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants