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

tcp: Set IPV6_V6ONLY for IPv6 listeners. #1555

Merged
merged 3 commits into from Apr 24, 2020
Merged

Conversation

twittner
Copy link
Contributor

@twittner twittner commented Apr 23, 2020

The current behaviour of listening on an IPv6 address varies depending on the operating system's IP address stack implementation. Some support IPv4-mapped IPv6 addresses (e.g. Linux and newer versions of Windows) so a single IPv6 address would support IPv4-mapped addresses too. Others do not (e.g. OpenBSD). If they do, then some support them by default (e.g. Linux) and some do not (e.g. Windows).

This PR attempts to implement the same behaviour accross operating systems. The strategy is as follows:

Disable IPv4-mapped IPv6 addresses, hence the socket option IPV6_V6ONLY is always set to true.

This allows binding two sockets to the same port and also avoids the problem of comparing mixed addresses which leads to issues such as #1552.

The current behaviour of listening on an IPv6 address varies depending
on the operating system's IP address stack implementation. Some support
IPv4-mapped IPv6 addresses (e.g. Linux and newer versions of Windows)
so a single IPv6 address would support IPv4-mapped addresses too.
Others do not (e.g. OpenBSD). If they do, then some support them by
default (e.g. Linux) and some do not (e.g. Windows).

This PR attempts to implement the same behaviour accross operating
systems. The strategy is as follows:

Disable IPv4-mapped IPv6 addresses, hence the socket option IPV6_V6ONLY
is always set to true.

This allows binding two sockets to the same port and also avoids the
problem of comparing mixed addresses which leads issues such as libp2p#1552.
Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Please also add an entry in CHANGELOG.md

socket.set_reuse_address(true)?;
}
socket.bind(&socket_addr.into())?;
socket.listen(1024)?; // we may want to make this configurable
Copy link
Member

Choose a reason for hiding this comment

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

The default in Rust seems to be 128
https://github.com/rust-lang/rust/blob/c06e4aca19046b07d952b16e9f002bfab38fde6b/src/libstd/sys/unix/ext/net.rs#L823

Suggested change
socket.listen(1024)?; // we may want to make this configurable
socket.listen(128)?; // we may want to make this configurable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following MIOs lead here as it is what tokio and async-std use.

s
};
if cfg!(target_family = "unix") {
socket.set_reuse_address(true)?;
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea of the implications of that change. What are the reasons for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know why it is restricted to Unix, but again this is what MIO does which in turn does what libstd does.

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved
transports/tcp/src/lib.rs Show resolved Hide resolved
s
};
if cfg!(target_family = "unix") {
socket.set_reuse_address(true)?;
Copy link
Member

Choose a reason for hiding this comment

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

Does that imply that on non-unix platforms one can not use dualstack nor two listeners (ipv4 and ipv6 on the same port)? (If so, not sure whether it would be important.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the particular platform. On Windows for example IPV6_V6ONLY seems to be true by default and use of SO_REUSEADDR is discouraged if I read this correctly, but frankly I do not know Windows and if someone has ideas how to configure things more optimally on this and other non-unix platforms we can make those changes in subsequent PRs.

CHANGELOG.md Show resolved Hide resolved
Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants