Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Commit

Permalink
Fix Sockets hang caused by concurrent Socket disposal (#29786) (#29846)
Browse files Browse the repository at this point in the history 
* Fix Sockets hang caused by concurrent Socket disposal

On Windows, if Socket.Send/ReceiveAsync is used, and there's a race condition where the Socket is disposed of between the time that the Socket checks for disposal and attempts to get a NativeOverlapped for use with the operation, the getting of the NativeOverlapped can throw an exception and cause us to leave a field in an inconsistent state, which in turn can cause the operation to hang, waiting for that state to be reset.

The fix is to correct the order in which we get the NativeOverlapped vs setting the relevant field.

This is a regression in 2.1.

* Change ArgumentException to ObjectDisposedException from concurrent disposal

If Socket is disposed of concurrently with the first operation on it that allocates a native overlapped, if the disposal happens between the check for disposal and the call to BindHandle, BindHandle may end up throwing an ArgumentException, which is not expected out of SendAsync.  We should instead throw an ObjectDisposedException.

* Add test for concurrent Send/ReceiveAsync and Dispose on Socket
  • Loading branch information
@stephentoub @danmoseley
stephentoub authored and danmoseley committed May 30, 2018
1 parent ed23f53 commit 7ce9270
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 40 deletions.
8 changes: 8 additions & 0 deletions src/Common/src/System/Net/SafeCloseSocket.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,15 @@ public ThreadPoolBoundHandle GetOrAllocateThreadPoolBoundHandle(bool trySkipComp
}
catch (Exception exception) when (!ExceptionCheck.IsFatal(exception))
{
bool closed = IsClosed;
CloseAsIs();
if (closed)
{
// If the handle was closed just before the call to BindHandle,
// we could end up getting an ArgumentException, which we should
// instead propagate as an ObjectDisposedException.
throw new ObjectDisposedException(typeof(Socket).FullName, exception);
}
throw;
}

Expand Down
50 changes: 25 additions & 25 deletions src/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ internal unsafe SocketError DoOperationAccept(Socket socket, SafeCloseSocket han
}
catch
{
_singleBufferHandleState = SingleBufferHandleState.None;
FreeNativeOverlapped(overlapped);
_singleBufferHandle.Dispose();
_singleBufferHandleState = SingleBufferHandleState.None;
throw;
}
}
Expand Down Expand Up @@ -261,9 +261,9 @@ internal unsafe SocketError DoOperationConnect(Socket socket, SafeCloseSocket ha
}
catch
{
_singleBufferHandleState = SingleBufferHandleState.None;
FreeNativeOverlapped(overlapped);
_singleBufferHandle.Dispose();
_singleBufferHandleState = SingleBufferHandleState.None;
throw;
}
}
Expand Down Expand Up @@ -296,13 +296,13 @@ internal unsafe SocketError DoOperationReceiveSingleBuffer(SafeCloseSocket handl
{
fixed (byte* bufferPtr = &MemoryMarshal.GetReference(_buffer.Span))
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None, $"Expected None, got {_singleBufferHandleState}");
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

NativeOverlapped* overlapped = AllocateNativeOverlapped();
try
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None, $"Expected None, got {_singleBufferHandleState}");
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

SocketFlags flags = _socketFlags;
SocketError socketError = Interop.Winsock.WSARecv(
handle.DangerousGetHandle(), // to minimize chances of handle recycling from misuse, this should use DangerousAddRef/Release, but it adds too much overhead
Expand All @@ -318,8 +318,8 @@ internal unsafe SocketError DoOperationReceiveSingleBuffer(SafeCloseSocket handl
}
catch
{
FreeNativeOverlapped(overlapped);
_singleBufferHandleState = SingleBufferHandleState.None;
FreeNativeOverlapped(overlapped);
throw;
}
}
Expand Down Expand Up @@ -368,13 +368,13 @@ internal unsafe SocketError DoOperationReceiveFromSingleBuffer(SafeCloseSocket h
{
fixed (byte* bufferPtr = &MemoryMarshal.GetReference(_buffer.Span))
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None);
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

NativeOverlapped* overlapped = AllocateNativeOverlapped();
try
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None);
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

SocketFlags flags = _socketFlags;
SocketError socketError = Interop.Winsock.WSARecvFrom(
handle.DangerousGetHandle(), // to minimize chances of handle recycling from misuse, this should use DangerousAddRef/Release, but it adds too much overhead
Expand All @@ -392,8 +392,8 @@ internal unsafe SocketError DoOperationReceiveFromSingleBuffer(SafeCloseSocket h
}
catch
{
FreeNativeOverlapped(overlapped);
_singleBufferHandleState = SingleBufferHandleState.None;
FreeNativeOverlapped(overlapped);
throw;
}
}
Expand Down Expand Up @@ -549,9 +549,9 @@ internal unsafe SocketError DoOperationReceiveMessageFrom(Socket socket, SafeClo
}
catch
{
_singleBufferHandleState = SingleBufferHandleState.None;
FreeNativeOverlapped(overlapped);
_singleBufferHandle.Dispose();
_singleBufferHandleState = SingleBufferHandleState.None;
throw;
}
}
Expand All @@ -564,13 +564,13 @@ internal unsafe SocketError DoOperationSendSingleBuffer(SafeCloseSocket handle)
{
fixed (byte* bufferPtr = &MemoryMarshal.GetReference(_buffer.Span))
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None);
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

