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

On 7/22/20 1:21 PM, Daniel P. Berrangé wrote:

Historically we avoided -fstack-protector* since it resulted in a broken build on Mingw. In GCC 10 in Fedora though, we have the opposite problem, getting a broken build if we don't enable one of the -fstack-protector* options. This also works in GCC 9, so we don't need to worry about the old brokeness which evidentally got fixed at some time without noticing.

...and I guess there's no "super old" mingw releases that we need to worry about, since it's always the mingw on the current release of Fedora that's used (did I get that right?) Reviewed-by: Laine Stump <laine@redhat.com>

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- m4/virt-compile-warnings.m4 | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index d3538d59f8..d171d09991 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -169,13 +169,11 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ gl_WARN_ADD([-Wframe-larger-than=262144], [RELAXED_FRAME_LIMIT_CFLAGS])

# Extra special flags - dnl -fstack-protector stuff passes gl_WARN_ADD with gcc - dnl on Mingw32, but fails when actually used case $host in aarch64-*-*) dnl "error: -fstack-protector not supported for this target [-Werror]" ;; - *-*-linux*) + *-*-linux* | *-*-mingw*) dnl Prefer -fstack-protector-strong if it's available. dnl There doesn't seem to be great overhead in adding dnl -fstack-protector-all instead of -fstack-protector.

On 7/22/20 1:21 PM, Daniel P. Berrangé wrote:

GCC 10 complains about "desc" possibly being a NULL dereference. Even though it is a false positive, we can easily avoid it.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com> So those were the only complaints of gcc 10? We got off easy :-)

--- tools/vsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c index 527c135424..b65e99cbd2 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -689,7 +689,7 @@ vshCmddefHelp(vshControl *ctl, const vshCmdDef *def) fputc('\n', stdout);

desc = vshCmddefGetInfo(def, "desc"); - if (*desc) { + if (desc && *desc) { /* Print the description only if it's not empty. */ fputs(_("\n DESCRIPTION\n"), stdout); fprintf(stdout, " %s\n", _(desc));