Enforce scatter/gather file I/O Windows API requirements et. al.

[teo-tsirpanis]

The Windows APIs that perform scatter/gather file I/O have more usage restrictions than their Unix counterparts. Besides requiring the file handle to be opened with buffering disabled, they also require each buffer segment to be aligned at page size boundaries, and will read/write one page from/to each segment.

Until now, the gather/scatter RandomAccess API Windows implementation only enforces the first requirement, allowing buffers containing segments without the proper alignment or length to be passed to the APIs, leading to potential undefined behavior like buffer overflows or underflows. For example, code around

runtime/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs

Line 468 in 0ebc7ec

MemoryHandle memoryHandle = buffer.Pin();

which was pinning the segments did not check the Memory<byte>.Length property at all, which is what raised my suspicions and led us here.

This PR refactors it to additionally check whether each buffer segment is aligned and is exactly as long as a page. The previous check that the total buffer size fits in 32 bits is consolidated with the other two, in a common function for both scatter and gather, that also got the responsibility to pin the segments while checking their alignment (to prevent the GC from potentially moving them around in between). If these requirements are found to be violated, the buffers are unpinned and the operation will be performed with multiple read/write syscalls (like before when the file handle was opened synchronously or with buffering enabled).

Furthermore the RandomAccess APIs on Windows now special-case buffers with only one or zero (new in this PR) segments earlier in the call chain, and there were some other smaller changes.

[ghost]

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

Issue Details

---

The Windows APIs that perform scatter/gather file I/O have more usage restrictions than their Unix counterparts. Besides requiring the file handle to be opened with buffering disabled, they also require each buffer segment to be aligned at page size boundaries, and will read/write one page from/to each segment.

Until now, the gather/scatter RandomAccess API Windows implementation only enforces the first requirement, allowing buffers containing segments without the proper alignment or length to be passed to the APIs, leading to potential undefined behavior like buffer overflows or underflows. For example, code around

runtime/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs

Line 468 in 0ebc7ec

MemoryHandle memoryHandle = buffer.Pin();

which was pinning the segments did not check the Memory<byte>.Length property at all, which is what raised my suspicions and led us here.

This PR refactors it to additionally check whether each buffer segment is aligned and is exactly as long as a page. The previous check that the total buffer size fits in 32 bits is consolidated with the other two, in a common function for both scatter and gather, that also got the responsibility to pin the segments while checking their alignment (to prevent the GC from potentially moving them around in between). If these requirements are found to be violated, the buffers are unpinned and the operation will be performed with multiple read/write syscalls (like before when the file handle was opened synchronously or with buffering enabled).

Furthermore the RandomAccess APIs on Windows now special-case buffers with only one or zero (new in this PR) segments earlier in the call chain, and there were some other smaller changes.

Author:	teo-tsirpanis
Assignees:	-
Labels:	area-System.IO, community-contribution
Milestone:	-

[adamsitnik]

Please provide a single unit test, where you create a buffer that does not meet ReadFileScatter|WriteFileGather size and alignment requirements, but meets ReadFile|WriteFile size and alignment requirements for file handles opened with NO_BUFFERING.

[teo-tsirpanis]

I added the test @adamsitnik.

I also noticed in lines like https://github.com/dotnet/runtime/blob/bae5e4ec0ebbc93542779800a2c5a64e0c8dc620/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs#L147 that we always open the handle with FileOptions.Asynchronous, even when we test synchronous I/O. Is that intentional or should I fix it as well?

[adamsitnik]

this change needs to be benchmarked as I would expect managed allocator to be faster than native for small arrays.

[teo-tsirpanis]

It was @jkotas' idea. Allocating a managed array has less overhead, but it also has to be pinned for the duration of the async operation.

[adamsitnik]

@teo-tsirpanis could you please benchmark your changes? The easiest way would be to contribute some new RandomAccess type benchmarks to dotnet/performance repo similar to FileStream benchmarks and run them using two CoreRun.exe: with and without your changes https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md#dotnet-runtime-prerequisites

dotnet run -c Release -f net6.0 --filter *RandomAccess* --corerun $pathToCoreRunWithoutYourChanges $pathToCoreRunWithYourChanges

[teo-tsirpanis]

I tried but my machine ran out of space. 😕

[adamsitnik]

I tried but my machine ran out of space

@teo-tsirpanis could you please share the benchmark source code? I am would be very happy to help

[adamsitnik]

@teo-tsirpanis I've implemented some benchmarks (dotnet/performance#1967) and run them against your fork.

This is what is going to be required to run them once dotnet/performance#1967 is merged (I am sharing that so you can run some benchmarks in the future):

git clone https://github.com/teo-tsirpanis/dotnet-runtime.git runtime
cd .\runtime\
.\build.cmd -c Release -subset clr+libs
robocopy .\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\7.0.0\ .\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\before /E
git checkout windows-vectored-io-refactor
taskkill /IM "dotnet.exe" /F
.\build.cmd -c Release -subset clr.corelib+clr.nativecorelib+libs.PreTest
robocopy .\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\7.0.0\ .\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\after /E
cd ..
git clone https://github.com/dotnet/performance.git performance
py .\performance\scripts\benchmarks_ci.py -f net6.0 --filter *Perf_RandomAccess_NoBuffering* --corerun .\runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\before\corerun.exe .\runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\after\corerun.exe

[adamsitnik]

that we always open the handle with FileOptions.Asynchronous, even when we test synchronous I/O. Is that intentional or should I fix it as well?

In this particular file where we test NO_BUFFERING support it's expected, as it's ReadFileScatter requirement:

The file handle must be created with the GENERIC_READ right, and the FILE_FLAG_OVERLAPPED and FILE_FLAG_NO_BUFFERING flags. For more information, see File Security and Access Rights.

FILE_FLAG_OVERLAPPED == FileOptions.Asynchronous (this is the tricky part)

But we should most probably extend the tests and cover all possible scenarios:

• sync operations on async handle
• async operations on async handle
• sync operations on sync handle
• async operations on sync handle

The test has been added

[adamsitnik]

But we should most probably extend the tests and cover all possible scenarios:

@teo-tsirpanis I've sent #57717 to address that

[adamsitnik]

The code LGTM, the perf also. @teo-tsirpanis once again thank you!

[adamsitnik]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1185379030