[Libguestfs] [nbdkit PATCH] nbd: Fix sporadic use-after-free

[Richard W.M. Jones]

On Mon, Dec 04, 2017 at 03:34:01PM -0600, Eric Blake wrote:
> Now that we properly clean up 'trans' in the reader thread, we
> must not dereference 'trans' from the write thread at any point
> after trans has been added to the list unless we have grabbed
> it back off the list ourselves, or we risk an occasional
> use-after-free or even double free that valgrind can detect.
> 
> Reported-by: Richard W.M. Jones <rjones at redhat.com>
> Fixes: cb189648f11df6c4f7287dcaed4bc856650e2c3b
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> Perhaps I should also add more comments to the code about transfer of
> ownership of 'trans' between threads?
> 
>  plugins/nbd/nbd.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
> index e79042c..9d40e87 100644
> --- a/plugins/nbd/nbd.c
> +++ b/plugins/nbd/nbd.c
> @@ -227,6 +227,26 @@ nbd_mark_dead (struct handle *h)
>    return -1;
>  }
> 
> +/* Find and remove the transaction corresponding to cookie from the list. */
> +static struct transaction *
> +find_trans_by_cookie (struct handle *h, uint64_t cookie)
> +{
> +  struct transaction **ptr;
> +  struct transaction *trans;
> +
> +  nbd_lock (h);
> +  ptr = &h->trans;
> +  while ((trans = *ptr) != NULL) {
> +    if (cookie == trans->u.cookie)
> +      break;
> +    ptr = &trans->next;
> +  }
> +  if (trans)
> +    *ptr = trans->next;
> +  nbd_unlock (h);
> +  return trans;
> +}
> +
>  /* Send a request, return 0 on success or -1 on write failure. */
>  static int
>  nbd_request_raw (struct handle *h, uint32_t type, uint64_t offset,
> @@ -260,6 +280,8 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t offset,
>  {
>    int err;
>    struct transaction *trans;
> +  int fd;
> +  uint64_t cookie;
> 
>    trans = calloc (1, sizeof *trans);
>    if (!trans) {
> @@ -282,9 +304,14 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t offset,
>    }
>    trans->next = h->trans;
>    h->trans = trans;
> +  fd = trans->u.fds[0];
> +  cookie = trans->u.cookie;
>    nbd_unlock (h);
> -  if (nbd_request_raw (h, type, offset, count, trans->u.cookie, req_buf) == 0)
> -    return trans->u.fds[0];
> +  if (nbd_request_raw (h, type, offset, count, cookie, req_buf) == 0)
> +    return fd;
> +  trans = find_trans_by_cookie (h, cookie);
> +  if (!trans)
> +    return nbd_mark_dead (h);
> 
>   err:
>    err = errno;
> @@ -309,7 +336,6 @@ static int
>  nbd_reply_raw (struct handle *h, int *fd)
>  {
>    struct reply rep;
> -  struct transaction **ptr;
>    struct transaction *trans;
>    void *buf;
>    uint32_t count;
> @@ -320,16 +346,7 @@ nbd_reply_raw (struct handle *h, int *fd)
>    if (be32toh (rep.magic) != NBD_REPLY_MAGIC)
>      return nbd_mark_dead (h);
>    nbdkit_debug ("received reply for cookie %#" PRIx64, rep.handle);
> -  nbd_lock (h);
> -  ptr = &h->trans;
> -  while ((trans = *ptr) != NULL) {
> -    if (rep.handle == trans->u.cookie)
> -      break;
> -    ptr = &trans->next;
> -  }
> -  if (trans)
> -    *ptr = trans->next;
> -  nbd_unlock (h);
> +  trans = find_trans_by_cookie (h, rep.handle);
>    if (!trans) {
>      nbdkit_error ("reply with unexpected cookie %#" PRIx64, rep.handle);
>      return nbd_mark_dead (h);
> -- 
> 2.14.3

I wasn't able to conclusively say that this fixes the problem
because it was quite rare and now isn't reproducing for me
at all.

Nevertheless, ACK.  Please push, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/