[PATCH 0/3] Fix virnetsocket failure in IPv4 disabled environment
This imperfection was reported by Andrea here: https://www.redhat.com/archives/libvir-list/2020-July/msg00753.html Michal Prívozník (3): virNetSocketCheckProtocols: Separate out checking family via getaddrinfo() virNetSocketCheckProtocols: lookup IPv6 only if suspecting IPv6 virNetSocketCheckProtocols: Confirm IPv4 by lookup too src/rpc/virnetsocket.c | 67 +++++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 24 deletions(-) -- 2.26.2
The virNetSocketCheckProtocols() function is supposed to tell caller whether IPv4 and/or IPv6 is supported on the system. In the initial round, it uses getifaddrs() to see if an interface has IPv4/IPv6 address assigned and then to double check IPv6 it uses getaddrinfo() to lookup IPv6 loopback address. Separate out this latter code because it is going to be reused. Since the original code lived under an #ifdef and the new function doesn't it is marked as unused - because on some systems it may be so. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 63 ++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index d1f4c531aa..b6bc3edc3b 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -141,16 +141,48 @@ static int virNetSocketForkDaemon(const char *binary) } #endif + +static int G_GNUC_UNUSED +virNetSocketCheckProtocolByLookup(const char *address, + int family, + bool *hasFamily) +{ + struct addrinfo hints; + struct addrinfo *ai = NULL; + int gaierr; + + memset(&hints, 0, sizeof(hints)); + hints.ai_family = family; + hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; + hints.ai_socktype = SOCK_STREAM; + + if ((gaierr = getaddrinfo(address, NULL, &hints, &ai)) != 0) { + *hasFamily = false; + + if (gaierr == EAI_FAMILY || +#ifdef EAI_ADDRFAMILY + gaierr == EAI_ADDRFAMILY || +#endif + gaierr == EAI_NONAME) { + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot resolve ::1 address: %s"), + gai_strerror(gaierr)); + return -1; + } + } else { + *hasFamily = true; + } + + freeaddrinfo(ai); + return 0; +} + int virNetSocketCheckProtocols(bool *hasIPv4, bool *hasIPv6) { #ifdef HAVE_IFADDRS_H struct ifaddrs *ifaddr = NULL, *ifa; - struct addrinfo hints; - struct addrinfo *ai = NULL; - int gaierr; - - memset(&hints, 0, sizeof(hints)); *hasIPv4 = *hasIPv6 = false; @@ -172,26 +204,9 @@ int virNetSocketCheckProtocols(bool *hasIPv4, freeifaddrs(ifaddr); - hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; - hints.ai_family = AF_INET6; - hints.ai_socktype = SOCK_STREAM; - if ((gaierr = getaddrinfo("::1", NULL, &hints, &ai)) != 0) { - if (gaierr == EAI_FAMILY || -# ifdef EAI_ADDRFAMILY - gaierr == EAI_ADDRFAMILY || -# endif - gaierr == EAI_NONAME) { - *hasIPv6 = false; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot resolve ::1 address: %s"), - gai_strerror(gaierr)); - return -1; - } - } - - freeaddrinfo(ai); + if (virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0) + return -1; VIR_DEBUG("Protocols: v4 %d v6 %d", *hasIPv4, *hasIPv6); -- 2.26.2
On a Wednesday in 2020, Michal Privoznik wrote:
The virNetSocketCheckProtocols() function is supposed to tell caller whether IPv4 and/or IPv6 is supported on the system. In the initial round, it uses getifaddrs() to see if an interface has IPv4/IPv6 address assigned and then to double check IPv6 it uses getaddrinfo() to lookup IPv6 loopback address. Separate out this latter code because it is going to be reused.
Since the original code lived under an #ifdef and the new function doesn't it is marked as unused - because on some systems it may be so.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 63 ++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 24 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index d1f4c531aa..b6bc3edc3b 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -141,16 +141,48 @@ static int virNetSocketForkDaemon(const char *binary) } #endif
+ +static int G_GNUC_UNUSED +virNetSocketCheckProtocolByLookup(const char *address, + int family, + bool *hasFamily) +{ + struct addrinfo hints; + struct addrinfo *ai = NULL; + int gaierr; + + memset(&hints, 0, sizeof(hints)); + hints.ai_family = family; + hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; + hints.ai_socktype = SOCK_STREAM; + + if ((gaierr = getaddrinfo(address, NULL, &hints, &ai)) != 0) { + *hasFamily = false; + + if (gaierr == EAI_FAMILY || +#ifdef EAI_ADDRFAMILY + gaierr == EAI_ADDRFAMILY || +#endif + gaierr == EAI_NONAME) { + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot resolve ::1 address: %s"),
s/::1/'%s'/
+ gai_strerror(gaierr)); + return -1; + } + } else { + *hasFamily = true; + }
Jano
There is not much sense trying to disprove host is IPv6 capable if we know after first round (getifaddrs()) that is is not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index b6bc3edc3b..b0d63f0f2c 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -205,7 +205,8 @@ int virNetSocketCheckProtocols(bool *hasIPv4, freeifaddrs(ifaddr); - if (virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0) + if (hasIPv6 && + virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0) return -1; VIR_DEBUG("Protocols: v4 %d v6 %d", *hasIPv4, *hasIPv6); -- 2.26.2
On 7/15/20 7:48 AM, Michal Privoznik wrote:
There is not much sense trying to disprove host is IPv6 capable if we know after first round (getifaddrs()) that is is not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index b6bc3edc3b..b0d63f0f2c 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -205,7 +205,8 @@ int virNetSocketCheckProtocols(bool *hasIPv4, freeifaddrs(ifaddr);
- if (virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0) + if (hasIPv6 &&
For this and the subsequent hasIPv4 patch, Coverity complains that checking "if (hasIPv6)" is a REVERSE_INULL issue since a few lines above we have "*hasIPv4 = *hasIPv6 = false;" or individual setting to true. This then should be "if (!*hasIPv6..." and of course the same for hasIPv4. /me wonders what kind of slight optimization exists or anyone cares about in the ifaddr loop once both has* bool ptrs are proven true - it's not like there's a set to false... John
+ virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0) return -1;
VIR_DEBUG("Protocols: v4 %d v6 %d", *hasIPv4, *hasIPv6);
On 7/17/20 12:33 PM, John Ferlan wrote:
On 7/15/20 7:48 AM, Michal Privoznik wrote:
There is not much sense trying to disprove host is IPv6 capable if we know after first round (getifaddrs()) that is is not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index b6bc3edc3b..b0d63f0f2c 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -205,7 +205,8 @@ int virNetSocketCheckProtocols(bool *hasIPv4, freeifaddrs(ifaddr);
- if (virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0) + if (hasIPv6 &&
For this and the subsequent hasIPv4 patch, Coverity complains that checking "if (hasIPv6)" is a REVERSE_INULL issue since a few lines above we have "*hasIPv4 = *hasIPv6 = false;" or individual setting to true.
This then should be "if (!*hasIPv6..." and of course the same for hasIPv4.
/me wonders what kind of slight optimization exists or anyone cares about in the ifaddr loop once both has* bool ptrs are proven true - it's not like there's a set to false...
D'oh! It should have been: if (*hasIPv6 && ..); because we are interested in the value of boolean, not its address. I'll post a patch shortly. Michal
Historically, if we found an interface with an IPv6 address we did not blindly trust that host is IPv6 capable (as in we can successfully translate IPv4 addresses) but used getaddrinfo() to confirm it. Turns out, we have use the same argument for IPv4. For instance, in an namespace created by the following steps, getaddrinfo("127.0.0.1", ...) fails (demonstrating by "Socket TCP/IPv4 Accept" test case failing in virnetsockettest): unshare -n ip link set lo up ip link add dummy0 type dummy ip link set dummy0 up Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index b0d63f0f2c..c62c2fb3fc 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -204,6 +204,9 @@ int virNetSocketCheckProtocols(bool *hasIPv4, freeifaddrs(ifaddr); + if (hasIPv4 && + virNetSocketCheckProtocolByLookup("127.0.0.1", AF_INET, hasIPv4) < 0) + return -1; if (hasIPv6 && virNetSocketCheckProtocolByLookup("::1", AF_INET6, hasIPv6) < 0) -- 2.26.2
On a Wednesday in 2020, Michal Privoznik wrote:
This imperfection was reported by Andrea here:
https://www.redhat.com/archives/libvir-list/2020-July/msg00753.html
Michal Prívozník (3): virNetSocketCheckProtocols: Separate out checking family via getaddrinfo() virNetSocketCheckProtocols: lookup IPv6 only if suspecting IPv6 virNetSocketCheckProtocols: Confirm IPv4 by lookup too
src/rpc/virnetsocket.c | 67 +++++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
John Ferlan -
Ján Tomko -
Michal Privoznik -
Michal Prívozník