-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Allow to specify file allocation size #51111
Conversation
Note regarding the 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. |
Tagging subscribers to this area: @carlossanlop Issue Detailsfixes #45946 this PR is not ready for review yet. Once I prove that it's useful from perf perspective on Windows, I am going to implement Unix support and then mark the PR as ready for review
|
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.UNICODE_STRING.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.OBJECT_ATTRIBUTES.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.IO_STATUS_BLOCK.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.NtCreateFile.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamConformanceTests.cs
Since a previous review comment thread showed some confusion as to why the It all stems from the fact that NtCreateFile needs native NT paths, and expects that any Win32 paths have converted to native ones already. If you just want to know what the correct code to use in the general case is, you can skip over the background information, but reading it provides much needed background information. Do note, that I'm not sure this most general case is actually worth implementing here. It would add support for UNC paths (I've no idea if UNC paths support specifying allocation size), and would let you one use the Background informationPaths that begin with Beyond the normal absolute, relative, drive relative path types, and UNC path types that everybody knows, Win32 has a few more path types. One is Both of those prefixes are resolved against the native NT path that was originally named Before calling NtCreateFile you are "supposed" to convert Win32 paths to native NT paths with the undocumented RtlDosPathNameToNtPathName_U function (or the RtlDosPathNameToRelativeNtPathName_U function, or their status code returning versions.) The intent appears to have been that only Windows itself would ever need to do this. It was intended that if anybody else was was calling NtCreateFile they would be constructing native paths directly, most likely for devices, but potentially for other kernel object too. So what do those functions do? Well in terms of actual path conversion, for Win32Nt paths (the ones that start Basically The General CaseIn the general case, if you want maximum compatibility when directly calling NtCreateFile, you would ideally would want to do the equivalent of: Of course, if something higher up has already ensured More InfoBesides reading the source code of ntdll.dll (or asking you favorite NT Kernel developer), the best resource for all of this is this post from Google's Project Zero that goes into even more detail than anybody had ever previously written down. As an aside: I'd have loved if that post delved a bit more into some of how the kernel processes the path afterwards (for example the per session overlays on top of Hope this helps, or at the very least was interesting. |
In this particular case runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs Lines 40 to 48 in 1197851
This is ensured by @KevinCathcart big thanks for providing all the information! It helped me to understand why this mapping is needed. |
@JeremyKuhne do you have any idea about how we could map the following security flags to runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs Lines 96 to 102 in c2f995e
currently, it's the only thing that blocks me from switching to Other mappings that we already have in place can be found here: https://github.com/adamsitnik/runtime/blob/c2f995efd5a26b2501e71df173894da7d0a707ae/src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs#L85-L176 |
The third argument to Passing the Hope this helps. |
As we don't allow anything else, there is no In a similar vein we don't allow Without the
Notes:
I put a fair amount of comments on mappings in the various parameters in my experimental library. For example: https://github.com/JeremyKuhne/WInterop/blob/main/src/WInterop.Desktop/Storage/CreateDisposition.cs If you haven't looked through my various types around CreateFile and NtCreateFile in the library it would probably be worth looking into it further. |
src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer_fo_as.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer_fo_as.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>
# Conflicts: # src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs
…PreallocationSize is negative
@jozkee yes, my plan is to add them in a separate PR, after we get an approval to change the old proposal from adding a new ctor argument (long allocationSize) to use |
@jozkee @carlossanlop I've switched to use |
// 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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlossanlop @jozkee this is to mimic the following logic:
runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs
Lines 83 to 87 in 88211b9
// For mitigating local elevation of privilege attack through named pipes | |
// make sure we always call CreateFile with SECURITY_ANONYMOUS so that the | |
// named pipe server can't impersonate a high privileged client security context | |
// (note that this is the effective default on CreateFile2) | |
flagsAndAttributes |= (Interop.Kernel32.SecurityOptions.SECURITY_SQOS_PRESENT | Interop.Kernel32.SecurityOptions.SECURITY_ANONYMOUS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM, thanks.
src/libraries/Common/src/Interop/Windows/Interop.SECURITY_QUALITY_OF_SERVICE.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.IO.Redist/src/Microsoft.IO.Redist.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs
Outdated
Show resolved
Hide resolved
public FileStream(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share = System.IO.FileShare.Read, int bufferSize = 4096, System.IO.FileOptions options = System.IO.FileOptions.None, long allocationSize = 0) { } | ||
public FileStream(string path, System.IO.FileStreamOptions options) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the FileStream API approved in #45946 (comment) will not be needed after all, make sure to communicate that in the thread.
Also consider not closing #45946 when this PR gets merged since there's still work to do related to the rest of other types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jozkee my plan is to just add FileStreamOptions
overloads everywhere: #24698 (comment)
…nd which ones throw
Value provided as
allocationSize
argument has no effect unless it's positive and a regular file is being created, overwritten, or replaced. It means that:< 0
FileStream
ctor throwsArgumentOutOfRangeException
0
it's just ignoredMy main motivation was to make it easy to use it, without performing a lot of OS-specific checks on the caller side that would have to ensure that given path points to a regular file (it can be used for not just creating new files, but also replacing or truncating existing ones).
IOException
is thrown when:< 1TB
disk on Windows 10, Ubuntu and macOS)Implementation details:
NtCreateFile
which provides an atomic way to create|replace|truncate a file if there is enough free space. This works quite well, with the only exception of a misleadingNT_STATUS_INVALID_PARAMETER
returned for files that are too big for given file system. I've found a workaround for that, please see the code.posix_fallocate
I've used this method. The difference with Windows is that it's an additional syscall (done after the file gets locked). It also changes the reportedLength
of given file (On WindowsAllocationSize != EndOfFile
so reported file length is not changed). The weird thing aboutposix_fallocate
is that it's spec does not mention what should have happen when there is not enough space. In case of Ubuntu, the entire available disk space is allocated... This is why I've marked theWhenDiskIsFullTheErrorMessageContainsAllDetails
test as[Outerloop]
and disabled the parallelism for the type that defines it.fctnl
withF_PREALLOCATE
command. In contrary toposix_fallocate
it's not modifying the reported file length, so that's why I've decided to also callftruncate
and "align the Unixes" (https://stackoverflow.com/a/29693383). I hate macOS because it's docs don't mention thatfctnl
can fail withENOSPC
(not enough space), but in reality, it can fail withENOSPC
(verified with my mac).posix_fallocate
but defineF_ALLOCSP
I've used it as a command forfctnl
. Side note: I got inspired by some popular Torrent clients ;)fixes #45946
fixes #52446