From: Jeremy Kuhne Date: Mon, 19 Jun 2017 22:31:01 +0000 (-0700) Subject: Don't close passed in FileStream handles in constructor (dotnet/coreclr#12253) X-Git-Tag: submit/tizen/20210909.063632~11030^2~6925^2~386 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d737c9b8f0e5cd4e6dad4a84c08d94b182ec9b6f;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Don't close passed in FileStream handles in constructor (dotnet/coreclr#12253) * 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 --- diff --git a/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Unix.cs b/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Unix.cs index 7d860ac..0fe3b95 100644 --- a/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Unix.cs +++ b/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Unix.cs @@ -105,15 +105,12 @@ namespace System.IO } /// Initializes a stream from an already open file handle (file descriptor). - /// The handle to the file. - /// The size of the buffer to use when buffering. - /// Whether access to the stream is performed asynchronously. - 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 } /// Gets a value indicating whether the current stream supports seeking. - public override bool CanSeek => CanSeekCore; + public override bool CanSeek => CanSeekCore(_fileHandle); /// Gets a value indicating whether the current stream supports seeking. - /// Separated out of CanSeek to enable making non-virtual call to this logic. - private bool CanSeekCore + /// + /// 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. + /// + 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() diff --git a/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Windows.cs b/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Windows.cs index c036ee6..6e7ba74 100644 --- a/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Windows.cs +++ b/src/coreclr/src/mscorlib/shared/System/IO/FileStream.Windows.cs @@ -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); + } } } diff --git a/src/coreclr/src/mscorlib/shared/System/IO/FileStream.cs b/src/coreclr/src/mscorlib/shared/System/IO/FileStream.cs index 7db8518..a124220 100644 --- a/src/coreclr/src/mscorlib/shared/System/IO/FileStream.cs +++ b/src/coreclr/src/mscorlib/shared/System/IO/FileStream.cs @@ -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) :