Skip to content

Commit fe22fb0

Browse files
justinvpjustinvp
committed
Avoid the byte[] alloc in Win32FileStream.VerifyHandleIsSync
Call the native ReadFile and WriteFile interop functions directly, passing a stack allocated byte, instead of allocating a byte[] of length 1 on the heap. Added common error handling for ERROR_INVALID_HANDLE.
1 parent 36d10db commit fe22fb0

1 file changed

Lines changed: 55 additions & 77 deletions

File tree

‎src/System.IO.FileSystem/src/System/IO/Win32FileStream.cs‎

Lines changed: 55 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -332,44 +332,33 @@ private static Interop.mincore.SECURITY_ATTRIBUTES GetSecAttrs(FileShare share)
332332
[System.Security.SecuritySafeCritical] // auto-generated
333333
private unsafe void VerifyHandleIsSync()
334334
{
335+
Debug.Assert(!_isAsync);
336+
335337
// Do NOT use this method on pipes. Reading or writing to a pipe may
336338
// cause an app to block incorrectly, introducing a deadlock (depending
337339
// on whether a write will wake up an already-blocked thread or this
338340
// Win32FileStream's thread).
341+
Debug.Assert(Interop.mincore.GetFileType(_handle) != Interop.mincore.FileTypes.FILE_TYPE_PIPE);
339342

340-
// Do NOT change this to use a byte[] of length 0, or test test won't
341-
// work. Our ReadFile & WriteFile methods are special cased to return
342-
// for arrays of length 0, since we'd get an IndexOutOfRangeException
343-
// while using C#'s fixed syntax.
344-
byte[] bytes = new byte[1];
345-
int errorCode = 0;
346-
int r = 0;
343+
byte* bytes = stackalloc byte[1];
344+
int numBytesReadWritten;
345+
int r = -1;
347346

348347
// If the handle is a pipe, ReadFile will block until there
349348
// has been a write on the other end. We'll just have to deal with it,
350349
// For the read end of a pipe, you can mess up and
351350
// accidentally read synchronously from an async pipe.
352351
if (_canRead)
353-
{
354-
#if USE_OVERLAPPED
355-
r = ReadFileNative(_handle, bytes, 0, 0, null, out errorCode);
356-
#else
357-
r = ReadFileNative(_handle, bytes, 0, 0, out errorCode);
358-
#endif
359-
}
352+
r = Interop.mincore.ReadFile(_handle, bytes, 0, out numBytesReadWritten, IntPtr.Zero);
360353
else if (_canWrite)
354+
r = Interop.mincore.WriteFile(_handle, bytes, 0, out numBytesReadWritten, IntPtr.Zero);
355+
356+
if (r == 0)
361357
{
362-
#if USE_OVERLAPPED
363-
r = WriteFileNative(_handle, bytes, 0, 0, null, out errorCode);
364-
#else
365-
r = WriteFileNative(_handle, bytes, 0, 0, out errorCode);
366-
#endif
358+
int errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(throwIfInvalidHandle: true);
359+
if (errorCode == ERROR_INVALID_PARAMETER)
360+
throw new ArgumentException(SR.Arg_HandleNotSync, "handle");
367361
}
368-
369-
if (errorCode == ERROR_INVALID_PARAMETER)
370-
throw new ArgumentException(SR.Arg_HandleNotSync, "handle");
371-
if (errorCode == Interop.mincore.Errors.ERROR_INVALID_HANDLE)
372-
throw Win32Marshal.GetExceptionForWin32Error(errorCode, "<OS handle>");
373362
}
374363

375364

