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

[Michael S. Tsirkin]

On Tue, Dec 18, 2018 at 06:51:22PM +0100, 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.
> 
> 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é <address@hidden>
> ---
>  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;
> +    s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
>  
>      return 0;

I don't think this works correctly if strnlen returns
sizeof(s->runstate). Which never happens so we probably should
jus add

        assert(e->size is <= sizeof(s->runstate));

But also I think this is not enough, there's a problem in post-load
in the call to qapi_enum_parse. You probably want to force
the last character to 0 there.


>  }
> -- 
> 2.17.2