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

[8.0] Guard against empty Accept address #108334

Open
wants to merge 2 commits into
base: release/8.0-staging
Choose a base branch
from

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Sep 27, 2024

Backports #108616

Fixes #108026 and #102663

Customer Impact

  • Customer reported
  • Found internally

There is problem in 8.0+ when Socket.Accept can fail and throw exception on background thread on macOS, which leads to application termination. This was already reported by 2 customers who upgraded to 8.0 from previous LTS 6.0 - see #108026 and #102663.

Regression

  • Yes - from 7.0 to 8.0
  • No

Prior 8.0 we used internal duplicate of class SocketAddress that was apparently more forgiving in certain corner cases. We switch in 8.0 to ussage of public API that is more strict.

Testing

The fix was merged to main and verified by customer. Also 8.0 binaries were tested in their lab. See #108334 (comment) for details.

Risk

Low. The fix basically checks for error condition to avoid throwing. It seems like the condition is coming from Darwin kernel where sometime accept operation fails to provide remote address.

@wfurt wfurt self-assigned this Sep 27, 2024
Copy link
Contributor

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

@wfurt
Copy link
Member Author

wfurt commented Oct 7, 2024

/backport to main

Copy link
Contributor

github-actions bot commented Oct 7, 2024

Started backporting to main: https://github.com/dotnet/runtime/actions/runs/11221178118

@karelz karelz changed the title guard agains empty Accept address guard against empty Accept address Oct 10, 2024
@karelz karelz modified the milestones: 10.0.0, 8.0.x Oct 10, 2024
@karelz karelz changed the title guard against empty Accept address [8.0] Guard against empty Accept address Oct 10, 2024
@carlossanlop
Copy link
Member

Friendly reminder that today 10/14 is Code Complete for the November Release. If this fix is intended to be included in that release, please make sure it's merged before 4pm PT. Otherwise, it will have to wait until next month.

@pepone
Copy link
Contributor

pepone commented Nov 15, 2024

@wfurt I finally come around to test this. We have two tests in https://github.com/zeroc-ice/ice that where hitting this issue pretty often when run with IPv6 and both are working fine with this branch.

We are also hitting this issue with GitHub macOS runners, for a different project

https://github.com/icerpc/icerpc-csharp/actions/runs/11842033975/job/32999706878

Pretty annoying because it brings the test host process down:

The active test run was aborted. Reason: Test host process crashed : Unhandled exception. System.ArgumentException: The supplied System.Net.SocketAddress is an invalid size for the System.Net.IPEndPoint end point. (Parameter 'socketAddress')
   at System.Net.IPEndPoint.Create(SocketAddress socketAddress)

@wfurt
Copy link
Member Author

wfurt commented Nov 16, 2024

it seems like we have enough evidence to proceed with servicing request @karelz

@karelz karelz added the Servicing-consider Issue for next servicing release review label Dec 5, 2024
@karelz
Copy link
Member

karelz commented Dec 5, 2024

Regression in 8.0 (LTS) against 7.0. Impact on customers - crash, without a workaround.
We should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Sockets os-mac-os-x macOS aka OSX Servicing-consider Issue for next servicing release review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants