Message ID | 20181218175122.3229-6-philmd@redhat.com |
---|---|
State | New |
Headers | show |
Series | Fix strncpy() warnings for GCC8 new -Wstringop-truncation | expand |
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?
On 18/12/18 20:33, Eric Blake wrote: >> 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). It cannot---this is a pre_save hook. A possible overflow bug exists, but it is in the call to qapi_enum_parse. Paolo
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é <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; > + 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
Le mer. 19 déc. 2018 00:16, Michael S. Tsirkin <mst@redhat.com> a écrit : > 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é <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; > > + 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)); > OK I'll resend Marc-André previous patch. > 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. > OK, I'll have a look there. > > > } > > -- > > 2.17.2 >
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; }
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é <philmd@redhat.com> --- migration/global_state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)