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

[Juergen Gross <jgross@xxxxxxxx>]

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?

If yes, there is no need for resending the patch.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature