On Tue, Mar 17, 2020 at 11:35:14AM +0100, Juan Quintela wrote: > Stefan Hajnoczi <stefa...@redhat.com> wrote: > > 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: > >> > >> > > >> > > > >> > > 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: > > Welcome to the club O:-) > > And I thought that with the reviewed-by I had finished here O:-) > > > 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[]? > > it is an error. All should be padded. > > > 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. > > Ok. > Taking a look at this. > > > 2. There is a GlobalState::size field that is only written and then > > migrated but never read from what I can tell. :? > > Grrr. It should be used, but it is not :-( > > What we have here: > - A static buffer > > uint8_t runstate[100]; > > That is partially filled. > size: is the size of that buffer that is filled. > > But, as we are using > > VMSTATE_BUFFER(runstate, GlobalState), > > We are always sending/receiving the whole buffer. THat is why we have > trouble with padding. What should we being doing? > > Sending just the size, the filled bytes, and making sure that there is > enough space on destination. > > But we aren't donig that. And at this point, I think that I am only > going to fix the 1st part (always zero pad everything sent). > > For fixing the other bit, I need to do an incompatible change. > > > Juan: Please clarify the above. Thanks! > > Thanks a lot. > > Later, Juan. > > PD: Why is it done this way? > Because at the moment, the problem was that qcow2 (as a system, not > as a device) didn't have a place where to plug pending requests. So > I created this section that always exist, and anything that has not > a device associated can hang a subsection here. Once that I created > it, nobody used it. > And now, just seing what you are telling, I didn't even used the > right approach.
Great, thanks for looking into this. Could you base your patches on top of this series? Then you can send them all together in a single pull request. That way we can be sure that padding will be added even after switching to pstrcpy() in my patch. Thanks, Stefan
signature.asc
Description: PGP signature