[libvirt PATCH 1/4] util: refactor code to workaround gcc 10.1.0 bug

[Laine Stump]

On 7/22/20 1:21 PM, Daniel P. Berrangé wrote:
> gcc 10.1.0 on Debian sid has a bug where the bounds checking gets
> confused beteen two branches:
>
> In file included from /usr/include/string.h:495,
>                   from ../../src/internal.h:28,
>                   from ../../src/util/virsocket.h:21,
>                   from ../../src/util/virsocketaddr.h:21,
>                   from ../../src/util/virnetdevip.h:21,
>                   from ../../src/util/virnetdevip.c:21:
> In function 'memcpy',
>      inlined from 'virNetDevGetifaddrsAddress' at ../../src/util/virnetdevip.c:914:13,
>      inlined from 'virNetDevIPAddrGet' at ../../src/util/virnetdevip.c:962:16:
> /usr/include/arm-linux-gnueabihf/bits/string_fortified.h:34:10: error: '__builtin_memcpy' offset [16, 27] from the object at 'addr' is out of the bounds of referenced subobject 'inet4' with type 'struct sockaddr_in' at offset 0 [-Werror=array-bounds]
>     34 |   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
>        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../../src/util/virnetdevip.h:21,
>                   from ../../src/util/virnetdevip.c:21:
> ../../src/util/virnetdevip.c: In function 'virNetDevIPAddrGet':
> ../../src/util/virsocketaddr.h:29:28: note: subobject 'inet4' declared here
>     29 |         struct sockaddr_in inet4;
>        |                            ^~~~~
> cc1: all warnings being treated as errors
>
> Note the source location is pointing to the "inet6" / AF_INET6 branch of
> the "if", but is complaining about bounds of the "inet4" field. Changing
> the code into a switch() is sufficient to avoid triggering the bug and
> is arguably better code too.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>

(Huh, I thought I sent an ack for this when you posted it the first time 
this morning, but I guess it failed to send, so it was still sitting in 
an obscured window on my screen...)


I don't have a system running gcc 10 yet, but this code looks to provide 
identical functionality, and still works with gcc 9.3.1


Reviewed-by: Laine Stump <laine at redhat.com>


> ---
>   src/util/virnetdevip.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index ba9e567e5a..8b85c7beca 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -897,26 +897,25 @@ virNetDevGetifaddrsAddress(const char *ifname,
>       }
>   
>       for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> -        int family;
> -
>           if (STRNEQ_NULLABLE(ifa->ifa_name, ifname))
>               continue;
>   
>           if (!ifa->ifa_addr)
>               continue;
> -        family = ifa->ifa_addr->sa_family;
> -
> -        if (family != AF_INET6 && family != AF_INET)
> -            continue;
>   
> -        if (family == AF_INET6) {
> +        switch (ifa->ifa_addr->sa_family) {
> +        case AF_INET6:
>               addr->len = sizeof(addr->data.inet6);
>               memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len);
> -        } else {
> +            break;
> +        case AF_INET:
>               addr->len = sizeof(addr->data.inet4);
>               memcpy(&addr->data.inet4, ifa->ifa_addr, addr->len);
> +            break;
> +        default:
> +            continue;
>           }
> -        addr->data.stor.ss_family = family;
> +        addr->data.stor.ss_family = ifa->ifa_addr->sa_family;
>           ret = 0;
>           goto cleanup;
>       }