[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 5/5] migration: Use strnlen() for fixed-size
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string |
Date: |
Wed, 19 Dec 2018 10:24:37 +0100 |
Le mer. 19 déc. 2018 00:16, Michael S. Tsirkin <address@hidden> 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é <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));
>
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
>
- Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays, (continued)
Re: [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays, Andrew Jones, 2018/12/19
[Qemu-devel] [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays, Philippe Mathieu-Daudé, 2018/12/18
[Qemu-devel] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string, Philippe Mathieu-Daudé, 2018/12/18
Re: [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation, Philippe Mathieu-Daudé, 2018/12/18
Re: [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation, Michael S. Tsirkin, 2018/12/18
Re: [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation, no-reply, 2018/12/24