Re: [PATCH 1/2] migration: avoid suspicious strncpy() use

[Eric Blake]

On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote:
> On 3/16/20 5:07 PM, Stefan Hajnoczi wrote:
> > gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
> > reports the following error:
> >
> >      CC      migration/global_state.o
> >    In file included from /usr/include/string.h:495,
> >                     from /home/stefanha/qemu/include/qemu/osdep.h:101,
> >                     from migration/global_state.c:13:
> >    In function ‘strncpy’,
> >        inlined from ‘global_state_store_running’ at 
> > migration/global_state.c:47:5:
> >    /usr/include/bits/string_fortified.h:106:10: error: 
> > ‘__builtin_strncpy’ specified bound 100 equals destination size 
> > [-Werror=stringop-truncation]
> >      106 |   return __builtin___strncpy_chk (__dest, __src, __len, 
> > __bos (__dest));
> >          |          
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
> > strings.
>
> There was a long discussion 1 year ago with it, and Eric suggested to 
> use strpadcpy after the assert() and I sent this patch:
> https://www.mail-archive.com/address@hidden/msg44925.html
> Not sure what's best.

strncpy() pads the tail, guaranteeing that for our fixed-size buffer, we 
guarantee the contents of all bytes in the buffer.  pstrcpy() does not 
(but pstrcpy() can be followed up with a memset() to emulate the 
remaining effects of strncpy() - at which point you have reimplemented 
strpadcpy).

> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > ---
> >   migration/global_state.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/global_state.c b/migration/global_state.c
> > index 25311479a4..cbe07f21a8 100644
> > --- a/migration/global_state.c
> > +++ b/migration/global_state.c
> > @@ -44,8 +44,8 @@ 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));
> > +    pstrcpy((char *)global_state.runstate,
> > +            sizeof(global_state.runstate), state);

Can we guarantee that the padding bytes have been previously set to 0, 
or do we need to go the extra mile with a memset() or strpadcpy() to 
guarantee that we have set the entire buffer?

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org