From 9e06080bed293d94ec1f874e62f25f147b20bc6c Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Fri, 12 Mar 2021 10:22:07 +0100 Subject: vhost-user: Fix double-close on slave_read() error path Some message types, e.g. VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG, can convey file descriptors. These must be closed before returning from slave_read() to avoid being leaked. This can currently be done in two different places: [1] just after the request has been processed [2] on the error path, under the goto label err: These path are supposed to be mutually exclusive but they are not actually. If the VHOST_USER_NEED_REPLY_MASK flag was passed and the sending of the reply fails, both [1] and [2] are performed with the same descriptor values. This can potentially cause subtle bugs if one of the descriptor was recycled by some other thread in the meantime. This code duplication complicates rollback for no real good benefit. Do the closing in a unique place, under a new fdcleanup: goto label at the end of the function. Signed-off-by: Greg Kurz Message-Id: <20210312092212.782255-3-groug@kaod.org> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Stefan Hajnoczi --- hw/virtio/vhost-user.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) (limited to 'hw/virtio') diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 6af9b43..acde1d2 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1475,13 +1475,6 @@ static void slave_read(void *opaque) ret = -EINVAL; } - /* Close the remaining file descriptors. */ - for (i = 0; i < fdsize; i++) { - if (fd[i] != -1) { - close(fd[i]); - } - } - /* * REPLY_ACK feature handling. Other reply types has to be managed * directly in their request handlers. @@ -1511,12 +1504,14 @@ static void slave_read(void *opaque) } } - return; + goto fdcleanup; err: qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL); close(u->slave_fd); u->slave_fd = -1; + +fdcleanup: for (i = 0; i < fdsize; i++) { if (fd[i] != -1) { close(fd[i]); -- cgit v1.1