Reading beyond EOF should return 0 for FILE_FLAG_NO_BUFFERING

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

@@ -38,15 +38,6 @@ public async Task ReadUsingSingleBuffer(bool asyncOperation, bool asyncHandle)
                 int current = 0;
                 int total = 0;
 
-                // From https://docs.microsoft.com/en-us/windows/win32/fileio/file-buffering:
-                // "File access sizes, including the optional file offset in the OVERLAPPED structure,
-                // if specified, must be for a number of bytes that is an integer multiple of the volume sector size."
-                // So if buffer and physical sector size is 4096 and the file size is 4097:
-                // the read from offset=0 reads 4096 bytes
-                // the read from offset=4096 reads 1 byte
-                // the read from offset=4097 THROWS (Invalid argument, offset is not a multiple of sector size!)
-                // That is why we stop at the first incomplete read (the next one would throw).
-                // It's possible to get 0 if we are lucky and file size is a multiple of physical sector size.
                 do
                 {
                     current = asyncOperation
@@ -57,7 +48,7 @@ public async Task ReadUsingSingleBuffer(bool asyncOperation, bool asyncHandle)
 
                     total += current;
                 }
-                while (current == buffer.Memory.Length);
+                while (current != 0);
 
                 Assert.Equal(fileSize, total);
             }
@@ -222,6 +213,36 @@ public async Task ReadWriteAsyncUsingEmptyBuffers()
             await RandomAccess.WriteAsync(handle, Array.Empty<ReadOnlyMemory<byte>>(), 0);
         }
 
+        [Theory]
+        [MemberData(nameof(AllAsyncSyncCombinations))]
+        public async Task ReadShouldReturnZeroForEndOfFile(bool asyncOperation, bool asyncHandle)
+        {
+            int fileSize = Environment.SystemPageSize + 1; // it MUST NOT be a multiple of it (https://github.com/dotnet/runtime/issues/62851)
+            string filePath = GetTestFilePath();
+            byte[] expected = RandomNumberGenerator.GetBytes(fileSize);
+            File.WriteAllBytes(filePath, expected);
+
+            using FileStream fileStream = new (filePath, FileMode.Open, FileAccess.Read, FileShare.None, 0, GetFileOptions(asyncHandle));
+            using SectorAlignedMemory<byte> buffer = SectorAlignedMemory<byte>.Allocate(Environment.SystemPageSize);
+
+            int current = 0;
+            int total = 0;
+
+            do
+            {
+                current = asyncOperation
+                    ? await fileStream.ReadAsync(buffer.Memory)
+                    : fileStream.Read(buffer.GetSpan());
+
+                Assert.True(expected.AsSpan(total, current).SequenceEqual(buffer.GetSpan().Slice(0, current)));
+
+                total += current;
+            }
+            while (current != 0);
+
+            Assert.Equal(fileSize, total);
+        }
+
         // when using FileOptions.Asynchronous we are testing Scatter&Gather APIs on Windows (FILE_FLAG_OVERLAPPED requirement)
         private static FileOptions GetFileOptions(bool asyncHandle) => (asyncHandle ? FileOptions.Asynchronous : FileOptions.None) | NoBuffering; 
     }

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

@@ -57,6 +57,10 @@ internal bool SupportsRandomAccess
 
         internal void EnsureThreadPoolBindingInitialized() { /* nop */ }
 
+        internal bool LengthCanBeCached => false;
+
+        internal bool HasCachedFileLength => false;
+
         private static SafeFileHandle Open(string path, Interop.Sys.OpenFlags flags, int mode,
                                            Func<Interop.ErrorInfo, Interop.Sys.OpenFlags, string, Exception?>? createOpenException)
         {
@@ -504,6 +508,13 @@ private bool GetCanSeek()
             return canSeek == NullableBool.True;
         }
 
