From: Markus Armbruster <armbru@redhat.com>
References: <1414947218-7618-1-git-send-email-mst@redhat.com>
Date: Mon, 03 Nov 2014 16:09:36 +0100
In-Reply-To: <1414947218-7618-1-git-send-email-mst@redhat.com> (Michael
	S. Tsirkin's message of "Sun, 2 Nov 2014 18:53:57 +0200")
Message-ID: <87bnoouxqn.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH] qemu-char: fix tcp_get_fds
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org

"Michael S. Tsirkin" <mst@redhat.com> writes:

> tcp_get_fds API discards fds if there's more than 1 of these.

s/tcp_get_fds/tcp_get_msgfds/ (subject as well)

What exactly doesn't work without this patch?

> It's tricky to fix this without API changes in the generic case.
>
> However, this API is only used by tests ATM, and tests know how
> many fds they expect.
>
> So let's not waste cycles trying to fix this properly:
> simply assume at most 16 fds (tests use at most 8 now).
> assert if some test tries to get more.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  qemu-char.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index bd0709b..1c4004c 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -88,6 +88,7 @@
>  #define READ_BUF_LEN 4096
>  #define READ_RETRIES 10
>  #define CHR_MAX_FILENAME_SIZE 256
> +#define TCP_MAX_FDS 16
>  
>  /***********************************************************/
>  /* Socket address helpers */
> @@ -2668,6 +2669,8 @@ static int tcp_get_msgfds(CharDriverState *chr, int *fds, int num)
>      TCPCharDriver *s = chr->opaque;
>      int to_copy = (s->read_msgfds_num < num) ? s->read_msgfds_num : num;
>  
> +    assert(num <= TCP_MAX_FDS);
> +
>      if (to_copy) {
>          int i;
>  

This where we copy received fds out of ->read_msgfds.  If someone asks
for more than TCP_MAX_FDS, the buffer in the next hunk is insufficient.

> @@ -2762,7 +2765,7 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>      struct iovec iov[1];
>      union {
>          struct cmsghdr cmsg;
> -        char control[CMSG_SPACE(sizeof(int))];
> +        char control[CMSG_SPACE(sizeof(int) * TCP_MAX_FDS)];
>      } msg_control;
>      int flags = 0;
>      ssize_t ret;

This is where we receive the fds into ->read_msgfds.  How many depends
on sizeof(msg_control).  One before your patch, TCP_MAX_FDS after.

Reviewed-by: Markus Armbruster <armbru@redhat.com>