Re: [PATCH v7 06/21] multi-process: define MPQemuMsg format and transmission functions

[Stefan Hajnoczi]

On Sat, Jun 27, 2020 at 10:09:28AM -0700, elena.ufimtseva@oracle.com wrote:
> +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc)
> +{
> +    Error *local_err = NULL;
> +    struct iovec send[2];
> +    int *fds = NULL;
> +    size_t nfds = 0;
> +
> +    send[0].iov_base = msg;
> +    send[0].iov_len = MPQEMU_MSG_HDR_SIZE;
> +
> +    send[1].iov_base = msg->bytestream ? msg->data2 : (void *)&msg->data1;
> +    send[1].iov_len = msg->size;
> +
> +    if (msg->num_fds) {
> +        nfds = msg->num_fds;
> +        fds = msg->fds;
> +    }
> +
> +    (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, 
> nfds,
> +                                      &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +    }

Error propagation is missing. Is this by design, i.e. errors only need
to be handled in the read path, or is the patch incomplete?

> +}
> +
> +static int mpqemu_readv(QIOChannel *ioc, struct iovec *iov, int **fds,
> +                        size_t *nfds, Error **errp)
> +{
> +    size_t size, len;
> +
> +    size = iov->iov_len;
> +
> +    while (size > 0) {
> +        len = qio_channel_readv_full(ioc, iov, 1, fds, nfds, errp);

iovec buffers are overwritten when this loop iterates because iov is
not updated after reading some data.

fds is leaked when this loop iterates multiple times and there are fds
each time. fds/nfds should probably be set to 0 after the first
iteration.

> +
> +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            if (qemu_in_coroutine()) {
> +                qio_channel_yield(ioc, G_IO_IN);
> +            } else {
> +                qio_channel_wait(ioc, G_IO_IN);
> +            }
> +            continue;
> +        }
> +
> +        if (len <= 0) {
> +            return -EIO;
> +        }
> +
> +        size -= len;
> +    }
> +
> +    return iov->iov_len;
> +}
> +
> +int mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc)
> +{
> +    Error *local_err = NULL;
> +    int *fds = NULL;
> +    struct iovec hdr, data;
> +    size_t nfds = 0;
> +
> +    hdr.iov_base = g_malloc0(MPQEMU_MSG_HDR_SIZE);
> +    hdr.iov_len = MPQEMU_MSG_HDR_SIZE;
> +
> +    if (mpqemu_readv(ioc, &hdr, &fds, &nfds, &local_err) < 0) {
> +        return -EIO;

hdr.iov_base is leaked.

Why is hdr.iov_base allocated on the heap? Can you read directly into
msg instead of using an extra variable?

> +    }
> +
> +    memcpy(msg, hdr.iov_base, hdr.iov_len);
> +
> +    free(hdr.iov_base);
> +    if (msg->size > MPQEMU_MSG_DATA_MAX) {
> +        error_report("The message size is more than MPQEMU_MSG_DATA_MAX %d",
> +                     MPQEMU_MSG_DATA_MAX);
> +        return -EINVAL;
> +    }
> +
> +    data.iov_base = g_malloc0(msg->size);
> +    data.iov_len = msg->size;
> +
> +    if (mpqemu_readv(ioc, &data, NULL, NULL, &local_err) < 0) {
> +        return -EIO;

data.iov_base is leaked.

> +    }
> +
> +    if (msg->bytestream) {
> +        msg->data2 = calloc(1, msg->size);

QEMU uses glib memory allocation functions when possible. Please use
g_malloc0().

The calloc() and memcpy() can be avoided like this:

  msg->data2 = data.iov_base;
  data.iov_base = NULL; /* the pointer has been moved, don't free it */

> +        memcpy(msg->data2, data.iov_base, msg->size);
> +    } else {
> +        memcpy((void *)&msg->data1, data.iov_base, msg->size);

Buffer overflow when msg->size > sizeof(msg->data1). There should be a
check:

  if (msg->bytestream) {
      ...
  } else {
      if (msg->size > sizeof(msg->data1)) {
          ...handle invalid size...
          return -EIO;
      }
      memcpy(&msg->data1, data.iov_base, msg->size);
  }

> +    }
> +
> +    free(data.iov_base);

Should be g_free() since the allocation was made with g_malloc0().

> +
> +    if (nfds) {
> +        msg->num_fds = nfds;
> +        memcpy(msg->fds, fds, nfds * sizeof(int));
> +    }

It's safer to move the msg->num_fds = nfds outside the if statement so
that it always happens. That way num_fds is set to 0 and we don't depend
on the caller initializing it to 0.

signature.asc
Description: PGP signature