| Message ID | 20181120152753.10463-3-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | strcpy: fix stringop-truncation warnings | expand |
On 11/20/18 9:27 AM, Marc-André Lureau wrote: > Adding an assert is enough to silence GCC. > > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running': > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation] > strncpy((char *)global_state.runstate, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > state, sizeof(global_state.runstate)); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > (alternatively, we could hard-code "running") > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > migration/global_state.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Eric Blake <eblake@redhat.com> I think this is safe for 3.1, but I know the migration code is particularly wary of assert()s, even when they are non-triggerable (a 100-byte buffer at global_state.runstate is big enough for ALL of the run states, not just RUN_STATE_RUNNING). > > diff --git a/migration/global_state.c b/migration/global_state.c > index 8e8ab5c51e..01805c567a 100644 > --- a/migration/global_state.c > +++ b/migration/global_state.c > @@ -42,6 +42,7 @@ int global_state_store(void) > void global_state_store_running(void) > { > const char *state = RunState_str(RUN_STATE_RUNNING); > + assert(strlen(state) < sizeof(global_state.runstate)); > strncpy((char *)global_state.runstate, > state, sizeof(global_state.runstate)); > } >
* Eric Blake (eblake@redhat.com) wrote: > On 11/20/18 9:27 AM, Marc-André Lureau wrote: > > Adding an assert is enough to silence GCC. > > > > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running': > > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation] > > strncpy((char *)global_state.runstate, > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > state, sizeof(global_state.runstate)); > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > cc1: all warnings being treated as errors > > > > (alternatively, we could hard-code "running") > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > migration/global_state.c | 1 + > > 1 file changed, 1 insertion(+) > > Reviewed-by: Eric Blake <eblake@redhat.com> > > I think this is safe for 3.1, but I know the migration code is particularly > wary of assert()s, even when they are non-triggerable (a 100-byte buffer at > global_state.runstate is big enough for ALL of the run states, not just > RUN_STATE_RUNNING). That's OK; the universe would have to be particularly broken to trigger that one, and it's in no way connected with any state, so it would trigger on even the most basic tests. However, I wonder if this fixes the problem on mingw builds - windows asserts are not marked as noreturn. Dave > > > > diff --git a/migration/global_state.c b/migration/global_state.c > > index 8e8ab5c51e..01805c567a 100644 > > --- a/migration/global_state.c > > +++ b/migration/global_state.c > > @@ -42,6 +42,7 @@ int global_state_store(void) > > void global_state_store_running(void) > > { > > const char *state = RunState_str(RUN_STATE_RUNNING); > > + assert(strlen(state) < sizeof(global_state.runstate)); > > strncpy((char *)global_state.runstate, > > state, sizeof(global_state.runstate)); > > } > > > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hi On Tue, Nov 20, 2018 at 9:22 PM Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Eric Blake (eblake@redhat.com) wrote: > > On 11/20/18 9:27 AM, Marc-André Lureau wrote: > > > Adding an assert is enough to silence GCC. > > > > > > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running': > > > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation] > > > strncpy((char *)global_state.runstate, > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > state, sizeof(global_state.runstate)); > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > cc1: all warnings being treated as errors > > > > > > (alternatively, we could hard-code "running") > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > --- > > > migration/global_state.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > > I think this is safe for 3.1, but I know the migration code is particularly > > wary of assert()s, even when they are non-triggerable (a 100-byte buffer at > > global_state.runstate is big enough for ALL of the run states, not just > > RUN_STATE_RUNNING). > > That's OK; the universe would have to be particularly broken to trigger > that one, and it's in no way connected with any state, so it would > trigger on even the most basic tests. > > However, I wonder if this fixes the problem on mingw builds - windows > asserts are not marked as noreturn. On f29, with mingw64-gcc-8.2.0-3.fc29.x86_64, it fixes the warning. > > Dave > > > > > > > diff --git a/migration/global_state.c b/migration/global_state.c > > > index 8e8ab5c51e..01805c567a 100644 > > > --- a/migration/global_state.c > > > +++ b/migration/global_state.c > > > @@ -42,6 +42,7 @@ int global_state_store(void) > > > void global_state_store_running(void) > > > { > > > const char *state = RunState_str(RUN_STATE_RUNNING); > > > + assert(strlen(state) < sizeof(global_state.runstate)); > > > strncpy((char *)global_state.runstate, > > > state, sizeof(global_state.runstate)); > > > } > > > > > > > -- > > Eric Blake, Principal Software Engineer > > Red Hat, Inc. +1-919-301-3266 > > Virtualization: qemu.org | libvirt.org > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote: > Hi > > On Tue, Nov 20, 2018 at 9:22 PM Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > > > * Eric Blake (eblake@redhat.com) wrote: > > > On 11/20/18 9:27 AM, Marc-André Lureau wrote: > > > > Adding an assert is enough to silence GCC. > > > > > > > > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running': > > > > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation] > > > > strncpy((char *)global_state.runstate, > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > state, sizeof(global_state.runstate)); > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > cc1: all warnings being treated as errors > > > > > > > > (alternatively, we could hard-code "running") > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > --- > > > > migration/global_state.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > > > > I think this is safe for 3.1, but I know the migration code is particularly > > > wary of assert()s, even when they are non-triggerable (a 100-byte buffer at > > > global_state.runstate is big enough for ALL of the run states, not just > > > RUN_STATE_RUNNING). > > > > That's OK; the universe would have to be particularly broken to trigger > > that one, and it's in no way connected with any state, so it would > > trigger on even the most basic tests. > > > > However, I wonder if this fixes the problem on mingw builds - windows > > asserts are not marked as noreturn. > > On f29, with mingw64-gcc-8.2.0-3.fc29.x86_64, it fixes the warning. OK, fine. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Dave > > > > Dave > > > > > > > > > > diff --git a/migration/global_state.c b/migration/global_state.c > > > > index 8e8ab5c51e..01805c567a 100644 > > > > --- a/migration/global_state.c > > > > +++ b/migration/global_state.c > > > > @@ -42,6 +42,7 @@ int global_state_store(void) > > > > void global_state_store_running(void) > > > > { > > > > const char *state = RunState_str(RUN_STATE_RUNNING); > > > > + assert(strlen(state) < sizeof(global_state.runstate)); > > > > strncpy((char *)global_state.runstate, > > > > state, sizeof(global_state.runstate)); > > > > } > > > > > > > > > > -- > > > Eric Blake, Principal Software Engineer > > > Red Hat, Inc. +1-919-301-3266 > > > Virtualization: qemu.org | libvirt.org > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 20/11/18 16:27, Marc-André Lureau wrote: > Adding an assert is enough to silence GCC. > > ~/src/qemu/migration/global_state.c: In function 'global_state_store_running': > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation] > strncpy((char *)global_state.runstate, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > state, sizeof(global_state.runstate)); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > (alternatively, we could hard-code "running") > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > migration/global_state.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/migration/global_state.c b/migration/global_state.c > index 8e8ab5c51e..01805c567a 100644 > --- a/migration/global_state.c > +++ b/migration/global_state.c > @@ -42,6 +42,7 @@ int global_state_store(void) > void global_state_store_running(void) > { > const char *state = RunState_str(RUN_STATE_RUNNING); > + assert(strlen(state) < sizeof(global_state.runstate)); > strncpy((char *)global_state.runstate, > state, sizeof(global_state.runstate)); > } >
diff --git a/migration/global_state.c b/migration/global_state.c index 8e8ab5c51e..01805c567a 100644 --- a/migration/global_state.c +++ b/migration/global_state.c @@ -42,6 +42,7 @@ int global_state_store(void) void global_state_store_running(void) { const char *state = RunState_str(RUN_STATE_RUNNING); + assert(strlen(state) < sizeof(global_state.runstate)); strncpy((char *)global_state.runstate, state, sizeof(global_state.runstate)); }
Adding an assert is enough to silence GCC. ~/src/qemu/migration/global_state.c: In function 'global_state_store_running': ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation] strncpy((char *)global_state.runstate, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ state, sizeof(global_state.runstate)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors (alternatively, we could hard-code "running") Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- migration/global_state.c | 1 + 1 file changed, 1 insertion(+)