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

[Julien Grall <julien@xxxxxxx>]

On 24/06/2021 10:18, Julien Grall wrote:
> Hi Juergen,
>
> On 24/06/2021 10:17, Juergen Gross wrote:
> > On 24.06.21 10:12, Julien Grall wrote:
> > > 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.
> > I guess you are planning to do the commit?
> That's my plan.
Committed.

> > If yes, there is no need for resending the patch.
> Thanks!
>
> Cheers,
--
Julien Grall