Make file handle checks accurate on Windows (dotnet/coreclr#19508)
authorJeremy Kuhne <jeremy.kuhne@microsoft.com>
Thu, 16 Aug 2018 04:16:43 +0000 (21:16 -0700)
committerGitHub <noreply@github.com>
Thu, 16 Aug 2018 04:16:43 +0000 (21:16 -0700)
* 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/libraries/System.Private.CoreLib/src/Interop/Windows/Interop.Libraries.cs
src/libraries/System.Private.CoreLib/src/Interop/Windows/NtDll/NtQueryInformationFile.cs [new file with mode: 0644]
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Win32.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.WinRT.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs
src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

index 7b77d2d..398d18a 100644 (file)
@@ -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 (file)
index 0000000..4ba39a7
--- /dev/null
@@ -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);
+    }
+}
index b78eebc..6d0952e 100644 (file)
     <Compile Include="$(MSBuildThisFileDirectory)System\Security\SecureString.Windows.cs" />
   </ItemGroup>
   <ItemGroup Condition="$(TargetsWindows) and '$(EnableWinRT)' != 'true'">
+    <Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\NtDll\NtQueryInformationFile.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\IO\FileStream.Win32.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\TimeZoneInfo.Win32.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Microsoft\Win32\SafeHandles\SafeLibraryHandle.cs" />
index d9fcf65..ae4b709 100644 (file)
@@ -61,10 +61,12 @@ namespace System.IO
             return SafeFileHandle.Open(_path, openFlags, (int)OpenPermissions);
         }
 
+        private static bool GetDefaultIsAsync(SafeFileHandle handle) => handle.IsAsync ?? DefaultIsAsync;
+
         /// <summary>Initializes a stream for reading or writing a Unix file.</summary>
         /// <param name="mode">How the file should be opened.</param>
         /// <param name="share">What other access to the file should be allowed.  This is currently ignored.</param>
-        private void Init(FileMode mode, FileShare share)
+        private void Init(FileMode mode, FileShare share, string originalPath)
         {
             _fileHandle.IsAsync = _useAsyncIO;
 
index 4b75ad6..bca553a 100644 (file)
@@ -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));
+        }
     }
 }
index 304088e..4e6d879 100644 (file)
@@ -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));
+            }
+        }
     }
 }
index 257e853..0b3f9c3 100644 (file)
@@ -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;
index 97740f3..fadbef8 100644 (file)
@@ -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(); } }