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@xxxxxxxxxx>
(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@xxxxxxxxxx>
---
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;
}
