Don't close passed in FileStream handles in constructor (dotnet/coreclr#12253)
authorJeremy Kuhne <jeremy.kuhne@microsoft.com>
Mon, 19 Jun 2017 22:31:01 +0000 (15:31 -0700)
committerGitHub <noreply@github.com>
Mon, 19 Jun 2017 22:31:01 +0000 (15:31 -0700)
* Don't close passed in FileStream handles in constructor

As the finalizer will run when throwing we need to be
careful to make sure we don't put the passed-in handle
in the member field until we're sure we've succeeeded.

Additionally we need to make sure any wrapping SafeHandle
we create on a passed in IntPtr isn't collected.

* Move platform specific assertion and add comment.

* Comment- fix mistake in CanSeekCore

Commit migrated from https://github.com/dotnet/coreclr/commit/8a8bda14a618b14264cb85c012e097b3e01992e9

src/coreclr/src/mscorlib/shared/System/IO/FileStream.Unix.cs
src/coreclr/src/mscorlib/shared/System/IO/FileStream.Windows.cs
src/coreclr/src/mscorlib/shared/System/IO/FileStream.cs

index 7d860ac..0fe3b95 100644 (file)
@@ -105,15 +105,12 @@ namespace System.IO
         }
 
         /// <summary>Initializes a stream from an already open file handle (file descriptor).</summary>
-        /// <param name="handle">The handle to the file.</param>
-        /// <param name="bufferSize">The size of the buffer to use when buffering.</param>
-        /// <param name="useAsyncIO">Whether access to the stream is performed asynchronously.</param>
-        private void InitFromHandle(SafeFileHandle handle)
+        private void InitFromHandle(SafeFileHandle handle, FileAccess access, bool useAsyncIO)
         {
-            if (_useAsyncIO)
+            if (useAsyncIO)
                 _asyncState = new AsyncState();
 
-            if (CanSeekCore) // use non-virtual CanSeekCore rather than CanSeek to avoid making virtual call during ctor
+            if (CanSeekCore(handle)) // use non-virtual CanSeekCore rather than CanSeek to avoid making virtual call during ctor
                 SeekCore(0, SeekOrigin.Current);
         }
 
@@ -189,26 +186,27 @@ namespace System.IO
         }
 
         /// <summary>Gets a value indicating whether the current stream supports seeking.</summary>
-        public override bool CanSeek => CanSeekCore;
+        public override bool CanSeek => CanSeekCore(_fileHandle);
 
         /// <summary>Gets a value indicating whether the current stream supports seeking.</summary>
-        /// <remarks>Separated out of CanSeek to enable making non-virtual call to this logic.</remarks>
-        private bool CanSeekCore
+        /// <remarks>
+        /// Separated out of CanSeek to enable making non-virtual call to this logic.
+        /// We also pass in the file handle to allow the constructor to use this before it stashes the handle.
+        /// </remarks>
+        private bool CanSeekCore(SafeFileHandle fileHandle)
         {
-            get
+            if (fileHandle.IsClosed)
             {
-                if (_fileHandle.IsClosed)
-                {
-                    return false;
-                }
+                return false;
+            }
 
-                if (!_canSeek.HasValue)
-                {
-                    // Lazily-initialize whether we're able to seek, tested by seeking to our current location.
-                    _canSeek = Interop.Sys.LSeek(_fileHandle, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0;
-                }
-                return _canSeek.Value;
+            if (!_canSeek.HasValue)
+            {
+                // Lazily-initialize whether we're able to seek, tested by seeking to our current location.
+                _canSeek = Interop.Sys.LSeek(fileHandle, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0;
             }
+
+            return _canSeek.Value;
         }
 
         private long GetLengthInternal()
index c036ee6..6e7ba74 100644 (file)
@@ -108,9 +108,28 @@ namespace System.IO
             }
         }
 
-        private void InitFromHandle(SafeFileHandle handle)
+        private void InitFromHandle(SafeFileHandle handle, FileAccess access, bool useAsyncIO)
         {
-            int handleType = Interop.Kernel32.GetFileType(_fileHandle);
+#if DEBUG
+            bool hadBinding = handle.ThreadPoolBinding != null;
+
+            try
+            {
+#endif
+                InitFromHandleImpl(handle, access, useAsyncIO);
+#if DEBUG
+            }
+            catch
+            {
+                Debug.Assert(hadBinding || handle.ThreadPoolBinding == null, "We should never error out with a ThreadPoolBinding we've added");
+                throw;
+            }
+#endif
+        }
+
+        private void InitFromHandleImpl(SafeFileHandle handle, FileAccess access, bool useAsyncIO)
+        {
+            int handleType = Interop.Kernel32.GetFileType(handle);
             Debug.Assert(handleType == Interop.Kernel32.FileTypes.FILE_TYPE_DISK || handleType == Interop.Kernel32.FileTypes.FILE_TYPE_PIPE || handleType == Interop.Kernel32.FileTypes.FILE_TYPE_CHAR, "FileStream was passed an unknown file type!");
 
             _canSeek = handleType == Interop.Kernel32.FileTypes.FILE_TYPE_DISK;
@@ -128,11 +147,11 @@ namespace System.IO
             // If, however, we've already bound this file handle to our completion port,
             // don't try to bind it again because it will fail.  A handle can only be
             // bound to a single completion port at a time.
-            if (_useAsyncIO && !GetSuppressBindHandle(handle))
+            if (useAsyncIO && !(handle.IsAsync ?? false))
             {
                 try
                 {
-                    _fileHandle.ThreadPoolBinding = ThreadPoolBoundHandle.BindHandle(_fileHandle);
+                    handle.ThreadPoolBinding = ThreadPoolBoundHandle.BindHandle(handle);
                 }
                 catch (Exception ex)
                 {
@@ -141,10 +160,10 @@ namespace System.IO
                     throw new ArgumentException(SR.Arg_HandleNotAsync, nameof(handle), ex);
                 }
             }
-            else if (!_useAsyncIO)
+            else if (!useAsyncIO)
             {
                 if (handleType != Interop.Kernel32.FileTypes.FILE_TYPE_PIPE)
-                    VerifyHandleIsSync();
+                    VerifyHandleIsSync(handle, access);
             }
 
             if (_canSeek)
@@ -153,11 +172,6 @@ namespace System.IO
                 _filePosition = 0;
         }
 
