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

ParseNetworkAddress might select IPv6 addresses over IPv4 when a hostname has both #722

Closed
jarmar opened this issue Nov 13, 2020 · 9 comments · Fixed by #740
Closed

ParseNetworkAddress might select IPv6 addresses over IPv4 when a hostname has both #722

jarmar opened this issue Nov 13, 2020 · 9 comments · Fixed by #740

Comments

@jarmar
Copy link
Contributor

jarmar commented Nov 13, 2020

Hi, thanks a lot for Jamulus. I'm trying to take a look at the feasibility of adding IPv6 support, and I believe I found a bug along the way.

If a hostname has both A and AAAA records, NetworkUtil::ParseNetworkAddress() might pick the address from the AAAA record instead of the one from the A record, and fail to connect to a server running on that host.

When given a hostname, NetworkUtil::ParseNetworkAddress() uses QHostInfo::fromName() to perform the lookup, and currently picks the first result from the list. The documentation of this method does not specify anything about the order of the list for cases where there are multiple addresses. Looking a few layers down in the implementation, it calls getaddrinfo(), whose behaviour seems to be system-dependent (see e.g. https://stackoverflow.com/a/11326970).

On my computer, getaddrinfo() returns IPv6 addresses before IPv4 addresses (when both are available). So when I try to connect to a hostname that has both A and AAAA records, Jamulus selects the IPv6 address, and fails to connect to the server running on that host.

(curiously, what happens when an IPv6 CHostAddress makes its way down to CSocket::SendPacket() is that toIPv4Address() fails and returns 0, which is interpreted as 0.0.0.0, so it tries to connect to localhost)

My suggestion is to make NetworkUtil::ParseNetworkAddress() look for the first IPv4 address in the list of returned addresses. The question is what to do if the returned result is non-empty, but only contains IPv6 addresses. Even though the rest of the function has some support for IPv6, I think it would make the most sense to return false when this happens. Right now, there's nothing useful that can be done with an IPv6 address anyway. If that sounds good, I could produce a pull request.

@corrados
Copy link
Contributor

Thanks for your investigation. So your change would be only be in function NetworkUtil::ParseNetworkAddress(), right?

@ann0see
Copy link
Member

ann0see commented Nov 14, 2020

I also experienced this problem while wanting to connect to localhost. A long term fix should be full ipv6 support.

@jarmar
Copy link
Contributor Author

jarmar commented Nov 14, 2020

@corrados: Yes, I think changing that method should be enough. I tried it out and it seems to solve the issue (including connecting to localhost).

@ann0see: I agree. I think some things have happened since #46 was opened that might make adding IPv6 support a bit easier than it was then. I will try things out a bit more and hopefully come up with a proposal for how support could be added.

@ghost
Copy link

ghost commented Nov 15, 2020

Selecting ipv6 before ipv4 is a bug in linux distros that have been zealously promoting ipv6.
The solution is to modify /etc/gai.conf (file probably doesn't exist). Here is my home made gai.conf
precedence ::1/128 50 0
precedence ::/0 40 1
precedence 2002::/16 30 2
precedence ::/96 20 3
precedence ::ffff:0:0/96 100 4

@pljones
Copy link
Collaborator

pljones commented Nov 16, 2020

Calling a configuration choice -- it's config, there to enable that choice, explicitly -- a bug is really rather a political statement in itself, I feel.

Devices may not have IPv4 addresses at all, so no matter how you order the preference of selection, you'll still get an IPv6 address. Not currently likely but...

Jamulus should support IPv6, really - it does expand the range of problems users are likely to find of course. (i.e. the local IPv6 address gets selected but there's no routing at all for IPv6 being the most likely...)

@ghost
Copy link

ghost commented Nov 19, 2020

I would like ipv6 implemented correctly and I see an ipv6 problem already. ipv4 was made without consideration that it would be superseded, thus ipv4 applications were coded similarly to how Jamulus is (they break when gai.conf is not configured with regard to ipv4). Now, ipv6 applications will repeat this problem if it is not addressed. Therefore gai.conf should not break ipv4, And ipv6 should not break when ipv4 is given precedence in gai.conf. (ipv6 should be ipv4 aware... but ipv4 must remain ignorant of ipv6)

  • Note that my gai.conf above gives precedence to ipv4.

@pljones
Copy link
Collaborator

pljones commented Nov 19, 2020

OK, I see what you mean -- if you have both IPv4 and IPv6 addressing and if you have IPv4 applications and if you use gai.conf to set the precedence of addressing, then the IPv4 stack shouldn't be broken. Agreed.

@pljones
Copy link
Collaborator

pljones commented Nov 22, 2020

Do we need both #46 and this issue?

jarmar added a commit to jarmar/jamulus that referenced this issue Nov 22, 2020
This is especially important for hostnames that have both IPv4 and IPv6
addresses, as NetworkUtil::ParseNetworkAddress() previously might have
returned IPv6 addresses for such cases.

This solves connecting to localhost on IPv6-enabled systems.
@jarmar
Copy link
Contributor Author

jarmar commented Nov 22, 2020

I think it makes sense to have both. While they originated from the same issue (hosts with both v4 and v6 addresses), #46 quickly turned into a general IPv6 feature request, while this one regards a very specific bug when trying to use IPv4 on IPv6-enabled hosts.

Though I agree that it would be nicer to have just one IPv6 issue. I'm not 100% on how to follow the contribution guidelines here, but since the suggested fix is so small, I will go ahead and create a pull request for this one.

corrados added a commit that referenced this issue Nov 22, 2020
Avoid selecting IPv6 results from hostname lookup (#722)
@ann0see ann0see mentioned this issue Feb 18, 2021
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 a pull request may close this issue.

4 participants