@@ -901,32 +890,11 @@ private long SeekCore(long offset, SeekOrigin origin)
901890
{
902891
Debug.Assert(!_handle.IsClosed && _canSeek, "!_handle.IsClosed && _parent.CanSeek");
903892
Debug.Assert(origin >= SeekOrigin.Begin && origin <= SeekOrigin.End, "origin>=SeekOrigin.Begin && origin<=SeekOrigin.End");
904-
int errorCode = 0;
905893
long ret = 0;
906894

907895
if (!Interop.mincore.SetFilePointerEx(_handle, offset, out ret, (uint)origin))
908896
{
909-
errorCode = Marshal.GetLastWin32Error();
910-
// #errorInvalidHandle
911-
// If ERROR_INVALID_HANDLE is returned, it doesn't suffice to set
912-
// the handle as invalid; the handle must also be closed.
913-
//
914-
// Marking the handle as invalid but not closing the handle
915-
// resulted in exceptions during finalization and locked column
916-
// values (due to invalid but unclosed handle) in SQL Win32FileStream
917-
// scenarios.
918-
//
919-
// A more mainstream scenario involves accessing a file on a
920-
// network share. ERROR_INVALID_HANDLE may occur because the network
921-
// connection was dropped and the server closed the handle. However,
922-
// the client side handle is still open and even valid for certain
923-
// operations.
924-
//
925-
// Note that _parent.Dispose doesn't throw so we don't need to special case.
926-
// SetHandleAsInvalid only sets _closed field to true (without
927-
// actually closing handle) so we don't need to call that as well.
928-
if (errorCode == Interop.mincore.Errors.ERROR_INVALID_HANDLE)
929-
_handle.Dispose();
897+
int errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid();
930898
throw Win32Marshal.GetExceptionForWin32Error(errorCode);
931899
}
932900

@@ -1587,26 +1555,14 @@ private unsafe int ReadFileNative(SafeFileHandle handle, byte[] bytes, int offse
15871555

15881556
if (r == 0)
15891557
{
1590-
errorCode = Marshal.GetLastWin32Error();
1591-
1592-
if (errorCode == ERROR_BROKEN_PIPE || errorCode == Interop.mincore.Errors.ERROR_PIPE_NOT_CONNECTED)
1593-
{
1594-
// This handle was a pipe, and it's done. Not an error, but EOF.
1595-
// However, the OS will not call AsyncFSCallback!
1596-
// Let the caller handle this, since ReadInternalCoreAsync & ReadCore
1597-
// need to do different things.
1598-
return -1;
1599-
}
1600-
1601-
// See code:#errorInvalidHandle in "private long SeekCore(long offset, SeekOrigin origin)".
1602-
if (errorCode == Interop.mincore.Errors.ERROR_INVALID_HANDLE)
1603-
_handle.Dispose();
1604-
1558+
errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid();
16051559
return -1;
16061560
}
16071561
else
1562+
{
16081563
errorCode = 0;
1609-
return numBytesRead;
1564+
return numBytesRead;
1565+
}
16101566
}
16111567

16121568
[System.Security.SecurityCritical] // auto-generated
@@ -1655,25 +1611,47 @@ private unsafe int WriteFileNative(SafeFileHandle handle, byte[] bytes, int offs
16551611

16561612
if (r == 0)
16571613
{
1658-
errorCode = Marshal.GetLastWin32Error();
1659-
1660-
if (errorCode == ERROR_NO_DATA)
1661-
{
1662-
// This handle was a pipe, and the pipe is being closed on the
1663-
// other side. Let the caller handle this, since Write
1664-
// and WriteAsync need to do different things.
1665-
return -1;
1666-
}
1667-
1668-
// See code:#errorInvalidHandle in "private long SeekCore(long offset, SeekOrigin origin)".
1669-
if (errorCode == Interop.mincore.Errors.ERROR_INVALID_HANDLE)
1670-
_handle.Dispose();
1671-
1614+
errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid();
16721615
return -1;
16731616
}
16741617
else
1618+
{
16751619
errorCode = 0;
1676-
return numBytesWritten;
1620+
return numBytesWritten;
1621+
}
1622+
}
1623+
1624+
[System.Security.SecurityCritical]
1625+
private int GetLastWin32ErrorAndDisposeHandleIfInvalid(bool throwIfInvalidHandle = false)
1626+
{
1627+
int errorCode = Marshal.GetLastWin32Error();
1628+
1629+
// If ERROR_INVALID_HANDLE is returned, it doesn't suffice to set
1630+
// the handle as invalid; the handle must also be closed.
1631+
//
1632+
// Marking the handle as invalid but not closing the handle
1633+
// resulted in exceptions during finalization and locked column
1634+
// values (due to invalid but unclosed handle) in SQL Win32FileStream
1635+
// scenarios.
1636+
//
1637+
// A more mainstream scenario involves accessing a file on a
1638+
// network share. ERROR_INVALID_HANDLE may occur because the network
1639+
// connection was dropped and the server closed the handle. However,
1640+
// the client side handle is still open and even valid for certain
1641+
// operations.
1642+
//
1643+
// Note that _parent.Dispose doesn't throw so we don't need to special case.
1644+
// SetHandleAsInvalid only sets _closed field to true (without
1645+
// actually closing handle) so we don't need to call that as well.
1646+
if (errorCode == Interop.mincore.Errors.ERROR_INVALID_HANDLE)
1647+
{
1648+
_handle.Dispose();
1649+
1650+
if (throwIfInvalidHandle)
1651+
throw Win32Marshal.GetExceptionForWin32Error(errorCode);
1652+
}
1653+
1654+
return errorCode;
16771655
}
16781656

16791657
[System.Security.SecuritySafeCritical]

0 commit comments

Comments
 (0)