-        private static bool GetSuppressBindHandle(SafeFileHandle handle)
-        {
-            return handle.IsAsync.HasValue ? handle.IsAsync.Value : false;
-        }
-
         private unsafe static Interop.Kernel32.SECURITY_ATTRIBUTES GetSecAttrs(FileShare share)
         {
             Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = default(Interop.Kernel32.SECURITY_ATTRIBUTES);
@@ -173,15 +187,13 @@ namespace System.IO
 
         // Verifies that this handle supports synchronous IO operations (unless you
         // didn't open it for either reading or writing).
-        private unsafe void VerifyHandleIsSync()
+        private unsafe static void VerifyHandleIsSync(SafeFileHandle handle, FileAccess access)
         {
-            Debug.Assert(!_useAsyncIO);
-
             // 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(_fileHandle) != Interop.Kernel32.FileTypes.FILE_TYPE_PIPE);
+            Debug.Assert(Interop.Kernel32.GetFileType(handle) != Interop.Kernel32.FileTypes.FILE_TYPE_PIPE);
 
             byte* bytes = stackalloc byte[1];
             int numBytesReadWritten;
@@ -191,20 +203,25 @@ namespace System.IO
             // 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
+            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(_fileHandle, bytes, 0, out numBytesReadWritten, IntPtr.Zero);
+                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
+            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(_fileHandle, bytes, 0, out numBytesReadWritten, IntPtr.Zero);
+                r = Interop.Kernel32.WriteFile(handle, bytes, 0, out numBytesReadWritten, IntPtr.Zero);
             }
 
             if (r == 0)
             {
-                int errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(throwIfInvalidHandle: true);
-                if (errorCode == ERROR_INVALID_PARAMETER)
-                    throw new ArgumentException(SR.Arg_HandleNotSync, "handle");
+                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);
+                }
             }
         }
 
index 7db8518..a124220 100644 (file)
@@ -81,8 +81,33 @@ namespace System.IO
 
         [Obsolete("This constructor has been deprecated.  Please use new FileStream(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) instead, and optionally make a new SafeFileHandle with ownsHandle=false if needed.  http://go.microsoft.com/fwlink/?linkid=14202")]
         public FileStream(IntPtr handle, FileAccess access, bool ownsHandle, int bufferSize, bool isAsync)
-            : this(new SafeFileHandle(handle, ownsHandle), access, bufferSize, isAsync)
         {
+            SafeFileHandle safeHandle = new SafeFileHandle(handle, ownsHandle: ownsHandle);
+            try
+            {
+                ValidateAndInitFromHandle(safeHandle, access, bufferSize, isAsync);
+            }
+            catch
+            {
+                // We don't want to take ownership of closing passed in handles
+                // *unless* the constructor completes successfully.
+                GC.SuppressFinalize(safeHandle);
+
+                // This would also prevent Close from being called, but is unnecessary
+                // as we've removed the object from the finalizer queue.
+                //
+                // safeHandle.SetHandleAsInvalid();
+                throw;
+            }
+
+            // Note: Cleaner to set the following fields in ValidateAndInitFromHandle,
+            // but we can't as they're readonly.
+            _access = access;
+            _useAsyncIO = isAsync;
+
+            // As the handle was passed in, we must set the handle field at the very end to
+            // avoid the finalizer closing the handle when we throw errors.
+            _fileHandle = safeHandle;
         }
 
         public FileStream(SafeFileHandle handle, FileAccess access)
@@ -95,7 +120,7 @@ namespace System.IO
         {
         }
 
-        public FileStream(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync)
+        private void ValidateAndInitFromHandle(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync)
         {
             if (handle.IsInvalid)
                 throw new ArgumentException(SR.Arg_InvalidHandle, nameof(handle));
@@ -110,13 +135,24 @@ namespace System.IO
             if (handle.IsAsync.HasValue && isAsync != handle.IsAsync.Value)
                 throw new ArgumentException(SR.Arg_HandleNotAsync, nameof(handle));
 
-            _access = access;
-            _useAsyncIO = isAsync;
             _exposedHandle = true;
             _bufferLength = bufferSize;
-            _fileHandle = handle;
 
-            InitFromHandle(handle);
+            InitFromHandle(handle, access, isAsync);
+        }
+
+        public FileStream(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync)
+        {
+            ValidateAndInitFromHandle(handle, access, bufferSize, isAsync);
+
+            // Note: Cleaner to set the following fields in ValidateAndInitFromHandle,
+            // but we can't as they're readonly.
+            _access = access;
+            _useAsyncIO = isAsync;
+
+            // As the handle was passed in, we must set the handle field at the very end to
+            // avoid the finalizer closing the handle when we throw errors.
+            _fileHandle = handle;
         }
 
         public FileStream(string path, FileMode mode) :