On Thu, 10 Jul 2025 at 13:44, Peter Maydell <[email protected]> wrote:
>
> On Thu, 26 Jun 2025 at 08:47, Cédric Le Goater <[email protected]> wrote:
> >
> > From: John Levon <[email protected]>
> >
> > Add the basic implementation for receiving vfio-user messages from the
> > control socket.
> >
>
> Hi; Coverity suggests there are some issues with this code
> (CID 1611807, 1611808, 1611809):
Hi; it looks like 1611807 and 1611808 (the resource leaks)
are still present in this code in current git; would somebody
like to have a look at this?
> > +/*
> > + * Receive and process one incoming message.
> > + *
> > + * For replies, find matching outgoing request and wake any waiters.
> > + * For requests, queue in incoming list and run request BH.
> > + */
> > +static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
> > +{
>
>
> > + /*
> > + * For replies, find the matching pending request.
> > + * For requests, reap incoming FDs.
> > + */
> > + if (isreply) {
> > + QTAILQ_FOREACH(msg, &proxy->pending, next) {
> > + if (hdr.id == msg->id) {
> > + break;
> > + }
> > + }
> > + if (msg == NULL) {
> > + error_setg(errp, "unexpected reply");
> > + goto err;
> > + }
> > + QTAILQ_REMOVE(&proxy->pending, msg, next);
> > +
> > + /*
> > + * Process any received FDs
> > + */
> > + if (numfds != 0) {
> > + if (msg->fds == NULL || msg->fds->recv_fds < numfds) {
> > + error_setg(errp, "unexpected FDs");
> > + goto err;
> > + }
> > + msg->fds->recv_fds = numfds;
> > + memcpy(msg->fds->fds, fdp, numfds * sizeof(int));
> > + }
> > + } else {
> > + if (numfds != 0) {
> > + reqfds = vfio_user_getfds(numfds);
> > + memcpy(reqfds->fds, fdp, numfds * sizeof(int));
> > + } else {
> > + reqfds = NULL;
> > + }
>
> Here we allocate memory into reqfds...
>
> > + }
> > +
> > + /*
> > + * Put the whole message into a single buffer.
> > + */
> > + if (isreply) {
> > + if (hdr.size > msg->rsize) {
> > + error_setg(errp, "reply larger than recv buffer");
> > + goto err;
> > + }
> > + *msg->hdr = hdr;
> > + data = (char *)msg->hdr + sizeof(hdr);
> > + } else {
> > + buf = g_malloc0(hdr.size);
> > + memcpy(buf, &hdr, sizeof(hdr));
> > + data = buf + sizeof(hdr);
> > + msg = vfio_user_getmsg(proxy, (VFIOUserHdr *)buf, reqfds);
> > + msg->type = VFIO_MSG_REQ;
>
> ...and here we allocate memory into msg...
>
> > + }
> > +
> > + /*
> > + * Read rest of message.
> > + */
> > + msgleft = hdr.size - sizeof(hdr);
> > + while (msgleft > 0) {
> > + ret = qio_channel_read(proxy->ioc, data, msgleft, errp);
> > +
> > + /* prepare to complete read on next iternation */
> > + if (ret == QIO_CHANNEL_ERR_BLOCK) {
> > + proxy->part_recv = msg;
> > + proxy->recv_left = msgleft;
> > + return ret;
> > + }
> > +
> > + if (ret <= 0) {
> > + goto fatal;
> > + }
>
> ...but here we may take an error-exit codepath to the 'fatal'
> label...
>
> > + trace_vfio_user_recv_read(hdr.id, ret);
> > +
> > + msgleft -= ret;
> > + data += ret;
> > + }
> > +
> > + vfio_user_process(proxy, msg, isreply);
> > + return 0;
> > +
> > + /*
> > + * fatal means the other side closed or we don't trust the stream
> > + * err means this message is corrupt
> > + */
> > +fatal:
> > + vfio_user_shutdown(proxy);
> > + proxy->state = VFIO_PROXY_ERROR;
> > +
> > + /* set error if server side closed */
> > + if (ret == 0) {
> > + error_setg(errp, "server closed socket");
> > + }
> > +
> > +err:
> > + for (i = 0; i < numfds; i++) {
> > + close(fdp[i]);
> > + }
> > + if (isreply && msg != NULL) {
> > + /* force an error to keep sending thread from hanging */
> > + vfio_user_set_error(msg->hdr, EINVAL);
> > + msg->complete = true;
> > + qemu_cond_signal(&msg->cv);
> > + }
> > + return -1;
>
> ...and in this error handling codepath we don't do anything
> to free either msg or reqfds.
thanks
-- PMM
