Avoid calling `getnameinfo` when formatting IPv6 addresses in `CNetAddr::ToStringIP`

practicalswift commented at 6:22 pm on April 22, 2021: contributor

Avoid calling getnameinfo when formatting IPv6 addresses in CNetAddr::ToStringIP.

Fixes #21466. Fixes #21967.

The IPv4 case was fixed in #21564.

net: Make IPv6ToString do zero compression as described in RFC 5952 c10f27fdb2

net: Avoid calling getnameinfo when formatting IPv6 addresses in CNetAddr::ToStringIP 54548bae80

DrahtBot added the label P2P on Apr 22, 2021

laanwj commented at 8:37 pm on April 22, 2021: member

Concept ACK

vasild approved

vasild commented at 10:08 am on May 3, 2021: member

ACK 54548bae8004a8f49d73bd29aeca8b41894214c4

Subawit approved

shobitb commented at 3:03 am on May 17, 2021: none

ACK c10f27fdb2d335954dd1017ce6d5800159427374

I’ve reviewed the code and it looks good. Also ran unit tests and make check completes successfully.

practicalswift commented at 8:27 am on May 17, 2021: contributor

@shobitb Thanks a lot for reviewing! I think you might have gotten the commit id wrong when ACK:ing. The last commit id in the sequence of commits is the one used when ACK:ing a PR assuming one ACK:s the entire PR and not just a subset of it :)

laanwj commented at 11:06 am on May 17, 2021: member

Code review ACK 54548bae8004a8f49d73bd29aeca8b41894214c4 It at first surprised me that there are no tests for IPv6ToString. However there are for the higher-level functions. As this passes the CNetAddr ToString tests, I think this is covered sufficiently.

laanwj merged this on May 17, 2021

laanwj closed this on May 17, 2021

in src/netaddress.cpp:586 in 54548bae80

614@@ -578,15 +615,6 @@ std::string CNetAddr::ToStringIP() const
615     case NET_IPV4:
616         return IPv4ToString(m_addr);
617     case NET_IPV6: {
618-        CService serv(*this, 0);
619-        struct sockaddr_storage sockaddr;
620-        socklen_t socklen = sizeof(sockaddr);
621-        if (serv.GetSockAddr((struct sockaddr*)&sockaddr, &socklen)) {
622-            char name[1025] = "";
623-            if (!getnameinfo((const struct sockaddr*)&sockaddr, socklen, name,

jonatack commented at 11:52 am on May 17, 2021:

Was removing link-local scoped id functionality an intended consequence of this PR?

practicalswift commented at 1:30 pm on May 17, 2021:

#21982 (comment)

laanwj commented at 2:41 pm on May 17, 2021:

If we want to restore that functionality we could pass m_scope_id to IPv6ToString and append it if non-zero.

jonatack commented at 6:29 am on May 18, 2021: member

According to #21982 (comment), no longer returning scoped ids was an intentional part of this PR. Maybe that wasn’t obvious to reviewers. After test coverage was removed by #21689, no other tests signalled the change.

It would be good to be more explicit in the PR description regarding why make the change, and straightforward about what support was being dropped.

vasild commented at 8:50 am on May 18, 2021: member

There seems to be two “functionalities” being discussed together, it took me some time to figure it out:

The functionality of IPv6 scope id, introduced in #7570. That is still working, has not been removed.
The printout of such IPv6 addresses - before we used the system getnameinfo() which included %zoneid (on all or most platforms). Now we roll our own which does not include %zoneid.

For reference, here is the FreeBSD implementation of getnameinfo() (which is under… eh… the BSD license):

https://github.com/freebsd/freebsd-src/blob/c232fd4b4191a722f8c3193ef1e7c6efd8385182/lib/libc/net/getnameinfo.c#L43-L46

https://github.com/freebsd/freebsd-src/blob/c232fd4b4191a722f8c3193ef1e7c6efd8385182/lib/libc/net/getnameinfo.c#L358-L375

https://github.com/freebsd/freebsd-src/blob/c232fd4b4191a722f8c3193ef1e7c6efd8385182/sys/netinet6/in6.h#L296-L297

practicalswift commented at 9:40 am on May 18, 2021: contributor

@vasild

Exactly. Thanks for bringing some sense to this.

What this entire discussion boils down to is if the expectation is that CNetAddr::ToStringIP() should print the IPv6 address or the IPv6 socket address:

IPv6 address (std::net::Ipv6Addr): “IPv6 addresses are defined as 128-bit integers in IETF RFC 4291. They are usually represented as eight 16-bit segments.”
IPv6 socket address (std::net::SocketAddrV6): “IPv6 socket addresses consist of an IPv6 address, a 16-bit port number, as well as fields containing the traffic class, the flow label, and a scope identifier (see IETF RFC 2553, Section 3.3 for more details).”

jonatack commented at 11:07 am on May 18, 2021: member

Indeed. What does not make sense to me is why this PR was not upfront about this, nor the reasons that were given to have the only test coverage removed willy-nilly allowing this to be changed without tests warning about it, nor why we should break user space to follow the rustlang version without discussion. If those questions do not make sense, then I apologize.

jonatack commented at 11:23 am on May 18, 2021: member

The comment I referred to above in #21982 (comment):

If you by “functionality” mean not appending an unwanted %zone_index string to CNetAddr::ToString(), then yes that non-appending of an unwanted string is obviously intentional. The new implementation of IPv6ToString (which is called from CNetAddr::ToString) follows Rust’s std::net::Ipv6Addr which does not append %zone_index. FWIW that is how the previous implementation of CNetAddr::ToString worked under macOS.

Yes, as the removed test and as #21690 showed, macOS returns a different getnameinfo format for scoped ids, without the zone id, from all the other platforms that do provide support for it. Why should we align with the macOS exception without discussing it? Does it not make sense to call attention to this change?

vasild commented at 12:06 pm on May 18, 2021: member

It should be easy to append %zoneid like @laanwj suggests?

jonatack commented at 12:23 pm on May 18, 2021: member

It should be easy to append %zoneid like @laanwj suggests?

#21985

gwillen referenced this in commit f934d41627 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

Avoid calling getnameinfo when formatting IPv6 addresses in CNetAddr::ToStringIP #21756

Avoid calling `getnameinfo` when formatting IPv6 addresses in `CNetAddr::ToStringIP` #21756