| Message ID | 20210616144324.31652-8-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xenstored: Bug fixes + Improve Live-Update | expand |
> On 16 Jun 2021, at 15:43, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > delay_request() is currently assuming that the request delayed is > always conn->in. This is currently correct, but it is a call for > a latent bug as the function allows the caller to specify any request. > > To prevent any future surprise, check if the request delayed is the > current one. > > Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution of a xenstore request") > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > --- > tools/xenstore/xenstored_core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index 2e5760fe4599..a5084a5b173d 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -306,7 +306,9 @@ int delay_request(struct connection *conn, struct buffered_data *in, > delayed_requests++; > list_add(&req->list, &conn->delayed); > > - conn->in = NULL; > + /* Unlink the request from conn if this is the current one */ > + if (conn->in == in) > + conn->in = NULL; > > return 0; > } > -- > 2.17.1 > >
On 16.06.21 16:43, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > delay_request() is currently assuming that the request delayed is > always conn->in. This is currently correct, but it is a call for > a latent bug as the function allows the caller to specify any request. > > To prevent any future surprise, check if the request delayed is the > current one. > > Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution of a xenstore request") Is the Fixes: tag really wanted in this patch? Currently there is nothing wrong without this patch. So a backport will be needed only if e.g. this series will be backported. I'm fine either way, but I think this should be Ian's call. > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
Hi Juergen, On 24/06/2021 09:44, Juergen Gross wrote: > On 16.06.21 16:43, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> delay_request() is currently assuming that the request delayed is >> always conn->in. This is currently correct, but it is a call for >> a latent bug as the function allows the caller to specify any request. >> >> To prevent any future surprise, check if the request delayed is the >> current one. >> >> Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution >> of a xenstore request") > > Is the Fixes: tag really wanted in this patch? Currently there is > nothing wrong without this patch. So a backport will be needed only if > e.g. this series will be backported. > > I'm fine either way, but I think this should be Ian's call. We don't usually backport to stable for tech preview (Xenstored Live-Update is one). In this case, I mainly added it because it makes easier for a developper to figure out where the bugs comes from and downstream may want to ingest it. This doesn't mean I request an official backport. I could just mention in the commit message if you prefer. Cheers,
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 2e5760fe4599..a5084a5b173d 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -306,7 +306,9 @@ int delay_request(struct connection *conn, struct buffered_data *in, delayed_requests++; list_add(&req->list, &conn->delayed); - conn->in = NULL; + /* Unlink the request from conn if this is the current one */ + if (conn->in == in) + conn->in = NULL; return 0; }