Re: [PATCH] tools/xenstored: Don't crash xenstored when Live-Update is cancelled

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 22/06/2021 11:23, Juergen Gross wrote:
> On 17.06.21 19:38, Julien Grall wrote:
> > From: Julien GralL <jgrall@xxxxxxxxxx>
> >
> > As Live-Update is asynchronous, it is possible to receive a request to
> > cancel it (either on the same connection or from a different one).
> >
> > Currently, this will crash xenstored because do_lu_start() assumes
> > lu_status will be valid. This is not the case when Live-Update has been
> > cancelled. This will result to dereference a NULL pointer and
> > crash Xenstored.
> >
> > Rework do_lu_start() to check if lu_status is NULL and return an
> > error in this case.
> >
> > Fixes: af216a99fb ("tools/xenstore: add the basic framework for doing 
> > the live update")
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ----
> >
> > This is currently based on top of:
> >
> > https://lore.kernel.org/xen-devel/20210616144324.31652-1-julien@xxxxxxx
> >
> > This can be re-ordered if necessary.
> > ---
> >   tools/xenstore/xenstored_control.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_control.c 
> > b/tools/xenstore/xenstored_control.c
> > index a045f102a420..37a3d39f20b5 100644
> > --- a/tools/xenstore/xenstored_control.c
> > +++ b/tools/xenstore/xenstored_control.c
> > @@ -696,7 +696,18 @@ static bool do_lu_start(struct delayed_request *req)
> >       time_t now = time(NULL);
> >       const char *ret;
> >       struct buffered_data *saved_in;
> > -    struct connection *conn = lu_status->conn;
> > +    struct connection *conn = req->data;
> > +
> > +    /*
> > +     * Cancellation may have been requested asynchronously. In this
> > +     * case, lu_status will be NULL.
> > +     */
> > +    if (!lu_status) {
> > +        ret = "Cancellation was requested";
> > +        conn = req->data;
> This will set conn to the same value it already has.
Ah yes. I will drop this line.

Also, I took the opportunity to replace

} else
  assert(...)

with just

assert(...)

This should improve a bit the readability. Let me know if you want me to 
resend the patch for that.
> Other than that:
>
> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
Thank you!

Cheers,

--
Julien Grall