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

Force AF_INET6 when binding to a NULL address #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sulix
Copy link

@sulix sulix commented Oct 9, 2023

Note: This probably needs a fallback to support IPv4-only systems before merging. I've also only tested it on Linux, on exactly one network so far. So it could easily cause as many problems as it solves.

Currently, to listen on 'all' addresses, NULL is used as the SDLNet_Address pointer. This results in NULL being passed to getaddrinfo() in MakeAddrInfoWithPort(), as well as the hint AI_PASSIVE and address family AF_UNSPEC being used.

It's the latter which can cause problems: on a dual-stack IPv4/IPv6 setup, AF_UNSPEC can resolve to AF_INET, resulting in the socket only binding to IPv4 addresses. This means that any attempt to connect to such a socket using the destination's IPv6 address (which may be the only address returned by SDLNet_ResolveHostname()) will fail.

There are two possible solutions here:

  • Create several sockets, one for each available address family, and bind each to their associated NULL address. (See [Feature Request] Compound Addresses (and sockets?) #84 for a proposal there)
  • Force the family to AF_INET6, and enable v4 address mapping, which will allow IPv4 addresses to be treated as IPv6 addresses of the form ::ffff:<address>. Also set the AI_V4MAPPED hint, which is enabled by default on Linux, but may not be everywhere.

This implements the latter: it has the advantage of being much simpler (only one socket is required), but the disadvantages of not working as-is on IPv4-only systems (which are rarer, but still exist), and not supporting non-IP protocols at all (I assume we're not planning to support IPX).

With this patch, the example programs all now function properly on my network (without it, I cannot use hostnames — even localhost — with voipchat, only explicit IPv4 addresses).

There are still some other potential pitfalls: SDLNet_GetLocalAddresses() still lists IPv4 addresses first, so if users bind explicitly to that, rather than use NULL, they'll be in trouble. Of course, it also returns loopback addresses first, so they'd be in trouble anyway…

Currently, to listen on 'all' addresses, NULL is used as the
SDLNet_Address pointer. This results in NULL being passed to
getaddrinfo() in MakeAddrInfoWithPort(), as well as the hint AI_PASSIVE
and address family AF_UNSPEC being used.

It's the latter which can cause problems: on a dual-stack IPv4/IPv6
setup, AF_UNSPEC can resolve to AF_INET, resulting in the socket only
binding to IPv4 addresses. This means that any attempt to connect to
such a socket using the destination's IPv6 address (which may be the
only address returned by SDLNet_ResolveHostname()) will fail.

There are two possible solutions here:
- Create several sockets, one for each available address family, and
  bind each to their associated NULL address.
- Force the family to AF_INET6, and enable v4 address mapping, which
  will allow IPv4 addresses to be treated as IPv6 addresses of the form
  ::ffff:<address>

This implements the latter: it has the advantage of being much simpler
(only one socket is required), but the disadvantages of not working
as-is on IPv4-only systems (which are rarer, but still exist), and not
supporting non-IP protocols at all (I assume we're not planning to
support IPX).
@JoelLinn
Copy link

There needs to be a fix for this. Either one described above or an additional parameter in SDLNet_CreateServer (IPv4, IPv6, default/whatever). Currently on some systems, SDLNet_ResolveHostname will resolve localhost to ipv6 and two simple SDLNet applications will not be able to communicate locally (unless setting 127.0.0.1 in the client of course)

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.

2 participants