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

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(a)redhat.com&gt; Fixes: cb189648f11df6c4f7287dcaed4bc856650e2c3b Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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