Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Socket connect with payload via SocketAsyncEventArgs fails #79654

Closed
mgravell opened this issue Dec 14, 2022 · 9 comments · Fixed by #79669
Closed

Socket connect with payload via SocketAsyncEventArgs fails #79654

mgravell opened this issue Dec 14, 2022 · 9 comments · Fixed by #79669

Comments

@mgravell
Copy link
Member

mgravell commented Dec 14, 2022

Description

Appears to have been introduced here with the removal of the early pin /cc @stephentoub

WSAConnect supports connect+transmit, which is supported in SocketAsyncEventArgs, and implemented in DoOperationConnectEx - however, a critical bug stops it working when a buffer is specified, faulting with "The system detected an invalid pointer address in attempting to use a pointer argument in a call.".

This is because the code sends (IntPtr)((byte*)_singleBufferHandle.Pointer + _offset), as the payload pointer, but _singleBufferHandle is not initialized until after ConnectEx, in ProcessIOCPResult (to avoid pins in the synchronous case).

Proposed fix: pass bufferPtr instead.

This API should probably be passing bufferPtr here, which is the same address via fixed (fine for the synchronous case, and ends up pinned while fixed in the async case)

Reproduction Steps

Fails on net7.0

(note I haven't bothered with a server here; not needed for demonstration)

using var client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
var saea = new SocketAsyncEventArgs();
saea.RemoteEndPoint = new IPEndPoint(IPAddress.Loopback, 42);
saea.SetBuffer(new byte[42]); // <== the presence of the buffer is what triggers the failure

Console.WriteLine(client.ConnectAsync(saea)); // reports false, so: not pending
Console.WriteLine(saea.SocketError); // reports Fault

Expected behavior

Connects and transmits; specifically, ConnectAsync should return true (pending), or should report false (complete) with a suitable SocketError

Actual behavior

ConnectAsync reports false (complete) with SocketError as Fault

Regression?

Yes; works in net6.0

Known Workarounds

connect without a buffer, then send afterwards

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 14, 2022
@ghost
Copy link

ghost commented Dec 14, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Appears to have been introduced here /cc @stephentoub

WSAConnect supports connect+transmit, which is supported in SocketAsyncEventArgs, and implemented in DoOperationConnectEx - however, a critical bug stops it working when a buffer is specified, faulting with "The system detected an invalid pointer address in attempting to use a pointer argument in a call.".

This is because the code sends (IntPtr)((byte*)_singleBufferHandle.Pointer + _offset), as the payload pointer, but _singleBufferHandle is not initialized until after ConnectEx, in ProcessIOCPResult (to avoid pins in the synchronous case).

Proposed fix: pass bufferPtr instead.

This API should probably be passing bufferPtr here, which is the same address via fixed (fine for the synchronous case, and ends up pinned while fixed in the async case)

Reproduction Steps

Requires preview CLR?

(note I haven't bothered with a server here; not needed for demonstration)

using var client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
var saea = new SocketAsyncEventArgs();
saea.RemoteEndPoint = new IPEndPoint(IPAddress.Loopback, 42);
saea.SetBuffer(new byte[42]); // <== the presence of the buffer is what triggers the failure
// normally we would configure a callback, check the bool etc - not needed to demonstrate failure
client.ConnectAsync(saea); // BOOM!!!

Expected behavior

Connects and transmits

Actual behavior

Faults with invalid address

Regression?

Works in CLR7

Known Workarounds

connect without a buffer, then send afterwards

Configuration

No response

Other information

No response

Author: mgravell
Assignees: -
Labels:

area-System.Net.Sockets

Milestone: -

@wfurt
Copy link
Member

wfurt commented Dec 14, 2022

Is this essentially #1476? AFAIK that was never fully implemented and supported. But I assume the break was not intentional so we should look into it.

@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Dec 14, 2022
@wfurt wfurt added this to the 8.0.0 milestone Dec 14, 2022
@MihaZupan MihaZupan added the bug label Dec 14, 2022
@antonfirsov
Copy link
Member

antonfirsov commented Dec 14, 2022

@wfurt isn't TFO more than just "data in SYN"?

Standard TCP already allows data to be carried in SYN packets [RFC793], Section 3.4) but forbids the receiver from delivering it to the application until the 3WHS is completed.

I would assume ConnectEx allows sending data, but the 3WHS should be completed in order to receive the data, unless the TFO flag is set.

This is definitely platform-specific behavior though, on Unix the buffer is ignored in all versions.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 14, 2022
@mgravell
Copy link
Member Author

This is definitely platform-specific behavior though, on Unix the buffer is ignored in all versions.

"connects successfully without sending the buffer" would also be an acceptable outcome, if that is the documented and intended behaviour; it is odd that it is different between platforms; "fails in an obscure way if a buffer is set": not so much

@antonfirsov
Copy link
Member

antonfirsov commented Dec 14, 2022

I would merge #79669 to fix the 7.0 regression and keep the Windows behavior consistent across .NET versions, and open a separate issue to document the platform differences. In my understanding, cross-platform behavior is only possible with TFO (#1476).

@mgravell
Copy link
Member Author

@antonfirsov fortunately it is somewhat discoverable by checking the BytesTransferred upon completion, which arguably people should always be doing - I believe this will be zero on platforms where the connect did not include the buffer

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 20, 2022
@antonfirsov antonfirsov modified the milestones: 8.0.0, 7.0.x Dec 20, 2022
@antonfirsov
Copy link
Member

antonfirsov commented Dec 20, 2022

Reopening, to track backport, this is a regression.

@antonfirsov antonfirsov reopened this Dec 20, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 20, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 5, 2023
@antonfirsov
Copy link
Member

Backport PR #79866 has been merged.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 12, 2023
@karelz
Copy link
Member

karelz commented Mar 22, 2023

Fixed in main (8.0) in PR #79669 and in 7.0.3 in PR #79866.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants