[PATCH] util: refactor code to workaround gcc 10.1.0 bug

Daniel P. Berrangé posted 1 patch 3 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200722153628.1844797-1-berrange@redhat.com
src/util/virnetdevip.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
[PATCH] util: refactor code to workaround gcc 10.1.0 bug
Posted by Daniel P. Berrangé 3 years, 9 months ago
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@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;
     }
-- 
2.24.1