Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

deal with dualmode sockets #37194

Merged
merged 1 commit into from
Apr 26, 2019
Merged

deal with dualmode sockets #37194

merged 1 commit into from
Apr 26, 2019

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Apr 25, 2019

This got broken by #30036.
We now use DualMode sockets and we can do IPv4 over IPv6 socket. That confuses logic to construct data connection.

With this change I tested both passive and active mode and I can get listing.

@wfurt wfurt requested a review from a team April 25, 2019 19:13
@wfurt wfurt self-assigned this Apr 25, 2019
@davidsh davidsh added this to the 3.0 milestone Apr 25, 2019
@wfurt
Copy link
Member Author

wfurt commented Apr 25, 2019

fixes #37175

@stephentoub
Copy link
Member

stephentoub commented Apr 25, 2019

This got broken by #30036.

This is just one piece of code that was apparently dependent on the behavior changed in #30036. Is it not likely that #30036 may have broken other things as well, including in code we don't control?

@davidsh
Copy link
Contributor

davidsh commented Apr 25, 2019

Is it not likely that #30036 may have broken other things as well, including in code we don't control?

It is possible. The original change #30036 can be considered a breaking change. I think it's "ok" for things like this to change in a major release of .NET Core, i.e. 3.0. But we should definitely document it in release notes and perhaps add more test coverage to other areas we suspect would be impacted.

@karelz

@wfurt
Copy link
Member Author

wfurt commented Apr 25, 2019

I don't know how to assess the risk but it is certainly possible. FTP was particularly vulnerable as it deeply cares about address and manipulates two sockets in coordinated way.
The exposure is only when no AddressFamily is specified. When AddressFamily is used we use one or other socket as specified.

@wfurt
Copy link
Member Author

wfurt commented Apr 25, 2019

note that original #29414 request seems reasonable. Better support for IPv6 and/or dual stack seems like a good thing.

@wfurt wfurt merged commit 4f781a4 into dotnet:master Apr 26, 2019
@wfurt wfurt deleted the v6ftp_37175 branch April 26, 2019 18:00
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants