From 999bd3145bea7a64f6752ae95e9a8eaee334c649 Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Wed, 15 Aug 2018 21:16:43 -0700 Subject: [PATCH] Make file handle checks accurate on Windows (dotnet/coreclr#19508) * Make handle checks accurate on Windows We can actually check the async state of a handle on Windows, so check directly when we have the API available. Also allow all filetypes through when explicitly using extended syntax `\\?\` . * Fix nits * Don't throw in the handle constructor overloads * Whoops * Removing the close handle entirely. No adverse effects from closing a bad handle- or leaving it open for that matter. Commit migrated from https://github.com/dotnet/coreclr/commit/3da101be85ee07b9b625f03b7570a03a5cd028cc --- .../src/Interop/Windows/Interop.Libraries.cs | 1 + .../Windows/NtDll/NtQueryInformationFile.cs | 46 ++++++++++++ .../src/System.Private.CoreLib.Shared.projitems | 1 + .../src/System/IO/FileStream.Unix.cs | 4 +- .../src/System/IO/FileStream.Win32.cs | 58 ++++++++++++++++ .../src/System/IO/FileStream.WinRT.cs | 49 +++++++++++++ .../src/System/IO/FileStream.Windows.cs | 81 ++++++---------------- .../src/System/IO/FileStream.cs | 9 +-- 8 files changed, 180 insertions(+), 69 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/Interop/Windows/NtDll/NtQueryInformationFile.cs diff --git a/src/libraries/System.Private.CoreLib/src/Interop/Windows/Interop.Libraries.cs b/src/libraries/System.Private.CoreLib/src/Interop/Windows/Interop.Libraries.cs index 7b77d2d..398d18a 100644 --- a/src/libraries/System.Private.CoreLib/src/Interop/Windows/Interop.Libraries.cs +++ b/src/libraries/System.Private.CoreLib/src/Interop/Windows/Interop.Libraries.cs @@ -13,5 +13,6 @@ internal static partial class Interop internal const string Ole32 = "ole32.dll"; internal const string OleAut32 = "oleaut32.dll"; internal const string User32 = "user32.dll"; + internal const string NtDll = "ntdll.dll"; } } diff --git a/src/libraries/System.Private.CoreLib/src/Interop/Windows/NtDll/NtQueryInformationFile.cs b/src/libraries/System.Private.CoreLib/src/Interop/Windows/NtDll/NtQueryInformationFile.cs new file mode 100644 index 0000000..4ba39a7 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/Interop/Windows/NtDll/NtQueryInformationFile.cs @@ -0,0 +1,46 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Microsoft.Win32.SafeHandles; +using System; +using System.Runtime.InteropServices; + +internal partial class Interop +{ + internal partial class NtDll + { + [DllImport(Libraries.NtDll, ExactSpelling = true)] + unsafe internal static extern int NtQueryInformationFile( + SafeFileHandle FileHandle, + out IO_STATUS_BLOCK IoStatusBlock, + void* FileInformation, + uint Length, + uint FileInformationClass); + + [StructLayout(LayoutKind.Sequential)] + internal struct IO_STATUS_BLOCK + { + IO_STATUS Status; + IntPtr Information; + } + + // This isn't an actual Windows type, we have to separate it out as the size of IntPtr varies by architecture + // and we can't specify the size at compile time to offset the Information pointer in the status block. + [StructLayout(LayoutKind.Explicit)] + internal struct IO_STATUS + { + [FieldOffset(0)] + int Status; + + [FieldOffset(0)] + IntPtr Pointer; + } + + internal const uint FileModeInformation = 16; + internal const uint FILE_SYNCHRONOUS_IO_ALERT = 0x00000010; + internal const uint FILE_SYNCHRONOUS_IO_NONALERT = 0x00000020; + + internal const int STATUS_INVALID_HANDLE = unchecked((int)0xC0000008); + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index b78eebc..6d0952e 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -805,6 +805,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs index d9fcf65..ae4b709 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs @@ -61,10 +61,12 @@ namespace System.IO return SafeFileHandle.Open(_path, openFlags, (int)OpenPermissions); } + private static bool GetDefaultIsAsync(SafeFileHandle handle) => handle.IsAsync ?? DefaultIsAsync; + /// Initializes a stream for reading or writing a Unix file. /// How the file should be opened. /// What other access to the file should be allowed. This is currently ignored. - private void Init(FileMode mode, FileShare share) + private void Init(FileMode mode, FileShare share, string originalPath) { _fileHandle.IsAsync = _useAsyncIO; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Win32.cs index 4b75ad6..bca553a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Win32.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Win32.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Diagnostics; using Microsoft.Win32.SafeHandles; namespace System.IO @@ -43,5 +44,62 @@ namespace System.IO Interop.Kernel32.CreateFile(_path, fAccess, share, ref secAttrs, mode, flagsAndAttributes, IntPtr.Zero)); } } + + private static bool GetDefaultIsAsync(SafeFileHandle handle) + { + return handle.IsAsync ?? !IsHandleSynchronous(handle, ignoreInvalid: true) ?? DefaultIsAsync; + } + + private static unsafe bool? IsHandleSynchronous(SafeFileHandle fileHandle, bool ignoreInvalid) + { + if (fileHandle.IsInvalid) + return null; + + uint fileMode; + + int status = Interop.NtDll.NtQueryInformationFile( + FileHandle: fileHandle, + IoStatusBlock: out Interop.NtDll.IO_STATUS_BLOCK ioStatus, + FileInformation: &fileMode, + Length: sizeof(uint), + FileInformationClass: Interop.NtDll.FileModeInformation); + + switch (status) + { + case 0: + // We we're successful + break; + case Interop.NtDll.STATUS_INVALID_HANDLE: + if (!ignoreInvalid) + { + throw Win32Marshal.GetExceptionForWin32Error(Interop.Errors.ERROR_INVALID_HANDLE); + } + else + { + return null; + } + default: + // Something else is preventing access + Debug.Fail("Unable to get the file mode information, status was" + status.ToString()); + return null; + } + + // If either of these two flags are set, the file handle is synchronous (not overlapped) + return (fileMode & (Interop.NtDll.FILE_SYNCHRONOUS_IO_ALERT | Interop.NtDll.FILE_SYNCHRONOUS_IO_NONALERT)) > 0; + } + + private static void VerifyHandleIsSync(SafeFileHandle handle, int fileType) + { + // As we can accurately check the handle type when we have access to NtQueryInformationFile we don't need to skip for + // any particular file handle type. + + // If the handle was passed in without an explicit async setting, we already looked it up in GetDefaultIsAsync + if (!handle.IsAsync.HasValue) + return; + + // If we can't check the handle, just assume it is ok. + if (!(IsHandleSynchronous(handle, ignoreInvalid: false) ?? true)) + throw new ArgumentException(SR.Arg_HandleNotSync, nameof(handle)); + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.WinRT.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.WinRT.cs index 304088e..4e6d879 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.WinRT.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.WinRT.cs @@ -40,5 +40,54 @@ namespace System.IO pCreateExParams: ref parameters)); } } + + private static bool GetDefaultIsAsync(SafeFileHandle handle) => handle.IsAsync ?? DefaultIsAsync; + + private static unsafe bool? IsHandleSynchronous(SafeFileHandle handle) + { + // Do NOT use this method on any type other than DISK. Reading or writing to a pipe may + // cause an app to block incorrectly, introducing a deadlock (depending on whether a write + // will wake up an already-blocked thread or this Win32FileStream's thread). + + byte* bytes = stackalloc byte[1]; + int numBytesReadWritten; + int r = -1; + + // If the handle is a pipe, ReadFile will block until there + // has been a write on the other end. We'll just have to deal with it, + // For the read end of a pipe, you can mess up and + // accidentally read synchronously from an async pipe. + if (_canRead) + r = Interop.Kernel32.ReadFile(handle, bytes, 0, out numBytesReadWritten, IntPtr.Zero); + else if (_canWrite) + r = Interop.Kernel32.WriteFile(handle, bytes, 0, out numBytesReadWritten, IntPtr.Zero); + + if (r == 0) + { + int errorCode = Marshal.GetLastWin32Error(); + switch (errorCode) + { + case Interop.Errors.ERROR_INVALID_PARAMETER: + return false; + case Interop.Errors.ERROR_INVALID_HANDLE: + throw Win32Marshal.GetExceptionForWin32Error(errorCode); + } + } + + return true; + } + + private static void VerifyHandleIsSync(SafeFileHandle handle, int fileType) + { + // The technique here only really works for FILE_TYPE_DISK. FileMode is the right thing to check, but it currently + // isn't available in WinRT. + + if (fileType == Interop.mincore.FileTypes.FILE_TYPE_DISK) + { + // If we can't check the handle, just assume it is ok. + if (!(IsHandleSynchronous() ?? true)) + throw new ArgumentException(SR.Arg_HandleNotSync, nameof(handle)); + } + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs index 257e853..0b3f9c3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs @@ -52,24 +52,29 @@ namespace System.IO private PreAllocatedOverlapped _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations private FileStreamCompletionSource _currentOverlappedOwner; // async op currently using the preallocated overlapped - private void Init(FileMode mode, FileShare share) + private void Init(FileMode mode, FileShare share, string originalPath) { - // Disallow access to all non-file devices from the Win32FileStream - // constructors that take a String. Everyone else can call - // CreateFile themselves then use the constructor that takes an - // IntPtr. Disallows "con:", "com1:", "lpt1:", etc. - int fileType = Interop.Kernel32.GetFileType(_fileHandle); - if (fileType != Interop.Kernel32.FileTypes.FILE_TYPE_DISK) + if (!PathInternal.IsExtended(originalPath)) { - int errorCode = fileType == Interop.Kernel32.FileTypes.FILE_TYPE_UNKNOWN ? Marshal.GetLastWin32Error() : Interop.Errors.ERROR_SUCCESS; + // To help avoid stumbling into opening COM/LPT ports by accident, we will block on non file handles unless + // we were explicitly passed a path that has \\?\. GetFullPath() will turn paths like C:\foo\con.txt into + // \\.\CON, so we'll only allow the \\?\ syntax. - _fileHandle.Dispose(); - - if (errorCode != Interop.Errors.ERROR_SUCCESS) + int fileType = Interop.Kernel32.GetFileType(_fileHandle); + if (fileType != Interop.Kernel32.FileTypes.FILE_TYPE_DISK) { - throw Win32Marshal.GetExceptionForWin32Error(errorCode); + int errorCode = fileType == Interop.Kernel32.FileTypes.FILE_TYPE_UNKNOWN + ? Marshal.GetLastWin32Error() + : Interop.Errors.ERROR_SUCCESS; + + _fileHandle.Dispose(); + + if (errorCode != Interop.Errors.ERROR_SUCCESS) + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode); + } + throw new NotSupportedException(SR.NotSupported_FileStreamOnNonFiles); } - throw new NotSupportedException(SR.NotSupported_FileStreamOnNonFiles); } // This is necessary for async IO using IO Completion ports via our @@ -168,8 +173,7 @@ namespace System.IO } else if (!useAsyncIO) { - if (handleType != Interop.Kernel32.FileTypes.FILE_TYPE_PIPE) - VerifyHandleIsSync(handle, access); + VerifyHandleIsSync(handle, handleType); } if (_canSeek) @@ -192,46 +196,6 @@ namespace System.IO return secAttrs; } - // Verifies that this handle supports synchronous IO operations (unless you - // didn't open it for either reading or writing). - private static unsafe void VerifyHandleIsSync(SafeFileHandle handle, FileAccess access) - { - // Do NOT use this method on pipes. Reading or writing to a pipe may - // cause an app to block incorrectly, introducing a deadlock (depending - // on whether a write will wake up an already-blocked thread or this - // Win32FileStream's thread). - Debug.Assert(Interop.Kernel32.GetFileType(handle) != Interop.Kernel32.FileTypes.FILE_TYPE_PIPE); - - byte* bytes = stackalloc byte[1]; - int numBytesReadWritten; - int r = -1; - - // If the handle is a pipe, ReadFile will block until there - // has been a write on the other end. We'll just have to deal with it, - // For the read end of a pipe, you can mess up and - // accidentally read synchronously from an async pipe. - if ((access & FileAccess.Read) != 0) // don't use the virtual CanRead or CanWrite, as this may be used in the ctor - { - r = Interop.Kernel32.ReadFile(handle, bytes, 0, out numBytesReadWritten, IntPtr.Zero); - } - else if ((access & FileAccess.Write) != 0) // don't use the virtual CanRead or CanWrite, as this may be used in the ctor - { - r = Interop.Kernel32.WriteFile(handle, bytes, 0, out numBytesReadWritten, IntPtr.Zero); - } - - if (r == 0) - { - int errorCode = Marshal.GetLastWin32Error(); - switch (errorCode) - { - case Interop.Errors.ERROR_INVALID_PARAMETER: - throw new ArgumentException(SR.Arg_HandleNotSync, "handle"); - case Interop.Errors.ERROR_INVALID_HANDLE: - throw Win32Marshal.GetExceptionForWin32Error(errorCode); - } - } - } - private bool HasActiveBufferOperation => _activeBufferOperation != null && !_activeBufferOperation.IsCompleted; @@ -627,7 +591,7 @@ namespace System.IO { if (closeInvalidHandle) { - throw Win32Marshal.GetExceptionForWin32Error(GetLastWin32ErrorAndDisposeHandleIfInvalid(throwIfInvalidHandle: false), _path); + throw Win32Marshal.GetExceptionForWin32Error(GetLastWin32ErrorAndDisposeHandleIfInvalid(), _path); } else { @@ -1238,7 +1202,7 @@ namespace System.IO } } - private int GetLastWin32ErrorAndDisposeHandleIfInvalid(bool throwIfInvalidHandle = false) + private int GetLastWin32ErrorAndDisposeHandleIfInvalid() { int errorCode = Marshal.GetLastWin32Error(); @@ -1262,9 +1226,6 @@ namespace System.IO if (errorCode == Interop.Errors.ERROR_INVALID_HANDLE) { _fileHandle.Dispose(); - - if (throwIfInvalidHandle) - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); } return errorCode; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs index 97740f3..fadbef8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -6,7 +6,6 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.Win32.SafeHandles; using System.Diagnostics; -using System.Security; namespace System.IO { @@ -233,7 +232,7 @@ namespace System.IO try { - Init(mode, share); + Init(mode, share, path); } catch { @@ -245,12 +244,6 @@ namespace System.IO } } - private static bool GetDefaultIsAsync(SafeFileHandle handle) - { - // This will eventually get more complicated as we can actually check the underlying handle type on Windows - return handle.IsAsync.HasValue ? handle.IsAsync.Value : false; - } - [Obsolete("This property has been deprecated. Please use FileStream's SafeFileHandle property instead. http://go.microsoft.com/fwlink/?linkid=14202")] public virtual IntPtr Handle { get { return SafeFileHandle.DangerousGetHandle(); } } -- 2.7.4