Stefan Hajnoczi <stefa...@redhat.com> wrote:
> 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.
Sure thing.
Thanks, Juan.
