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

[FileStream] handle UNC and device paths #54483

Merged
merged 2 commits into from
Jun 23, 2021
Merged

[FileStream] handle UNC and device paths #54483

merged 2 commits into from
Jun 23, 2021

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jun 21, 2021 •
edited
Loading

Fixes #54474
Fixes #54475
Fixes #54337

@adamsitnik adamsitnik added this to the 6.0.0 milestone Jun 21, 2021
@ghost
Copy link

ghost commented Jun 21, 2021

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

Issue Details

Fixes #54474

Author: adamsitnik
Assignees: -
Labels:

area-System.IO
Milestone: 6.0.0

@adamsitnik adamsitnik linked an issue Jun 21, 2021 that may be closed by this pull request
A System.NullReferenceException was throwing when get a **gif** image using Image.FromFile() method #54475
Closed
@adamsitnik adamsitnik mentioned this pull request Jun 21, 2021
Closed
jozkee
jozkee reviewed Jun 22, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/IO/PathInternal.Windows.cs Outdated

// this method works only for `fullPath` returned by Path.GetFullPath
// currently we don't have interest in supporting relative paths
internal static void DosToNtPath(ReadOnlySpan<char> fullPath, ref ValueStringBuilder vsb)
Copy link
Member

Choose a reason for hiding this comment

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

I assume for the opposite, translate NT to DOS, a helper like this should be needed, right?
For context #54253 (comment) , a Symlinks API uses DeviceIoControl which returns an NT path that I need to translate to DOS.

This is my silly attempt on doing it:
https://github.com/dotnet/runtime/blob/908530a70613af1172138e48d041d6c5d710d866/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs#L513-L518
Also, I think I mixed the names (DOS and NT) in the comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jozkee You could use RtlNtPathNameToDosPathName, but the problem is that it's internal and not documented, so we should not be using that.

The mapping that you have pointed to seems to be missing a few translations:

  • \??\UNC\ to \\ (files located on a remote machine)
  • \??\ to \\.\ for devices like names pipes: \\.\pipe\$pipeName

But the question is: are they valid in this context? Can someone create a link to a named pipe or a file located on a network share?

FWIW the best doc about the paths I've found so far: https://googleprojectzero.blogspot.com/2016/02/the-definitive-guide-on-win32-to-nt.html

Copy link

Choose a reason for hiding this comment

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

Symbolic links to files on network shares are possible; fsutil behavior set symlinkevaluation can enable or disable them. I don't know about symbolic links to named pipes.

Copy link
Member

Choose a reason for hiding this comment

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

The mapping that you have pointed to seems to be missing a few translations:

The first example you pointed should still work, as far as I know: Converting \??\UNC\ to \\?\UNC, should be equivalent. The difference is that you do not want to pass a path prefixed with \??\ to the user, and should always translate it to \\?\.

Regarding the second example: Have you seen cases where Windows gives you a path prefixed with \??\ but you were expecting it to start with \\.\? And how would you know this?

src/libraries/System.Private.CoreLib/src/System/IO/PathInternal.Windows.cs Outdated Show resolved Hide resolved
src/libraries/Common/tests/Tests/System/IO/PathInternal.Windows.Tests.cs Outdated Show resolved Hide resolved
@adamsitnik adamsitnik mentioned this pull request Jun 22, 2021
Merged
stephentoub
stephentoub approved these changes Jun 22, 2021
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

carlossanlop
carlossanlop approved these changes Jun 23, 2021
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

LGTM

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
{
uint ntStatus;
IntPtr fileHandle;
Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = default;
Copy link
Member

Choose a reason for hiding this comment

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

default isn't really needed, right?

@danmoseley danmoseley mentioned this pull request Jun 23, 2021
Closed
jozkee
jozkee reviewed Jun 23, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
// probably be consistent w/ every other directory.
int errorCode = Marshal.GetLastPInvokeError();

if (errorCode == Interop.Errors.ERROR_PATH_NOT_FOUND && fullPath!.Length == PathInternal.GetRootLength(fullPath))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (errorCode == Interop.Errors.ERROR_PATH_NOT_FOUND && fullPath!.Length == PathInternal.GetRootLength(fullPath))
if (errorCode == Interop.Errors.ERROR_PATH_NOT_FOUND && fullPath.Length == PathInternal.GetRootLength(fullPath))

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs Show resolved Hide resolved
@adamsitnik adamsitnik merged commit 0aafceb into dotnet:main Jun 23, 2021
@adamsitnik
Copy link
Member Author

/backport to release/6.0-preview6

@github-actions
Copy link
Contributor

Started backporting to release/6.0-preview6: https://github.com/dotnet/runtime/actions/runs/963422709

@github-actions github-actions bot mentioned this pull request Jun 23, 2021
Merged
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 23, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_exception_i…
ae2a17c 
…nitial_config

* origin/main: (362 commits)
  [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300)
  Put Crossgen2 in sync with dotnet#54235 (dotnet#54438)
  Move System.Object serialization to ObjectConverter (dotnet#54436)
  Move setting fHasVirtualStaticMethods out of sanity check section (dotnet#54574)
  [wasm] Compile .bc->.o in parallel, before passing to the linker (dotnet#54053)
  Change PathInternal.IsCaseSensitive to a constant (dotnet#54340)
  Make mono_polling_required a public symbol (dotnet#54592)
  Respect EventSource::IsSupported setting in more codepaths (dotnet#51977)
  Root ComActivator for hosting (dotnet#54524)
  Add ILLink annotations to S.D.Common related to DbConnectionStringBuilder (dotnet#54280)
  Fix finalizer issue with regions (dotnet#54550)
  Add support for multi-arch install locations (dotnet#53763)
  Update library testing docs page to reduce confusion (dotnet#54324)
  [FileStream] handle UNC and device paths (dotnet#54483)
  Update NetAnalyzers version (dotnet#54511)
  Added runtime dependency to fix the intermittent test failures (dotnet#54587)
  Disable failing System.Reflection.Tests.ModuleTests.GetMethods (dotnet#54564)
  [wasm] Move AOT builds from `runtime-staging` to `runtime` (dotnet#54577)
  Keep obj node for ArrayIndex. (dotnet#54584)
  Disable another failing MemoryCache test (dotnet#54578)
  ...
@adamsitnik adamsitnik mentioned this pull request Jun 25, 2021
Closed
@adamsitnik adamsitnik mentioned this pull request Jun 25, 2021
Merged
@adamsitnik adamsitnik deleted the issue54474 branch July 2, 2021 11:09
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KalleOlaviNiemitalo KalleOlaviNiemitalo KalleOlaviNiemitalo left review comments

@jozkee jozkee jozkee left review comments

@stephentoub stephentoub stephentoub approved these changes

@carlossanlop carlossanlop carlossanlop approved these changes

Assignees
No one assigned
Labels
area-System.IO
Projects
None yet
Milestone
6.0.0
5 participants
@adamsitnik @carlossanlop @stephentoub @jozkee @KalleOlaviNiemitalo