Re: [PATCH 2/4] vhost-user: Convert slave channel to QIOChannelSocket

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

On Tue, 9 Mar 2021 15:17:21 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Mon, Mar 08, 2021 at 01:31:39PM +0100, Greg Kurz wrote:
> > +    g_autofree int *fd = NULL;
> > +    size_t fdsize = 0;
> > +    int i;
> >  
> >      /* Read header */
> >      iov.iov_base = &hdr;
> >      iov.iov_len = VHOST_USER_HDR_SIZE;
> >  
> >      do {
> > -        size = recvmsg(u->slave_fd, &msgh, 0);
> > -    } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> > +        size = qio_channel_readv_full(ioc, &iov, 1, &fd, &fdsize, NULL);
> > +    } while (size == QIO_CHANNEL_ERR_BLOCK);
> 
> Is it possible to leak file descriptors and fd[] memory if we receive a
> short message and then loop around? qio_channel_readv_full() will
> overwrite &fd and that's how the leak occurs.
> 

qio_channel_readv_full() only returns QIO_CHANNEL_ERR_BLOCK when the
channel is non-blocking mode and no data is available. Under the hood,
this translates to this code in qio_channel_socket_readv():

 retry:
    ret = recvmsg(sioc->fd, &msg, sflags);
    if (ret < 0) {
        if (errno == EAGAIN) {
            return QIO_CHANNEL_ERR_BLOCK;
        }
        if (errno == EINTR) {
            goto retry;
        }

        error_setg_errno(errp, errno,
                         "Unable to read from socket");
        return -1;
    }

This is strictly equivalent to what we currently have. This cannot
leak file descriptors because we would only loop until the first
byte of real data is available and ancillary data cannot fly without
real data, i.e. no file descriptors when recvmsg() returns EAGAIN.

> On the other hand, it looks like ioc is in blocking mode. I'm not sure
> QIO_CHANNEL_ERR_BLOCK can occur?
> 

You're right but the existing code is ready to handle the non-blocking
case, so I just kept this behavior.

> > @@ -1500,8 +1479,8 @@ static void slave_read(void *opaque)
> >  
> >      /* Read payload */
> >      do {
> > -        size = read(u->slave_fd, &payload, hdr.size);
> > -    } while (size < 0 && (errno == EINTR || errno == EAGAIN));
> > +        size = qio_channel_read(ioc, (char *) &payload, hdr.size, NULL);
> > +    } while (size == QIO_CHANNEL_ERR_BLOCK);
> 
> Same question here.

This also ends up in qio_channel_socket_readv().

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]