Re: [PATCH RESEND v6 11/36] multi-process: define mpqemu-link object

[Jag Raman]

> On May 12, 2020, at 4:56 AM, Stefan Hajnoczi <address@hidden> wrote:
> 
> On Wed, Apr 22, 2020 at 09:13:46PM -0700, address@hidden wrote:
>> From: Jagannathan Raman <address@hidden>
>> 
>> Defines mpqemu-link object which forms the communication link between
>> QEMU & emulation program.
>> Adds functions to configure members of mpqemu-link object instance.
>> Adds functions to send and receive messages over the communication
>> channel.
>> Adds GMainLoop to handle events received on the communication channel.
>> 
>> Signed-off-by: Jagannathan Raman <address@hidden>
>> Signed-off-by: John G Johnson <address@hidden>
>> Signed-off-by: Elena Ufimtseva <address@hidden>
> 
> This will change a lot when integrated into the QEMU event loop so I've
> skipped a lot of the code.
> 
> QIOChannel is probably the appropriate object to use instead of directly
> accessing a file descriptor.

OK, got it. Thanks!

> 
>> +/**
>> + * mpqemu_cmd_t:
>> + *
>> + * proc_cmd_t enum type to specify the command to be executed on the remote
>> + * device.
>> + */
>> +typedef enum {
>> +    INIT = 0,
>> +    MAX,
>> +} mpqemu_cmd_t;
>> +
>> +/**
>> + * MPQemuMsg:
>> + * @cmd: The remote command
>> + * @bytestream: Indicates if the data to be shared is structured (data1)
>> + *              or unstructured (data2)
>> + * @size: Size of the data to be shared
>> + * @data1: Structured data
>> + * @fds: File descriptors to be shared with remote device
>> + * @data2: Unstructured data
>> + *
>> + * MPQemuMsg Format of the message sent to the remote device from QEMU.
>> + *
>> + */
>> +typedef struct {
>> +    mpqemu_cmd_t cmd;
> 
> Please use an int field on the wire because the C standard says:
> 
>  Each enumerated type shall be compatible with char, a signed integer
>  type, or an unsigned integer type. The choice of type is
>  implementation-defined, but shall be capable of representing the
>  values of all the members of the enumeration.
> 
> So the compiler may make this a char field (which would introduce
> padding before the bytestream field) but if a new enum constant FOO =
> 0x100 is added then the compiler might change the size to 16-bit.
> 
>> +int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
>> +{
>> +    int rc;
>> +    uint8_t *data;
>> +    union {
>> +        char control[CMSG_SPACE(REMOTE_MAX_FDS * sizeof(int))];
>> +        struct cmsghdr align;
>> +    } u;
>> +    struct msghdr hdr;
>> +    struct cmsghdr *chdr;
>> +    size_t fdsize;
>> +    int sock = chan->sock;
>> +    QemuMutex *lock = &chan->recv_lock;
>> +
>> +    struct iovec iov = {
>> +        .iov_base = (char *) msg,
>> +        .iov_len = MPQEMU_MSG_HDR_SIZE,
>> +    };
>> +
>> +    memset(&hdr, 0, sizeof(hdr));
>> +    memset(&u, 0, sizeof(u));
>> +
>> +    hdr.msg_iov = &iov;
>> +    hdr.msg_iovlen = 1;
>> +    hdr.msg_control = &u;
>> +    hdr.msg_controllen = sizeof(u);
>> +
>> +    WITH_QEMU_LOCK_GUARD(lock) {
>> +        do {
>> +            rc = recvmsg(sock, &hdr, 0);
>> +        } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>> +
>> +        if (rc < 0) {
> 
> Missing rc != MPQEMU_MSG_HDR_SIZE check. If this was a short read we
> should not attempt to parse uninitialized bytes in msg.
> 
> This is more defensive than relying on catching bogus input values later
> on and also protects against accidentally revealing uninitialized memory
> contents by observing our error handling response.
> 
>> +            qemu_log_mask(LOG_REMOTE_DEBUG, "%s - recvmsg rc is %d, "
>> +                          "errno is %d, sock %d\n", __func__, rc, errno, 
>> sock);
>> +            return rc;
>> +        }
>> +
>> +        msg->num_fds = 0;
>> +        for (chdr = CMSG_FIRSTHDR(&hdr); chdr != NULL;
>> +             chdr = CMSG_NXTHDR(&hdr, chdr)) {
>> +            if ((chdr->cmsg_level == SOL_SOCKET) &&
>> +                (chdr->cmsg_type == SCM_RIGHTS)) {
>> +                fdsize = chdr->cmsg_len - CMSG_LEN(0);
>> +                msg->num_fds = fdsize / sizeof(int);
>> +                if (msg->num_fds > REMOTE_MAX_FDS) {
>> +                    qemu_log_mask(LOG_REMOTE_DEBUG,
>> +                                  "%s: Max FDs exceeded\n", __func__);
>> +                    return -ERANGE;
>> +                }
>> +
>> +                memcpy(msg->fds, CMSG_DATA(chdr), fdsize);
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (msg->bytestream) {
>> +            if (!msg->size) {
>> +                qemu_mutex_unlock(lock);
> 
> Duplicate unlock, we're already inside WITH_QEMU_LOCK_GUARD().
> 
>> +                return -EINVAL;
>> +            }
>> +
>> +            msg->data2 = calloc(1, msg->size);
> 
> What is the maximum message size? Please pick one and enforce it to
> protect against huge allocations that cause us to run out of memory.
> 
>> +            data = msg->data2;
>> +        } else {
>> +            data = (uint8_t *)&msg->data1;
> 
> Adding a uint8_t member to the union eliminates the need for a cast:
> 
>  union {
>      uint64_t u64;
>      uint8_t u8;
>  } data1;
> 
>  ...
> 
>  data = &msg->data1.u8;
> 
>> +        }
>> +
>> +        if (msg->size) {
>> +            do {
>> +                rc = read(sock, data, msg->size);
>> +            } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>> +        }
> 
> Short reads are an error. Please check that the sum of rc values is
> equal to msg->size.
> 
>> +    }
>> +    return rc;
>> +}
> ...
>> +bool mpqemu_msg_valid(MPQemuMsg *msg)
>> +{
>> +    if (msg->cmd >= MAX) {
>> +        return false;
>> +    }
> 
> Checking msg->cmd < 0 is also useful here, especially when the field
> type is changed to int.