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.

Sign up for GitHub

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

fix ReceiveFrom with dual mode socket #92086

Merged
merged 3 commits into from
Sep 15, 2023
Merged

fix ReceiveFrom with dual mode socket #92086

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4017cb2
fix ReceiveFrom with dual mode socket
wfurt Sep 14, 2023
d233774
test
wfurt Sep 14, 2023
4661a6d
feedback
wfurt Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Primitives/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
<value>This property is not implemented by this class.</value>
</data>
<data name="net_InvalidAddressFamily" xml:space="preserve">
<value>The AddressFamily {0} is not valid for the {1} end point, use {2} instead.</value>
<value>The AddressFamily {0} is not valid for the {1} end point.</value>
</data>
<data name="net_InvalidSocketAddressSize" xml:space="preserve">
<value>The supplied {0} is an invalid size for the {1} end point.</value>
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,9 @@ public override EndPoint Create(SocketAddress socketAddress)
{
ArgumentNullException.ThrowIfNull(socketAddress);

if (socketAddress.Family != AddressFamily)
if (socketAddress.Family != AddressFamily.InterNetwork && socketAddress.Family != AddressFamily.InterNetworkV6)
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName, AddressFamily.ToString()), nameof(socketAddress));
throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
}

int minSize = AddressFamily == AddressFamily.InterNetworkV6 ? SocketAddress.IPv6AddressSize : SocketAddress.IPv4AddressSize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're no longer requiring that socketAddress.Family == AddressFamily, does this size check still matter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to but than we can possibly get exception when we access the underlying buffer and it is too small. So I feel the extra check provides better feedback.

Expand Down
16 changes: 14 additions & 2 deletions src/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,19 @@ public static void ToString_Invoke_ReturnsExpected(IPEndPoint endPoint, string e
Assert.Equal(expected, endPoint.ToString());
}

[Fact]
public static void Create_DifferentAF_Success()
{
SocketAddress sa = new SocketAddress(AddressFamily.InterNetwork, SocketAddress.GetMaximumAddressSize(AddressFamily.InterNetworkV6));
var ep = new IPEndPoint(IPAddress.IPv6Any, 0);
Assert.NotNull(ep.Create(sa));

sa = new SocketAddress(AddressFamily.InterNetworkV6);
ep = new IPEndPoint(IPAddress.Any, 0);

Assert.NotNull(ep.Create(sa));
}
wfurt marked this conversation as resolved.
Show resolved Hide resolved

public static IEnumerable<object[]> Serialize_TestData()
{
yield return new object[] { new IPAddress(2), 16 };
Expand Down Expand Up @@ -195,8 +208,7 @@ public static void Create_NullSocketAddress_ThrowsArgumentNullException()

public static IEnumerable<object[]> Create_InvalidAddressFamily_TestData()
{
yield return new object[] { new IPEndPoint(2, 500), new SocketAddress(Sockets.AddressFamily.InterNetworkV6) };
yield return new object[] { new IPEndPoint(IPAddress.Parse("192.169.0.9"), 500), new SocketAddress(Sockets.AddressFamily.InterNetworkV6) };
yield return new object[] { new IPEndPoint(2, 500), new SocketAddress(Sockets.AddressFamily.Unknown) };
yield return new object[] { new IPEndPoint(IPAddress.Parse("0:0:0:0:0:0:0:1"), 500), new SocketAddress(Sockets.AddressFamily.InterNetwork) };
}

Expand Down
17 changes: 12 additions & 5 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,13 +927,13 @@ internal void FinishOperationSyncSuccess(int bytesTransferred, SocketFlags flags
{
try
{
if (_remoteEndPoint!.AddressFamily == _socketAddress!.Family)
if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
{
_remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
_remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
}
Comment on lines +930 to 933
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something IPEndPoint.Create(SocketAddress) should be doing instead of doing it at this one call site?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is tricky - we have cases when the address is IPv4 and Create would just work - but we want IPv6 IPv4Mapped addresses for DualMode sockets. I guess mostly for compatibility reasons so this is handled as special case.

else if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress.Family == AddressFamily.InterNetwork)
else
{
_remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
_remoteEndPoint = _remoteEndPoint!.Create(_socketAddress!);
}
}
catch
Expand All @@ -949,7 +949,14 @@ internal void FinishOperationSyncSuccess(int bytesTransferred, SocketFlags flags
{
try
{
_remoteEndPoint = _remoteEndPoint!.Create(_socketAddress!);
if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
{
_remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
}
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here: if this logic were moved into IPEndPoint.Create, would that a) address the duplication between these two call sites and b) potentially avoid bugs elsewhere? Or would that introduce problems?

{
_remoteEndPoint = _remoteEndPoint!.Create(_socketAddress!);
}
}
catch
{
Expand Down
46 changes: 46 additions & 0 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/ReceiveFrom.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,52 @@ public async Task ReceiveSent_UDP_Success(bool ipv4)
}
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public async Task ReceiveSent_DualMode_Success(bool ipv4)
{
const int Offset = 10;
const int DatagramSize = 256;
const int DatagramsToSend = 16;

IPAddress address = ipv4 ? IPAddress.Loopback : IPAddress.IPv6Loopback;
using Socket receiver = new Socket(SocketType.Dgram, ProtocolType.Udp);
using Socket sender = new Socket(SocketType.Dgram, ProtocolType.Udp);
if (receiver.DualMode != true || sender.DualMode != true)
{
throw new SkipException("DualMode not available");
}

ConfigureNonBlocking(sender);
ConfigureNonBlocking(receiver);

receiver.BindToAnonymousPort(address);
sender.BindToAnonymousPort(address);

byte[] sendBuffer = new byte[DatagramSize];
var receiveInternalBuffer = new byte[DatagramSize + Offset];
var emptyBuffer = new byte[Offset];
ArraySegment<byte> receiveBuffer = new ArraySegment<byte>(receiveInternalBuffer, Offset, DatagramSize);

Random rnd = new Random(0);

for (int i = 0; i < DatagramsToSend; i++)
{
rnd.NextBytes(sendBuffer);
sender.SendTo(sendBuffer, receiver.LocalEndPoint);

IPEndPoint remoteEp = new IPEndPoint(ipv4 ? IPAddress.Any : IPAddress.IPv6Any, 0);

SocketReceiveFromResult result = await ReceiveFromAsync(receiver, receiveBuffer, remoteEp);

Assert.Equal(DatagramSize, result.ReceivedBytes);
AssertExtensions.SequenceEqual(emptyBuffer, new ReadOnlySpan<byte>(receiveInternalBuffer, 0, Offset));
AssertExtensions.SequenceEqual(sendBuffer, new ReadOnlySpan<byte>(receiveInternalBuffer, Offset, DatagramSize));
Assert.Equal(sender.LocalEndPoint, result.RemoteEndPoint);
}
}

[Theory]
[InlineData(false)]
[InlineData(true)]
Expand Down