Fix lifetime handling of ReceiveMessageFromAsync buffer on Windows #22012
Conversation
The test currently only tests a single send/receive pair, which doesn't catch issues when trying to reuse a SocketAsyncEventArgs for multiple operations iteratively. The test is also duplicated for IPv4 and IPv6. Fix both issues.
When a ReceiveMessageFromAsync is first used with a SocketAsyncEventArgs instance, it initializes the _wsaMessageBuffer. Then in order to use the buffer, it pins it, storing both a GCHandle and a pointer to the target object. But if the SAEA's buffer is ever changed with, for example, a SetBuffer call, a routine is invoked on the SocketAsyncEventArgs that frees all of its pinned data, including these for the _wsaMessageBuffer. Then the next time ReceiveMessageFromAsync is used, this handle ends up not getting recreated, and we end up dereferencing a null pointer. The fix is to separate the creation of the buffer from the creation of the pinning handle, lazily initializing each independently. If the pinning handle is freed due to SetBuffer, the next invocation will reinitialize it.
| Debug.Assert( | ||
| !_wsaMessageBufferGCHandle.IsAllocated || | ||
| (_wsaMessageBufferGCHandle.Target == _wsaMessageBuffer && _wsaMessageBuffer != null)); | ||
| Debug.Assert((_ptrWSAMessageBuffer != IntPtr.Zero) == _wsaMessageBufferGCHandle.IsAllocated); |
There was a problem hiding this comment.
Seems like there should also be an assert that if _ptrWSAMessageBuffer is not null, then it's == Marshal.UnsafeAddrOfPinnedArrayElement(_wsaMessageBuffer, 0). That's correct, right?
BTW, at some point it would be nice to clean some of this stuff up, the pinning logic is unnecessarily complicated. A couple thoughts:
(1) Marshal.UnsafeAddrOfPinnedArrayElement(_wsaMessageBuffer, 0) can't be that expensive; rather than storing the raw ptr, we should just do this on demand.
(2) Even better, we could just allocate the WSAMessageBuffer (and WSABuffer also) on the stack when we do the actual native socket call. Again, I really doubt that the cost of doing this would matter at all.
Thoughts?
There was a problem hiding this comment.
That's correct, right?
Yes.
BTW, at some point it would be nice to clean some of this stuff up
Yes, I was thinking the same thing yesterday, in particular because it'd be nice to remove the extra IntPtr field(s) from SAEA. I'm not doing that in this PR, though.
we could just allocate the WSAMessageBuffer (and WSABuffer also) on the stack when we do the actual native socket call
For an async call?
| if (_wsaMessageBuffer == null) | ||
| { | ||
| _wsaMessageBuffer = new byte[sizeof(Interop.Winsock.WSAMsg)]; | ||
| } | ||
| if (_ptrWSAMessageBuffer == IntPtr.Zero) | ||
| { |
There was a problem hiding this comment.
Just for clarity, I think I'd structure the asserts like this -- let me know if I missed something here...
if (_wsaMessageBuffer == null)
{
Debug.Assert(!_wsaMessageBufferGCHandle.IsAllocated);
Debug.Assert(_ptrWSAMessageBuffer == IntPtr.Zero);
_wsaMessageBuffer = new byte[sizeof(Interop.Winsock.WSAMsg)];
}
if (_ptrWSAMessageBuffer == IntPtr.Zero)
{
Debug.Assert(!_wsaMessageBufferGCHandle.IsAllocated);
_wsaMessageBufferGCHandle = GCHandle.Alloc(_wsaMessageBuffer, GCHandleType.Pinned);
_ptrWSAMessageBuffer = Marshal.UnsafeAddrOfPinnedArrayElement(_wsaMessageBuffer, 0);
}
else
{
Debug.Assert(_wsaMessageBufferGCHandle.IsAllocated);
Debug.Assert(_ptrWSAMessageBuffer == Marshal.UnsafeAddrOfPinnedArrayElement(_wsaMessageBuffer, 0);
}
Thoughts?
|
Generally LGTM |
…e_fix Fix lifetime handling of ReceiveMessageFromAsync buffer on Windows Commit migrated from dotnet/corefx@5b767b4
When a ReceiveMessageFromAsync is first used with a SocketAsyncEventArgs instance, it initializes the _wsaMessageBuffer. Then in order to use the buffer, it pins it, storing both a GCHandle and a pointer to the target object. But if the SAEA's buffer is ever changed with, for example, a SetBuffer call, a routine is invoked on the SocketAsyncEventArgs that frees all of its pinned data, including these for the _wsaMessageBuffer. Then the next time ReceiveMessageFromAsync is used, this handle ends up not getting recreated, and we end up dereferencing a null pointer.
The essentially-one-line fix is to separate the creation of the buffer from the creation of the pinning handle, lazily initializing each independently. If the pinning handle is freed due to SetBuffer, the next invocation will reinitialize it.
I also consolidated and augmented the existing ReceiveMessageFromAsync tests to test multiple receives with the same SocketAsyncEventArgs in order to catch this case. The test now fails before the fix and passes after.
Contributes to https://github.com/dotnet/corefx/issues/21995 (hoping to port fix to 2.0 branch)
cc: @davidsh, @CIPop, @geoffkizer