+        internal long GetFileLength()
+        {
+            int result = Interop.Sys.FStat(this, out Interop.Sys.FileStatus status);
+            FileStreamHelpers.CheckFileCall(result, Path);
+            return status.Size;
+        }
+
         private enum NullableBool
         {
             Undefined = 0,

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

@@ -4,7 +4,6 @@
 using System;
 using System.Diagnostics;
 using System.IO;
-using System.IO.Strategies;
 using System.Runtime.InteropServices;
 using System.Threading;
 
@@ -13,6 +12,8 @@ namespace Microsoft.Win32.SafeHandles
     public sealed partial class SafeFileHandle : SafeHandleZeroOrMinusOneIsInvalid
     {
         internal const FileOptions NoBuffering = (FileOptions)0x20000000;
+        private long _length = -1; // negative means that hasn't been fetched.
+        private bool _lengthCanBeCached; // file has been opened for reading and not shared for writing.
         private volatile FileOptions _fileOptions = (FileOptions)(-1);
         private volatile int _fileType = -1;
 
@@ -22,10 +23,16 @@ public SafeFileHandle() : base(true)
 
         public bool IsAsync => (GetFileOptions() & FileOptions.Asynchronous) != 0;
 
+        internal bool IsNoBuffering => (GetFileOptions() & NoBuffering) != 0;
+
         internal bool CanSeek => !IsClosed && GetFileType() == Interop.Kernel32.FileTypes.FILE_TYPE_DISK;
 
         internal ThreadPoolBoundHandle? ThreadPoolBinding { get; set; }
 
+        internal bool LengthCanBeCached => _lengthCanBeCached;
+
+        internal bool HasCachedFileLength => _lengthCanBeCached && _length >= 0;
+
         internal static unsafe SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize)
         {
             using (DisableMediaInsertionPrompt.Create())
@@ -103,6 +110,7 @@ private static unsafe SafeFileHandle CreateFile(string fullPath, FileMode mode,
 
             fileHandle._path = fullPath;
             fileHandle._fileOptions = options;
+            fileHandle._lengthCanBeCached = (share & FileShare.Write) == 0 && (access & FileAccess.Write) == 0;
             return fileHandle;
         }
 
@@ -252,5 +260,34 @@ internal int GetFileType()
 
             return fileType;
         }
+
+        internal unsafe long GetFileLength()
+        {
+            if (!_lengthCanBeCached)
+            {
+                return GetFileLengthCore();
+            }
+
+            // On Windows, when the file is locked for writes we can cache file length
+            // in memory and avoid subsequent native calls which are expensive.
+            if (_length < 0)
+            {
+                _length = GetFileLengthCore();
+            }
+
+            return _length;
+
+            long GetFileLengthCore()
+            {
+                Interop.Kernel32.FILE_STANDARD_INFO info;
+
+                if (!Interop.Kernel32.GetFileInformationByHandleEx(this, Interop.Kernel32.FileStandardInfo, &info, (uint)sizeof(Interop.Kernel32.FILE_STANDARD_INFO)))
+                {
+                    throw Win32Marshal.GetExceptionForLastWin32Error(Path);
+                }
+
+                return info.EndOfFile;
+            }
+        }
     }
 }

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

