Follow up on Scatter/Gather API changes (#58447)

* Allocate an array of memory handles only if needed.

* Remove an unnecessary variable in the multiple-syscall write gather.

* Actually verify the content read by the read scatter operation.

* Delay allocating native memory.

* Verify that the whole file was read in the scatter/gather test.

* Test the case when the scatter/gather buffers are acceptable by the Windows API.

* Avoid null pointer dereferences when passing an empty segment array.

* Test performing scatter/gather I/O with an empty segment array.

Co-authored-by: Stephen Toub <[email protected]>

src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs

@@ -177,18 +177,17 @@ public async Task WriteAsyncUsingMultipleBuffers(bool async)
             Assert.Equal(content, File.ReadAllBytes(filePath));
         }
 
-        [Fact]
-        public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers()
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task ReadWriteAsyncUsingMultipleBuffers(bool memoryPageSized)
         {
             string filePath = GetTestFilePath();
-            // The Windows scatter/gather APIs accept segments that are exactly one page long.
-            // Combined with the FILE_FLAG_NO_BUFFERING's requirements, the segments must also
-            // be aligned at page size boundaries and have a size of a multiple of the page size.
-            // Using segments with a length of twice the page size adheres to the second requirement
-            // but not the first. The RandomAccess implementation will see it and issue sequential
-            // read/write syscalls per segment, instead of one scatter/gather syscall.
-            // This test verifies that fallback behavior.
-            int bufferSize = Environment.SystemPageSize * 2;
+            // We test with buffers both one and two memory pages long. In the former case,
+            // the I/O operations will issue one scatter/gather API call, and in the latter
+            // case they will issue multiple calls; one per buffer. The buffers must still
+            // be aligned to comply with FILE_FLAG_NO_BUFFERING's requirements.
+            int bufferSize = Environment.SystemPageSize * (memoryPageSized ? 1 : 2);
             int fileSize = bufferSize * 2;
             byte[] content = RandomNumberGenerator.GetBytes(fileSize);
 
@@ -202,10 +201,22 @@ public async Task ReadWriteAsyncUsingNonPageSizedMultipleBuffers()
                 await RandomAccess.WriteAsync(handle, new ReadOnlyMemory<byte>[] { firstHalf, secondHalf }, 0);
 
                 buffer.GetSpan().Clear();
-                await RandomAccess.ReadAsync(handle, new Memory<byte>[] { firstHalf, secondHalf }, 0);
+                long nRead = await RandomAccess.ReadAsync(handle, new Memory<byte>[] { firstHalf, secondHalf }, 0);
+
+                Assert.Equal(buffer.GetSpan().Length, nRead);
+                AssertExtensions.SequenceEqual(buffer.GetSpan(), content.AsSpan());
             }
+        }
+
+        [Fact]
+        public async Task ReadWriteAsyncUsingEmptyBuffers()
+        {
+            string filePath = GetTestFilePath();
+            using SafeFileHandle handle = File.OpenHandle(filePath, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.Asynchronous | NoBuffering);
 
-            Assert.Equal(content, await File.ReadAllBytesAsync(filePath));
+            long nRead = await RandomAccess.ReadAsync(handle, Array.Empty<Memory<byte>>(), 0);
+            Assert.Equal(0, nRead);
+            await RandomAccess.WriteAsync(handle, Array.Empty<ReadOnlyMemory<byte>>(), 0);
         }
     }
 }

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

@@ -4,6 +4,7 @@
 using System.Buffers;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.IO.Strategies;
 using System.Numerics;
 using System.Runtime.InteropServices;
