fix ReceiveFrom with dual mode socket by wfurt · Pull Request #92086 · dotnet/runtime

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt · 2023-09-14T21:24:24Z

Fixes#92006

This is regression against 7.0
It seems like the old internal code handled cases when IPv4 EndPoint was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case fails now and we fail to set RemoteEndPoint leaving whatever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

The regression was introduced by PR #89841

ghost · 2023-09-14T21:24:43Z

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

Issue Details

fixes #92006
This is regression from #89841. It seems like the old internal code handled cases when IPv4 EP was passed to ReceiveFrom on dual mode socket and the received address is actually IPv4 mapped into IPv6. But that case failed now and we would fail to set RemoteEndPoint leaving what ever was there - in case of the provided repro creating loop or possibly sending message to wrong peer.

After thinking about it I see no reason why IPEndPoint.Create needs to really care about AddressFamily as long as it is IP. The documentation claims that it needs to be correct type but it does not talk about IPv4 vs IPv6. And we allocate new EndPoint so the actual value of the old one does not matter. It will also make it easier to consume new API from #88970 as we exposed SocketAddresses to consumers and they may need to eat with it.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Sockets`
Milestone:	9.0.0

stephentoub · 2023-09-14T21:32:25Z

+ throw new ArgumentException(SR.Format(SR.net_InvalidAddressFamily, socketAddress.Family.ToString(), GetType().FullName), nameof(socketAddress));
 }

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


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

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.

stephentoub · 2023-09-14T21:37:30Z

+ if (_remoteEndPoint!.AddressFamily == AddressFamily.InterNetworkV6 && _socketAddress!.Family == AddressFamily.InterNetwork)
 {
- _remoteEndPoint = _remoteEndPoint!.Create(_socketAddress);
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
 }


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

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.

stephentoub · 2023-09-14T21:38:20Z

+ {
+ _remoteEndPoint = new IPEndPoint(_socketAddress.GetIPAddress().MapToIPv6(), _socketAddress.GetPort());
+ }
+ else


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?

karelz · 2023-09-15T07:50:14Z

/backport to release/8.0

github-actions · 2023-09-15T07:50:28Z

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6195384920

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

wfurt added 2 commits September 14, 2023 13:59

fix ReceiveFrom with dual mode socket
4017cb2

test
d233774

wfurt added the area-System.Net.Sockets label Sep 14, 2023

wfurt added this to the 9.0.0 milestone Sep 14, 2023

wfurt requested review from antonfirsov, rzikm and stephentoub September 14, 2023 21:24

wfurt self-assigned this Sep 14, 2023

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/src/System/Net/IPEndPoint.cs Outdated

stephentoub reviewed Sep 14, 2023
View reviewed changes

Comment threadsrc/libraries/System.Net.Primitives/tests/FunctionalTests/IPEndPointTest.cs

stephentoub reviewed Sep 14, 2023
View reviewed changes

stephentoub approved these changes Sep 15, 2023
View reviewed changes

feedback
4661a6d

wfurt merged commit a6c64c3 into dotnet:mainSep 15, 2023

wfurt deleted the dualUdp branch September 15, 2023 04:40

github-actionsBot mentioned this pull request Sep 15, 2023
[release/8.0] fix ReceiveFrom with dual mode socket #92103
Merged

karelz mentioned this pull request Sep 16, 2023
Incorrect port from Socket.ReceiveFromAsync(SocketAsyncEventArgs) #92006
Closed

ghost locked as resolved and limited conversation to collaborators Oct 15, 2023

Uh oh!

Conversation

wfurt commented Sep 14, 2023• edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 14, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karelz commented Sep 15, 2023

Uh oh!

github-actionsBot commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

wfurt commented Sep 14, 2023• edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 14, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karelz commented Sep 15, 2023

Uh oh!

github-actionsBot commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

wfurt commented Sep 14, 2023• edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 14, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wfurt commented Sep 14, 2023•
edited by karelz
Loading

wfurt commented Sep 14, 2023•
edited by karelz
Loading

wfurt commented Sep 14, 2023•
edited by karelz
Loading

wfurt commented Sep 14, 2023•
edited by karelz
Loading