From 2f67019615bea51ff592bc181ceff204f93db670 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 19 Mar 2026 23:04:06 +0100 Subject: [PATCH 1/4] fix SafeFileHandle.CanSeek performance regression, fixes #125660 --- .../Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs index 54486b367900cc..797ccb560ecb1b 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs @@ -336,6 +336,11 @@ internal FileHandleType GetFileTypeCore() { Interop.Kernel32.FileTypes.FILE_TYPE_CHAR => FileHandleType.CharacterDevice, Interop.Kernel32.FileTypes.FILE_TYPE_PIPE => GetPipeOrSocketType(), + // GetFileType can return FILE_TYPE_DISK for regular files, directories and symbolic links. + // However, when Path is not null, it means that the handle was created by SafeFileHandle.Open + // which does not allow opening directories or symbolic links (it resolves them). + // That is why we can skip the extra check performed by GetDiskBasedType. + Interop.Kernel32.FileTypes.FILE_TYPE_DISK when Path is not null => FileHandleType.RegularFile, Interop.Kernel32.FileTypes.FILE_TYPE_DISK => GetDiskBasedType(), _ => FileHandleType.Unknown }; From 342d089a2c271d9a76a738f5656355dbce689674 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 20 Mar 2026 12:38:02 +0100 Subject: [PATCH 2/4] fix the bug and improve test coverage: - use existing public API for opening directories as SafeFileHandle to ensure we have test coverage for it - make sure NoBuffering and BackupOrRestore are not defined in multiple places - fix the bug - remove volatile --- .../SafeHandles/SafeFileHandle.Windows.cs | 26 ++++++++++------- .../src/System/IO/RandomAccess.Windows.cs | 2 +- .../System/IO/Strategies/FileStreamHelpers.cs | 5 +++- .../SafeFileHandle/GetFileType.Windows.cs | 29 ++++++++++++------- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs index 797ccb560ecb1b..693bb0f578dcf3 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs @@ -2,10 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; -using System.Buffers; using System.ComponentModel; using System.Diagnostics; using System.IO; +using System.IO.Strategies; using System.Runtime.InteropServices; using System.Threading; @@ -13,10 +13,10 @@ namespace Microsoft.Win32.SafeHandles { public sealed partial class SafeFileHandle : SafeHandleZeroOrMinusOneIsInvalid { - internal const FileOptions NoBuffering = (FileOptions)0x20000000; + private const FileOptions UnititializedOptions = (FileOptions)(-1); 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 FileOptions _fileOptions = UnititializedOptions; public SafeFileHandle() : base(true) { @@ -106,7 +106,7 @@ public static partial void CreateAnonymousPipe(out SafeFileHandle readHandle, ou public bool IsAsync => (GetFileOptions() & FileOptions.Asynchronous) != 0; - internal bool IsNoBuffering => (GetFileOptions() & NoBuffering) != 0; + internal bool IsNoBuffering => (GetFileOptions() & FileStreamHelpers.NoBuffering) != 0; internal bool CanSeek => !IsClosed && Type == FileHandleType.RegularFile; @@ -280,7 +280,7 @@ private void InitThreadPoolBinding() internal unsafe FileOptions GetFileOptions() { FileOptions fileOptions = _fileOptions; - if (fileOptions != (FileOptions)(-1)) + if (fileOptions != UnititializedOptions) { return fileOptions; } @@ -323,7 +323,7 @@ internal unsafe FileOptions GetFileOptions() } if ((options & Interop.NtDll.CreateOptions.FILE_NO_INTERMEDIATE_BUFFERING) != 0) { - result |= NoBuffering; + result |= FileStreamHelpers.NoBuffering; } return _fileOptions = result; @@ -331,16 +331,22 @@ internal unsafe FileOptions GetFileOptions() internal FileHandleType GetFileTypeCore() { + Debug.Assert(Path is null || _fileOptions != UnititializedOptions, "When Path is set, _fileOptions are also provided"); + int kernelFileType = Interop.Kernel32.GetFileType(this); return kernelFileType switch { Interop.Kernel32.FileTypes.FILE_TYPE_CHAR => FileHandleType.CharacterDevice, Interop.Kernel32.FileTypes.FILE_TYPE_PIPE => GetPipeOrSocketType(), // GetFileType can return FILE_TYPE_DISK for regular files, directories and symbolic links. - // However, when Path is not null, it means that the handle was created by SafeFileHandle.Open - // which does not allow opening directories or symbolic links (it resolves them). - // That is why we can skip the extra check performed by GetDiskBasedType. - Interop.Kernel32.FileTypes.FILE_TYPE_DISK when Path is not null => FileHandleType.RegularFile, + // When Path is not null, it means that the handle was created by SafeFileHandle.Open. + // This method resolves symbolic links, so it can't be a symbolic link. + // However, it accepts FILE_FLAG_BACKUP_SEMANTICS as an option, + // which makes it possible to open a directory. + // So when Path is not null and options don't include FILE_FLAG_BACKUP_SEMANTICS, + // it's a regular file. In such case, we don't need to call GetDiskBasedType() + // and we can avoid performing two sys-calls and regressing performance for CanSeek. + Interop.Kernel32.FileTypes.FILE_TYPE_DISK when Path is not null && ((_fileOptions & FileStreamHelpers.BackupOrRestore) == 0) => FileHandleType.RegularFile, Interop.Kernel32.FileTypes.FILE_TYPE_DISK => GetDiskBasedType(), _ => FileHandleType.Unknown }; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 3ece14b5fd82a7..ad3614376ab396 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -448,7 +448,7 @@ internal static void WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyList handle.IsAsync && ((handle.GetFileOptions() & SafeFileHandle.NoBuffering) != 0); + => handle.IsAsync && ((handle.GetFileOptions() & FileStreamHelpers.NoBuffering) != 0); // From the same source: // "Each buffer must be at least the size of a system memory page and must be aligned on a system diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs index e3676a6a4e2835..af2bfd18220908 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs @@ -8,10 +8,13 @@ namespace System.IO.Strategies { internal static partial class FileStreamHelpers { + internal const FileOptions NoBuffering = (FileOptions)0x20000000; + internal const FileOptions BackupOrRestore = (FileOptions)0x02000000; + // NOTE: any change to FileOptions enum needs to be matched here as it's used in the error validation private const FileOptions ValidFileOptions = FileOptions.WriteThrough | FileOptions.Asynchronous | FileOptions.RandomAccess | FileOptions.DeleteOnClose | FileOptions.SequentialScan | FileOptions.Encrypted - | (FileOptions)0x20000000 /* NoBuffering */ | (FileOptions)0x02000000 /* BackupOrRestore */; + | NoBuffering | BackupOrRestore; /// Caches whether Serialization Guard has been disabled for file writes private static int s_cachedSerializationSwitch; diff --git a/src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/SafeFileHandle/GetFileType.Windows.cs b/src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/SafeFileHandle/GetFileType.Windows.cs index 00fba057c13d09..90ad33818e2f1d 100644 --- a/src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/SafeFileHandle/GetFileType.Windows.cs +++ b/src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/SafeFileHandle/GetFileType.Windows.cs @@ -12,20 +12,29 @@ namespace System.IO.Tests [PlatformSpecific(TestPlatforms.Windows)] public class SafeFileHandle_GetFileType_Windows : FileSystemTest { - [Fact] - public unsafe void GetFileType_Directory() + [Theory] + [InlineData(true)] + [InlineData(false)] + public unsafe void GetFileType_Directory(bool usePublicAPI) { string path = GetTestFilePath(); Directory.CreateDirectory(path); - using SafeFileHandle handle = Interop.Kernel32.CreateFile( - path, - Interop.Kernel32.GenericOperations.GENERIC_READ, - FileShare.ReadWrite, - null, - FileMode.Open, - Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS, - IntPtr.Zero); + using SafeFileHandle handle = usePublicAPI + ? File.OpenHandle( + path, + FileMode.Open, + FileAccess.Read, + FileShare.ReadWrite, + (FileOptions)Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS) + : Interop.Kernel32.CreateFile( + path, + Interop.Kernel32.GenericOperations.GENERIC_READ, + FileShare.ReadWrite, + null, + FileMode.Open, + Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS, + IntPtr.Zero); Assert.False(handle.IsInvalid); Assert.Equal(FileHandleType.Directory, handle.Type); From 7c5d1536fc76ef7a5b30ec00c1d05804a45913c9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 20 Mar 2026 13:35:05 +0100 Subject: [PATCH 3/4] fix the typo --- .../Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs index 693bb0f578dcf3..f6ef273e0b51fe 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs @@ -13,10 +13,10 @@ namespace Microsoft.Win32.SafeHandles { public sealed partial class SafeFileHandle : SafeHandleZeroOrMinusOneIsInvalid { - private const FileOptions UnititializedOptions = (FileOptions)(-1); + private const FileOptions UninitializedOptions = (FileOptions)(-1); 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 FileOptions _fileOptions = UnititializedOptions; + private FileOptions _fileOptions = UninitializedOptions; public SafeFileHandle() : base(true) { @@ -280,7 +280,7 @@ private void InitThreadPoolBinding() internal unsafe FileOptions GetFileOptions() { FileOptions fileOptions = _fileOptions; - if (fileOptions != UnititializedOptions) + if (fileOptions != UninitializedOptions) { return fileOptions; } @@ -331,7 +331,7 @@ internal unsafe FileOptions GetFileOptions() internal FileHandleType GetFileTypeCore() { - Debug.Assert(Path is null || _fileOptions != UnititializedOptions, "When Path is set, _fileOptions are also provided"); + Debug.Assert(Path is null || _fileOptions != UninitializedOptions, "When Path is set, _fileOptions are also provided."); int kernelFileType = Interop.Kernel32.GetFileType(this); return kernelFileType switch From 002c5fc94d5faaaddacf988218aa22d20fb8c96c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 20 Mar 2026 23:31:52 +0100 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Dan Moseley --- .../src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs index f6ef273e0b51fe..393154799ce2f4 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs @@ -344,8 +344,8 @@ internal FileHandleType GetFileTypeCore() // However, it accepts FILE_FLAG_BACKUP_SEMANTICS as an option, // which makes it possible to open a directory. // So when Path is not null and options don't include FILE_FLAG_BACKUP_SEMANTICS, - // it's a regular file. In such case, we don't need to call GetDiskBasedType() - // and we can avoid performing two sys-calls and regressing performance for CanSeek. + // it's a regular file. In such case, we don't need to call GetDiskBasedType() which would be + // an extra sys-call. Interop.Kernel32.FileTypes.FILE_TYPE_DISK when Path is not null && ((_fileOptions & FileStreamHelpers.BackupOrRestore) == 0) => FileHandleType.RegularFile, Interop.Kernel32.FileTypes.FILE_TYPE_DISK => GetDiskBasedType(), _ => FileHandleType.Unknown