[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] migration: avoid suspicious strncpy() use
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 1/2] migration: avoid suspicious strncpy() use |
Date: |
Tue, 17 Mar 2020 09:52:22 +0000 |
On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote:
> 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?
I don't understand GlobalState:
1. You ask if runstate[] must be padded with NULs but neither
global_state_store() nor register_global_state() do that. Is it
really necessary to pad runstate[]?
If yes, is it safe for global_state_store() and
register_global_state() to not pad runstate[]?
If we decide the pad runstate[] to prevent information leaks to the
migration destination then I think it should be done in the pre-save
function so that it's guaranteed to happen no matter which of the 3
functions that write runstate[] has been called.
2. There is a GlobalState::size field that is only written and then
migrated but never read from what I can tell. :?
Juan: Please clarify the above. Thanks!
signature.asc
Description: PGP signature
- [PATCH 0/2] gcc 9.2 strncpy(3) warnings fixes, Stefan Hajnoczi, 2020/03/16
- [PATCH 1/2] migration: avoid suspicious strncpy() use, Stefan Hajnoczi, 2020/03/16
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use, Juan Quintela, 2020/03/16
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use, Philippe Mathieu-Daudé, 2020/03/16
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use, Eric Blake, 2020/03/16
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use,
Stefan Hajnoczi <=
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use, Philippe Mathieu-Daudé, 2020/03/17
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use, Juan Quintela, 2020/03/17
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use, Stefan Hajnoczi, 2020/03/17
- Re: [PATCH 1/2] migration: avoid suspicious strncpy() use, Juan Quintela, 2020/03/17
[PATCH 2/2] hw/rdma: avoid suspicious strncpy() use, Stefan Hajnoczi, 2020/03/16