NativeOverlapped* overlapped = AllocateNativeOverlapped();
try
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None);
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

SocketError socketError = Interop.Winsock.WSASend(
handle.DangerousGetHandle(), // to minimize chances of handle recycling from misuse, this should use DangerousAddRef/Release, but it adds too much overhead
ref wsaBuffer,
Expand All @@ -585,8 +585,8 @@ internal unsafe SocketError DoOperationSendSingleBuffer(SafeCloseSocket handle)
}
catch
{
FreeNativeOverlapped(overlapped);
_singleBufferHandleState = SingleBufferHandleState.None;
FreeNativeOverlapped(overlapped);
throw;
}
}
Expand Down Expand Up @@ -738,13 +738,13 @@ internal unsafe SocketError DoOperationSendToSingleBuffer(SafeCloseSocket handle
{
fixed (byte* bufferPtr = &MemoryMarshal.GetReference(_buffer.Span))
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None);
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

NativeOverlapped* overlapped = AllocateNativeOverlapped();
try
{
Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.None);
_singleBufferHandleState = SingleBufferHandleState.InProcess;
var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };

SocketError socketError = Interop.Winsock.WSASendTo(
handle.DangerousGetHandle(), // to minimize chances of handle recycling from misuse, this should use DangerousAddRef/Release, but it adds too much overhead
ref wsaBuffer,
Expand All @@ -761,8 +761,8 @@ internal unsafe SocketError DoOperationSendToSingleBuffer(SafeCloseSocket handle
}
catch
{
FreeNativeOverlapped(overlapped);
_singleBufferHandleState = SingleBufferHandleState.None;
FreeNativeOverlapped(overlapped);
throw;
}
}
Expand Down Expand Up @@ -916,8 +916,8 @@ private void FreePinHandles()

if (_singleBufferHandleState != SingleBufferHandleState.None)
{
_singleBufferHandle.Dispose();
_singleBufferHandleState = SingleBufferHandleState.None;
_singleBufferHandle.Dispose();
}

if (_multipleBufferGCHandles != null)
Expand Down Expand Up @@ -1142,8 +1142,8 @@ void CompleteCoreSpin() // separate out to help inline the fast path

if (_singleBufferHandleState == SingleBufferHandleState.Set)
{
_singleBufferHandle.Dispose();
_singleBufferHandleState = SingleBufferHandleState.None;
_singleBufferHandle.Dispose();
}
}
}
Expand Down
99 changes: 84 additions & 15 deletions src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,90 @@ public void SocketSendReceiveBufferSize_SetZero_ThrowsSocketException()
Assert.Equal(e.SocketErrorCode, SocketError.InvalidArgument);
}
}

[Fact]
public async Task SendAsync_ConcurrentDispose_SucceedsOrThrowsAppropriateException()
{
if (UsesSync) return;

for (int i = 0; i < 20; i++) // run multiple times to attempt to force various interleavings
{
(Socket client, Socket server) = CreateConnectedSocketPair();
using (client)
using (server)
using (var b = new Barrier(2))
{
Task dispose = Task.Factory.StartNew(() =>
{
b.SignalAndWait();
client.Dispose();
}, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);

Task send = Task.Factory.StartNew(() =>
{
b.SignalAndWait();
SendAsync(client, new ArraySegment<byte>(new byte[1])).GetAwaiter().GetResult();
}, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);

await dispose;
Exception error = await Record.ExceptionAsync(() => send);
if (error != null)
{
Assert.True(error is ObjectDisposedException || error is SocketException, error.ToString());
}
}
}
}

[Fact]
public async Task ReceiveAsync_ConcurrentDispose_SucceedsOrThrowsAppropriateException()
{
if (UsesSync) return;

for (int i = 0; i < 20; i++) // run multiple times to attempt to force various interleavings
{
(Socket client, Socket server) = CreateConnectedSocketPair();
using (client)
using (server)
using (var b = new Barrier(2))
{
Task dispose = Task.Factory.StartNew(() =>
{
b.SignalAndWait();
client.Dispose();
}, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);

Task send = Task.Factory.StartNew(() =>
{
SendAsync(server, new ArraySegment<byte>(new byte[1])).GetAwaiter().GetResult();
b.SignalAndWait();
ReceiveAsync(client, new ArraySegment<byte>(new byte[1])).GetAwaiter().GetResult();
}, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);

await dispose;
Exception error = await Record.ExceptionAsync(() => send);
if (error != null)
{
Assert.True(error is ObjectDisposedException || error is SocketException, error.ToString());
}
}
}
}

protected static (Socket, Socket) CreateConnectedSocketPair()
{
using (Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
listener.Listen(1);

Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
client.Connect(listener.LocalEndPoint);
Socket server = listener.Accept();

return (client, server);
}
}
}

public class SendReceive
Expand Down Expand Up @@ -1248,21 +1332,6 @@ select Task.Factory.StartNew(() => pair.Item1.Receive(new byte[1]), Cancellation
}
}).Dispose();
}

private static (Socket, Socket) CreateConnectedSocketPair()
{
using (Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
listener.Listen(1);

Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
client.Connect(listener.LocalEndPoint);
Socket server = listener.Accept();

return (client, server);
}
}
}

public sealed class SendReceiveSyncForceNonBlocking : SendReceive<SocketHelperSyncForceNonBlocking> { }
Expand Down

0 comments on commit 7ce9270

Please sign in to comment.