[Qemu-devel] [Bug 1803872] Re: [PATCH v3 5/5] migration: Use strnlen() for fixed-size string

[Eric Blake <1803872@bugs.launchpad.net>]

On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
> GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
> by string-modifying functions declared in <string.h>, such strncpy(),
> used in global_state_store_running().
> 
> Since the global_state.runstate does not necessarily contains a
> terminating NUL character, We had to use the QEMU_NONSTRING attribute.

s/We/we/

> 
> The GCC manual says about the nonstring attribute:
> 
>    However, when the array is declared with the attribute the call to
>    strlen is diagnosed because when the array doesn’t contain a
>    NUL-terminated string the call is undefined. [...]
>    In addition, calling strnlen and strndup with such arrays is safe
>    provided a suitable bound is specified, and not diagnosed.
> 
> GCC indeed found an incorrect use of strlen(), because this array
> is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
> using qapi_enum_parse which does not get the buffer length.
> 
> Use strnlen() which returns sizeof(s->runstate) if the array is not
> NUL-terminated.
> 
> This fixes:
> 
>      CC      migration/global_state.o
>    qemu/migration/global_state.c: In function 'global_state_pre_save':
>    qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=]
>         s->size = strlen((char *)s->runstate) + 1;
>                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
>         uint8_t runstate[100] QEMU_NONSTRING;
>                 ^~~~~~~~
>    cc1: all warnings being treated as errors
>    make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   migration/global_state.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 6e19333422..c19030ef62 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
>       GlobalState *s = opaque;
>   
>       trace_migrate_global_state_pre_save((char *)s->runstate);
> -    s->size = strlen((char *)s->runstate) + 1;

The old code sets s->size to the string length + space for the NUL byte 
(by assuming that a NUL byte was present), and accidentally sets it 
beyond the s->runstate array if there was no NUL byte (our existing 
runstate names are shorter than 100 bytes, so this could only happen on 
a malicious stream).

> +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;

The new code can still end up setting s->size beyond the array.  Is that 
intended, or would it be better to use strnlen(s->runstate, 
sizeof(s->runstate) - 1) + 1?

Also, as I argued on 4/5, why isn't this squashed in with the patch that 
marks the field NONSTRING?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1803872

Title:
  gcc 8.2 reports stringop-truncation when building qemu

Status in QEMU:
  New

Bug description:
  QEMU 3.0

  block/sheepdog.c: In function 'find_vdi_name':
  block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
       strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  If this is the intended behavior, please suppress the warning. For
  example:

  #pragma GCC diagnostic push
  #pragma GCC diagnostic ignored "-Wstringop-truncation"
      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  #pragma GCC diagnostic pop

  This also happens on other sources, for example hw/acpi/core.c, so
  another option is to suppress it globally on CFLAGS (-Wno-stringop-
  truncation)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1803872/+subscriptions