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é <phi...@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; > + 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