@@ -256,7 +256,7 @@ public static byte[] ReadAllBytes(string path)
             using (SafeFileHandle sfh = OpenHandle(path, FileMode.Open, FileAccess.Read, FileShare.Read, options))
             {
                 long fileLength = 0;
-                if (sfh.CanSeek && (fileLength = RandomAccess.GetFileLength(sfh)) > Array.MaxLength)
+                if (sfh.CanSeek && (fileLength = sfh.GetFileLength()) > Array.MaxLength)
                 {
                     throw new IOException(SR.IO_FileTooLong2GB);
                 }
@@ -530,7 +530,7 @@ private static async Task<string> InternalReadAllTextAsync(string path, Encoding
             SafeFileHandle sfh = OpenHandle(path, FileMode.Open, FileAccess.Read, FileShare.Read, options);
 
             long fileLength = 0L;
-            if (sfh.CanSeek && (fileLength = RandomAccess.GetFileLength(sfh)) > Array.MaxLength)
+            if (sfh.CanSeek && (fileLength = sfh.GetFileLength()) > Array.MaxLength)
             {
                 sfh.Dispose();
                 return Task.FromException<byte[]>(ExceptionDispatchInfo.SetCurrentStackTrace(new IOException(SR.IO_FileTooLong2GB)));

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

@@ -19,13 +19,6 @@ public static partial class RandomAccess
         // that get stackalloced in the Linux kernel.
         private const int IovStackThreshold = 8;
 
-        internal static long GetFileLength(SafeFileHandle handle)
-        {
-            int result = Interop.Sys.FStat(handle, out Interop.Sys.FileStatus status);
-            FileStreamHelpers.CheckFileCall(result, handle.Path);
-            return status.Size;
-        }
-
         internal static unsafe int ReadAtOffset(SafeFileHandle handle, Span<byte> buffer, long fileOffset)
         {
             fixed (byte* bufPtr = &MemoryMarshal.GetReference(buffer))

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

@@ -18,18 +18,6 @@ public static partial class RandomAccess
     {
         private static readonly IOCompletionCallback s_callback = AllocateCallback();
 
-        internal static unsafe long GetFileLength(SafeFileHandle handle)
-        {
-            Interop.Kernel32.FILE_STANDARD_INFO info;
-
-            if (!Interop.Kernel32.GetFileInformationByHandleEx(handle, Interop.Kernel32.FileStandardInfo, &info, (uint)sizeof(Interop.Kernel32.FILE_STANDARD_INFO)))
-            {
-                throw Win32Marshal.GetExceptionForLastWin32Error(handle.Path);
-            }
-
-            return info.EndOfFile;
-        }
-
         internal static unsafe int ReadAtOffset(SafeFileHandle handle, Span<byte> buffer, long fileOffset)
         {
             if (handle.IsAsync)
@@ -53,8 +41,8 @@ internal static unsafe int ReadAtOffset(SafeFileHandle handle, Span<byte> buffer
                         // "If lpOverlapped is not NULL, then when a synchronous read operation reaches the end of a file,
                         // ReadFile returns FALSE and GetLastError returns ERROR_HANDLE_EOF"
                         return numBytesRead;
-                    case Interop.Errors.ERROR_BROKEN_PIPE:
-                        // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe.
+                    case Interop.Errors.ERROR_BROKEN_PIPE: // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe.
+                    case Interop.Errors.ERROR_INVALID_PARAMETER when IsEndOfFileForNoBuffering(handle, fileOffset):
                         return 0;
                     default:
                         throw Win32Marshal.GetExceptionForWin32Error(errorCode, handle.Path);
@@ -100,6 +88,7 @@ private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<b
                     {
                         case Interop.Errors.ERROR_HANDLE_EOF: // logically success with 0 bytes read (read at end of file)
                         case Interop.Errors.ERROR_BROKEN_PIPE:
+                        case Interop.Errors.ERROR_INVALID_PARAMETER when IsEndOfFileForNoBuffering(handle, fileOffset):
                             // EOF on a pipe. Callback will not be called.
                             // We clear the overlapped status bit for this special case (failure
                             // to do so looks like we are freeing a pending overlapped later).
@@ -272,6 +261,7 @@ private static unsafe (SafeFileHandle.OverlappedValueTaskSource? vts, int errorC
 
                         case Interop.Errors.ERROR_HANDLE_EOF: // logically success with 0 bytes read (read at end of file)
                         case Interop.Errors.ERROR_BROKEN_PIPE:
+                        case Interop.Errors.ERROR_INVALID_PARAMETER when IsEndOfFileForNoBuffering(handle, fileOffset):
                             // EOF on a pipe. Callback will not be called.
                             // We clear the overlapped status bit for this special case (failure
                             // to do so looks like we are freeing a pending overlapped later).
@@ -743,6 +733,18 @@ static unsafe void Callback(uint errorCode, uint numBytes, NativeOverlapped* pOv
             }
         }
 
+        // From https://docs.microsoft.com/en-us/windows/win32/fileio/file-buffering:
+        // "File access sizes, including the optional file offset in the OVERLAPPED structure,
+        // if specified, must be for a number of bytes that is an integer multiple of the volume sector size."
+        // So if buffer and physical sector size is 4096 and the file size is 4097:
+        // the read from offset=0 reads 4096 bytes
+        // the read from offset=4096 reads 1 byte
+        // the read from offset=4097 fails with ERROR_INVALID_PARAMETER (the offset is not a multiple of sector size)
+        // Based on feedback received from customers (https://github.com/dotnet/runtime/issues/62851),
+        // it was decided to not throw, but just return 0.
+        private static bool IsEndOfFileForNoBuffering(SafeFileHandle fileHandle, long fileOffset)
+            => fileHandle.IsNoBuffering && fileHandle.CanSeek && fileOffset >= fileHandle.GetFileLength();
+
         // We need to store the reference count (see the comment in FreeNativeOverlappedIfItIsSafe) and an EventHandle to signal the completion.
         // We could keep these two things separate, but since ManualResetEvent is sealed and we want to avoid any extra allocations, this type has been created.
         // It's basically ManualResetEvent with reference count.

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

@@ -3,7 +3,6 @@
 
 using System.Collections.Generic;
 using System.IO.Strategies;
-using System.Runtime.CompilerServices;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Win32.SafeHandles;
@@ -25,7 +24,7 @@ public static long GetLength(SafeFileHandle handle)
         {
             ValidateInput(handle, fileOffset: 0);
 
-            return GetFileLength(handle);
+            return handle.GetFileLength();
         }
 
         /// <summary>

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs

@@ -4,7 +4,6 @@
 using System.Diagnostics;
 using System.Threading;
 using System.Threading.Tasks;
-using System.Threading.Tasks.Sources;
 using Microsoft.Win32.SafeHandles;
 
 namespace System.IO.Strategies
@@ -16,9 +15,7 @@ internal abstract class OSFileStreamStrategy : FileStreamStrategy
         private readonly FileAccess _access; // What file was opened for.
 
         protected long _filePosition;
-        private long _length = -1; // negative means that hasn't been fetched.
         private long _appendStart; // When appending, prevent overwriting file.
-        private bool _lengthCanBeCached; // SafeFileHandle hasn't been exposed, file has been opened for reading and not shared for writing.
 
         internal OSFileStreamStrategy(SafeFileHandle handle, FileAccess access)
         {
@@ -45,7 +42,6 @@ internal OSFileStreamStrategy(string path, FileMode mode, FileAccess access, Fil
             string fullPath = Path.GetFullPath(path);
 
             _access = access;
-            _lengthCanBeCached = (share & FileShare.Write) == 0 && (access & FileAccess.Write) == 0;
 
             _fileHandle = SafeFileHandle.Open(fullPath, mode, access, share, options, preallocationSize);
 
@@ -78,34 +74,13 @@ internal OSFileStreamStrategy(string path, FileMode mode, FileAccess access, Fil
 
         public sealed override bool CanWrite => !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0;
 
-        public unsafe sealed override long Length
-        {
-            get
-            {
-                if (!LengthCachingSupported)
-                {
-                    return RandomAccess.GetFileLength(_fileHandle);
-                }
-
-                // On Windows, when the file is locked for writes we can cache file length
-                // in memory and avoid subsequent native calls which are expensive.
-
-                if (_length < 0)
-                {
-                    _length = RandomAccess.GetFileLength(_fileHandle);
-                }
-
-                return _length;
-            }
-        }
+        public unsafe sealed override long Length => _fileHandle.GetFileLength();
 
         // in case of concurrent incomplete reads, there can be multiple threads trying to update the position
         // at the same time. That is why we are using Interlocked here.
         internal void OnIncompleteOperation(int expectedBytesTransferred, int actualBytesTransferred)
             => Interlocked.Add(ref _filePosition, actualBytesTransferred - expectedBytesTransferred);
 
-        private bool LengthCachingSupported => OperatingSystem.IsWindows() && _lengthCanBeCached;
-
         /// <summary>Gets or sets the position within the current stream</summary>
         public sealed override long Position
         {
@@ -129,9 +104,6 @@ internal sealed override SafeFileHandle SafeFileHandle
                     FileStreamHelpers.Seek(_fileHandle, _filePosition, SeekOrigin.Begin);
                 }
 
-                _lengthCanBeCached = false;
-                _length = -1; // invalidate cached length
-
                 return _fileHandle;
             }
         }
@@ -224,10 +196,7 @@ protected unsafe void SetLengthCore(long value)
             Debug.Assert(value >= 0, "value >= 0");
 
             FileStreamHelpers.SetFileLength(_fileHandle, value);
-            if (LengthCachingSupported)
-            {
-                _length = value;
-            }
+            Debug.Assert(!_fileHandle.LengthCanBeCached, "If length can be cached (file opened for reading, not shared for writing), it should be impossible to modify file length");
 
             if (_filePosition > value)
             {
@@ -314,7 +283,7 @@ public sealed override ValueTask<int> ReadAsync(Memory<byte> destination, Cancel
                 return RandomAccess.ReadAtOffsetAsync(_fileHandle, destination, fileOffset: -1, cancellationToken);
             }
 
-            if (LengthCachingSupported && _length >= 0 && Volatile.Read(ref _filePosition) >= _length)
+            if (_fileHandle.HasCachedFileLength && Volatile.Read(ref _filePosition) >= _fileHandle.GetFileLength())
             {
                 // We know for sure that the file length can be safely cached and it has already been obtained.
                 // If we have reached EOF we just return here and avoid a sys-call.