On Sat, Jun 27, 2020 at 10:09:28AM -0700, elena.ufimts...@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