File.OpenHandle #53344

adamsitnik · 2021-05-27T12:15:45Z

I've moved all the logic responsible for opening and initializing SafeFileHandle to SafeFileHandle. A lot of duplicated code (mostly for initializing ThreadPoolBinding) got removed. On Unix, it's also now much more clear how many syscalls we perform to just open a file.

Other changes:

Instead of using NtCreateFile for cases when preallocationSize was specified and CreateFileW when not, I've followed @JeremyKuhne suggestion and switched to using NtCreateFile only. This has simplified the code and provided some minor perf gain. I've discovered a bug in the previous implementation (related to DesiredAccess.DELETE) fixed it, and added a test
On Unix, I've realized that once we call fstat() to ensure that a given file is not a directory we can also determine whether given file is seekable or not. This has removed one syscall for the initialization of CanSeek.

Contributes to #24847

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

ghost · 2021-05-27T12:15:51Z

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #24847

Author:	adamsitnik
Assignees:	-
Labels:	`area-System.IO`
Milestone:	6.0.0

dotnet-issue-labeler · 2021-05-27T12:15:51Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/System.IO.FileSystem/tests/File/OpenHandle.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

stephentoub · 2021-05-27T14:24:37Z

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

+                {
+                    IntPtr fileHandle = NtCreateFile(fullPath, mode, access, share, options, preallocationSize);
+
+                    return ValidateFileHandle(new SafeFileHandle(fileHandle, ownsHandle: true), fullPath, (options & FileOptions.Asynchronous) != 0);


There's a bunch of other options being set below, e.g. disabling Inheritable, setting SECURITY_SQOS_PRESENT and SECURITY_ANONYMOUS, etc... why is that not relevant if a preallocation size is set?

It's set, but in a different place:

runtime/src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs

Lines 81 to 87 in d5cef92

// For mitigating local elevation of privilege attack through named pipes

// make sure we always call NtCreateFile with SECURITY_ANONYMOUS so that the

// named pipe server can't impersonate a high privileged client security context

SECURITY_QUALITY_OF_SERVICE securityQualityOfService = new SECURITY_QUALITY_OF_SERVICE(

ImpersonationLevel.Anonymous, // SECURITY_ANONYMOUS

ContextTrackingMode.Static,

effectiveOnly: false);

I decided to follow @JeremyKuhne suggestion and stop using CreateFileW and NtCreateFile and use NtCreateFile only. There is a minor perf gain (2% on opening the file) but the code became much cleaner

How are we handling https://docs.microsoft.com/en-us/windows/win32/devnotes/calling-internal-apis ?

@stephentoub for some reason NtCreateFile() has two docs:

https://docs.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntcreatefile (says that it's internal)

https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntcreatefile (no mention of internal)

I am not sure which doc is right, but we have been using NtCreateFile for more than 3 years now: dotnet/corefx#27195

Does NtCreateFile support every kind of file the higher level CreateFileW does? My understanding is NtCreateFile is a bit faster because CreateFile does additional work to special-case various things.

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

…omFile.ReadAtOffset are going to need it)

adamsitnik · 2021-05-28T16:39:34Z

@stephentoub apologies for the confusion, it's not ready for review yet. I've converted it to a draft.

…s it's a Windows-specific concept

…Seek for regular files

Co-authored-by: Stephen Toub <stoub@microsoft.com>

adamsitnik · 2021-05-31T08:56:12Z

src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs

            {
-                result |= DesiredAccess.DELETE; // required by FILE_DELETE_ON_CLOSE and FILE_SUPERSEDE (which deletes a file if it exists)
+                result |= DesiredAccess.DELETE; // required by FILE_DELETE_ON_CLOSE


this was a bug, I've added a test CreateAndOverwrite that ensures that everything works as expected

adamsitnik · 2021-05-31T08:58:45Z

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

@@ -78,18 +74,28 @@ private static void ValidateHandle(SafeFileHandle handle, FileAccess access, int
            {
                throw new ArgumentOutOfRangeException(nameof(access), SR.ArgumentOutOfRange_Enum);
            }
-            else if (bufferSize <= 0)
+            else if (bufferSize < 0)


this is something that I've missed in #52928

* with NtCreateFile there is no need for Validation (NtCreateFile returns error and we never create a safe file handle) * unify status codes * complete error mapping

… file

adamsitnik · 2021-06-03T10:38:37Z

Closing in favor of #53669

adamsitnik added 3 commits May 27, 2021 13:00

move logic related to opening and initializing handle from FileStream…

d37d1ee

…Helpers to SafeFileHandle itself

move the arguments validation logic common for FileStream ctor and Fi…

539393d

…le.OpenHandle to FileStreamHelpers

introduce File.OpenHandle

b60e876

adamsitnik added the area-System.IO label May 27, 2021

adamsitnik added this to the 6.0.0 milestone May 27, 2021

dotnet-issue-labeler bot added the new-api-needs-documentation label May 27, 2021