Skip to content

File.OpenHandle#53344

Closed
adamsitnik wants to merge 23 commits intodotnet:mainfrom
adamsitnik:safeFileHandle
Closed

File.OpenHandle#53344
adamsitnik wants to merge 23 commits intodotnet:mainfrom
adamsitnik:safeFileHandle

Conversation

@adamsitnik
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik commented May 27, 2021 •
edited
Loading

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

@adamsitnik adamsitnik added this to the 6.0.0 milestone May 27, 2021
@ghost
Copy link
Copy Markdown

ghost commented May 27, 2021

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

@ghost
Copy link
Copy Markdown

ghost commented May 27, 2021

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.

stephentoub
stephentoub reviewed May 27, 2021
View reviewed changes
stephentoub
stephentoub reviewed May 27, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs Outdated
{
IntPtr fileHandle = NtCreateFile(fullPath, mode, access, share, options, preallocationSize);

return ValidateFileHandle(new SafeFileHandle(fileHandle, ownsHandle: true), fullPath, (options & FileOptions.Asynchronous) != 0);
Copy link
Copy Markdown
Member

@stephentoub stephentoub May 27, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

@adamsitnik adamsitnik May 31, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@stephentoub stephentoub May 31, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@adamsitnik adamsitnik May 31, 2021

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Member

@stephentoub stephentoub May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

stephentoub
stephentoub reviewed May 27, 2021
View reviewed changes
@runfoapp runfoapp bot mentioned this pull request May 27, 2021
Open
@adamsitnik adamsitnik marked this pull request as draft May 28, 2021 09:07
@adamsitnik adamsitnik added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 28, 2021
@adamsitnik
Copy link
Copy Markdown
Member Author

adamsitnik commented May 28, 2021

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

adamsitnik
adamsitnik commented May 31, 2021
View reviewed changes
src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs
if ((options & FileOptions.DeleteOnClose) != 0)
{
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
Copy link
Copy Markdown
Member Author

@adamsitnik adamsitnik May 31, 2021

Choose a reason for hiding this comment

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

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

adamsitnik
adamsitnik commented May 31, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs
throw new ArgumentOutOfRangeException(nameof(access), SR.ArgumentOutOfRange_Enum);
}
else if (bufferSize <= 0)
else if (bufferSize < 0)
Copy link
Copy Markdown
Member Author

@adamsitnik adamsitnik May 31, 2021

Choose a reason for hiding this comment

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

this is something that I've missed in #52928

adamsitnik and others added 4 commits May 31, 2021 11:01
@adamsitnik
add missing ;
6c58252
@adamsitnik
simplify Windows implementation:
fb24f5c 
* 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
@adamsitnik
unify CanSeek and IsPipe for the .NET 5 Windows strategy as well
8baf0bd
@adamsitnik
move the !MS_IO_REDIST methods from File.cs to File.netcoreapp.cs…
50aba1f 
… file
@adamsitnik adamsitnik removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 31, 2021
@adamsitnik adamsitnik marked this pull request as ready for review May 31, 2021 11:10
@adamsitnik adamsitnik requested a review from stephentoub May 31, 2021 17:07
@adamsitnik
Copy link
Copy Markdown
Member Author

adamsitnik commented Jun 3, 2021

Closing in favor of #53669

@adamsitnik adamsitnik closed this Jun 3, 2021
@adamsitnik adamsitnik deleted the safeFileHandle branch July 2, 2021 11:10
@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

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

No one assigned

Labels

area-System.IO new-api-needs-documentation

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@adamsitnik @stephentoub