Re: [PATCH v2] hw/net/rtl8319: Work around GCC sanitizer / -Wstringop-overflow bug

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> If you compile QEMU with GCC with -fsanitize=address and
> -Wstringop-overflow, this causes GCC to produce a false-positive
> warning which it does not produce when the sanitizer is not enabled
> (and which makes compilation fail if you're using -Werror, as we do
> by default for builds from git):
>
> ../../hw/net/rtl8139.c: In function ‘rtl8139_io_writeb’:
> ../../hw/net/rtl8139.c:2264:17: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>  2264 |                 memcpy(data_to_checksum, saved_ip_header + 12, 8);
>       |                 ^
> In file included from ../../hw/net/rtl8139.c:62:
> /home/pm215/qemu/include/net/eth.h:50:14: note: at offset [8, 48] into destination object ‘ip_ver_len’ of size 1
>    50 |     uint8_t  ip_ver_len;     /* version and header length */
>       |              ^~~~~~~~~~
> ../../hw/net/rtl8139.c:2192:21: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>  2192 |                     memcpy(data_to_checksum, saved_ip_header + 12, 8);
>       |                     ^
> /home/pm215/qemu/include/net/eth.h:50:14: note: at offset [8, 48] into destination object ‘ip_ver_len’ of size 1
>    50 |     uint8_t  ip_ver_len;     /* version and header length */
>       |              ^~~~~~~~~~
> ../../hw/net/rtl8139.c:2192:21: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>  2192 |                     memcpy(data_to_checksum, saved_ip_header + 12, 8);
>       |                     ^
> /home/pm215/qemu/include/net/eth.h:50:14: note: at offset [8, 48] into destination object ‘ip_ver_len’ of size 1
>    50 |     uint8_t  ip_ver_len;     /* version and header length */
>       |              ^~~~~~~~~~
> In file included from /home/pm215/qemu/include/system/memory.h:21,
>                  from /home/pm215/qemu/include/hw/pci/pci.h:4,
>                  from /home/pm215/qemu/include/hw/pci/pci_device.h:4,
>                  from ../../hw/net/rtl8139.c:54:
> In function ‘stl_he_p’,
>     inlined from ‘stl_be_p’ at /home/pm215/qemu/include/qemu/bswap.h:371:5,
>     inlined from ‘rtl8139_cplus_transmit_one’ at ../../hw/net/rtl8139.c:2244:21,
>     inlined from ‘rtl8139_cplus_transmit’ at ../../hw/net/rtl8139.c:2345:28,
>     inlined from ‘rtl8139_io_writeb’ at ../../hw/net/rtl8139.c:2728:17:
> /home/pm215/qemu/include/qemu/bswap.h:284:5: error: writing 4 bytes into a region of size 0 [-Werror=stringop-overflow=]
>   284 |     __builtin_memcpy(ptr, &v, sizeof(v));
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/pm215/qemu/include/net/eth.h: In function ‘rtl8139_io_writeb’:
> /home/pm215/qemu/include/net/eth.h:50:14: note: at offset [24, 64] into destination object ‘ip_ver_len’ of size 1
>    50 |     uint8_t  ip_ver_len;     /* version and header length */
>       |              ^~~~~~~~~~
>
> This has been triaged as a bug in GCC:
>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114494
>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99673
> (the sanitizer pass rewrites the IR in a way that conflicts with its
> use by the warning pass that runs afterwards).
>
> Since this is the only place in our code where we hit this, work
> around it by disabling the -Wstringop-overflow in the part of
> the function that hits it. We do this only when using the
> address sanitizer on GCC, so that we still get the benefit
> of the warning in most compilation scenarios.
>
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3006
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v2: disable the warning rather than using the incorrect attempt
> at a workaround that v1 had.
>
> On the fence about whether this is worth backporting to stable.
> ---
>  hw/net/rtl8139.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 2ad6338ebe..424af73a18 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -2124,6 +2124,26 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      hlen, ip->ip_sum);
>              }
>  
> +            /*
> +             * The code in this function triggers a GCC bug where an
> +             * interaction between -fsanitize=address and -Wstringop-overflow
> +             * results in a false-positive stringop-overflow warning that is
> +             * only emitted when the address sanitizer is enabled:
> +             *     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114494
> +             *     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99673
> +             * GCC incorrectly thinks that the eth_payload_data buffer has
> +             * the type and size of the first field in 'struct ip_header', i.e.
> +             * one byte, and then complains about all other attempts to access
> +             * data in the buffer.
> +             *
> +             * Work around this by disabling the warning when building with
> +             * GCC and the address sanitizer is enabled.
> +             */
> +#pragma GCC diagnostic push
> +#if !defined(__clang__) && defined(QEMU_SANITIZE_ADDRESS)
> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> +#endif
> +
>              if ((txdw0 & CP_TX_LGSEN) && ip_protocol == IP_PROTO_TCP)
>              {
>                  /* Large enough for the TCP header? */
> @@ -2307,6 +2327,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                  /* restore IP header */
>                  memcpy(eth_payload_data, saved_ip_header, hlen);
>              }
> +
> +#pragma GCC diagnostic pop
> +
>          }
>  
>  skip_offload:

Tested-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro