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/qemu-block@nongnu.org/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 <stefa...@redhat.com> > > > --- > > > 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