@@ -437,8 +438,8 @@ private static bool CanUseScatterGatherWindowsAPIs(SafeFileHandle handle)
         // The pinned MemoryHandles and the pointer to the segments must be cleaned-up
         // with the CleanupScatterGatherBuffers method.
         private static unsafe bool TryPrepareScatterGatherBuffers<T, THandler>(IReadOnlyList<T> buffers,
-            THandler handler, out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)
-            where THandler: struct, IMemoryHandler<T>
+            THandler handler, [NotNullWhen(true)] out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes)
+            where THandler : struct, IMemoryHandler<T>
         {
             int pageSize = s_cachedPageSize;
             Debug.Assert(BitOperations.IsPow2(pageSize), "Page size is not a power of two.");
@@ -447,13 +448,11 @@ private static unsafe bool TryPrepareScatterGatherBuffers<T, THandler>(IReadOnly
             long alignedAtPageSizeMask = pageSize - 1;
 
             int buffersCount = buffers.Count;
-            handlesToDispose = new MemoryHandle[buffersCount];
+            handlesToDispose = null;
             segmentsPtr = IntPtr.Zero;
             totalBytes = 0;
 
-            // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. "
-            long* segments = (long*) NativeMemory.Alloc((nuint)(buffersCount + 1), sizeof(long));
-            segments[buffersCount] = 0;
+            long* segments = null;
 
             bool success = false;
             try
@@ -469,36 +468,55 @@ private static unsafe bool TryPrepareScatterGatherBuffers<T, THandler>(IReadOnly
                         return false;
                     }
 
-                    MemoryHandle handle = handlesToDispose[i] = handler.Pin(in buffer);
-                    long ptr = segments[i] = (long)handle.Pointer;
+                    MemoryHandle handle = handler.Pin(in buffer);
+                    long ptr = (long)handle.Pointer;
                     if ((ptr & alignedAtPageSizeMask) != 0)
                     {
+                        handle.Dispose();
                         return false;
                     }
+
+                    // We avoid allocations if there are no
+                    // buffers or the first one is unacceptable.
+                    (handlesToDispose ??= new MemoryHandle[buffersCount])[i] = handle;
+                    if (segments == null)
+                    {
+                        // "The array must contain enough elements to store nNumberOfBytesToWrite
+                        // bytes of data, and one element for the terminating NULL."
+                        segments = (long*)NativeMemory.Alloc((nuint)buffersCount + 1, sizeof(long));
+                        segments[buffersCount] = 0;
+                    }
+                    segments[i] = ptr;
                 }
 
                 segmentsPtr = (IntPtr)segments;
                 totalBytes = (int)totalBytes64;
                 success = true;
-                return true;
+                return handlesToDispose != null;
             }
             finally
             {
                 if (!success)
                 {
-                    CleanupScatterGatherBuffers(handlesToDispose, (IntPtr) segments);
+                    CleanupScatterGatherBuffers(handlesToDispose, (IntPtr)segments);
                 }
             }
         }
 
-        private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[] handlesToDispose, IntPtr segmentsPtr)
+        private static unsafe void CleanupScatterGatherBuffers(MemoryHandle[]? handlesToDispose, IntPtr segmentsPtr)
         {
-            foreach (MemoryHandle handle in handlesToDispose)
+            if (handlesToDispose != null)
             {
-                handle.Dispose();
+                foreach (MemoryHandle handle in handlesToDispose)
+                {
+                    handle.Dispose();
+                }
             }
 
-            NativeMemory.Free((void*) segmentsPtr);
+            if (segmentsPtr != IntPtr.Zero)
+            {
+                NativeMemory.Free((void*)segmentsPtr);
+            }
         }
 
         private static ValueTask<long> ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList<Memory<byte>> buffers,
@@ -510,7 +528,7 @@ private static ValueTask<long> ReadScatterAtOffsetAsync(SafeFileHandle handle, I
             }
 
             if (CanUseScatterGatherWindowsAPIs(handle)
-                && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes))
+                && TryPrepareScatterGatherBuffers(buffers, default(MemoryHandler), out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes))
             {
                 return ReadScatterAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken);
             }
@@ -607,7 +625,7 @@ private static ValueTask WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOn
             }
 
             if (CanUseScatterGatherWindowsAPIs(handle)
-                && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[] handlesToDispose, out IntPtr segmentsPtr, out int totalBytes))
+                && TryPrepareScatterGatherBuffers(buffers, default(ReadOnlyMemoryHandler), out MemoryHandle[]? handlesToDispose, out IntPtr segmentsPtr, out int totalBytes))
             {
                 return WriteGatherAtOffsetSingleSyscallAsync(handle, handlesToDispose, segmentsPtr, fileOffset, totalBytes, cancellationToken);
             }
@@ -671,13 +689,12 @@ private static unsafe ValueTask WriteFileGatherAsync(SafeFileHandle handle, IntP
 
         private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byte>> buffers, long fileOffset, CancellationToken cancellationToken)
         {
-            long bytesWritten = 0;
             int buffersCount = buffers.Count;
             for (int i = 0; i < buffersCount; i++)
             {
                 ReadOnlyMemory<byte> rom = buffers[i];
-                await WriteAtOffsetAsync(handle, rom, fileOffset + bytesWritten, cancellationToken).ConfigureAwait(false);
-                bytesWritten += rom.Length;
+                await WriteAtOffsetAsync(handle, rom, fileOffset, cancellationToken).ConfigureAwait(false);
+                fileOffset += rom.Length;
             }
         }