Re: [Qemu-devel] [PULL 01/12] util/iov: introduce qemu_iovec_init_extended

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 27 Aug 2019 at 21:16, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Introduce new initialization API, to create requests with padding. Will
> be used in the following patch. New API uses qemu_iovec_init_buf if
> resulting io vector has only one element, to avoid extra allocations.
> So, we need to update qemu_iovec_destroy to support destroying such
> QIOVs.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-id: 20190604161514.262241-2-vsementsov@virtuozzo.com
> Message-Id: <20190604161514.262241-2-vsementsov@virtuozzo.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Hi -- Coverity thinks this new function could have an
out-of-bounds read (CID 1405302):

> +/*
> + * Compile new iovec, combining @head_buf buffer, sub-qiov of @mid_qiov,
> + * and @tail_buf buffer into new qiov.
> + */
> +void qemu_iovec_init_extended(
> +        QEMUIOVector *qiov,
> +        void *head_buf, size_t head_len,
> +        QEMUIOVector *mid_qiov, size_t mid_offset, size_t mid_len,
> +        void *tail_buf, size_t tail_len)
> +{
> +    size_t mid_head, mid_tail;
> +    int total_niov, mid_niov = 0;
> +    struct iovec *p, *mid_iov;
> +
> +    if (mid_len) {
> +        mid_iov = qiov_slice(mid_qiov, mid_offset, mid_len,
> +                             &mid_head, &mid_tail, &mid_niov);
> +    }
> +
> +    total_niov = !!head_len + mid_niov + !!tail_len;
> +    if (total_niov == 1) {
> +        qemu_iovec_init_buf(qiov, NULL, 0);
> +        p = &qiov->local_iov;
> +    } else {
> +        qiov->niov = qiov->nalloc = total_niov;
> +        qiov->size = head_len + mid_len + tail_len;
> +        p = qiov->iov = g_new(struct iovec, qiov->niov);
> +    }
> +
> +    if (head_len) {
> +        p->iov_base = head_buf;
> +        p->iov_len = head_len;
> +        p++;
> +    }
> +
> +    if (mid_len) {
> +        memcpy(p, mid_iov, mid_niov * sizeof(*p));
> +        p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head;
> +        p[0].iov_len -= mid_head;
> +        p[mid_niov - 1].iov_len -= mid_tail;
> +        p += mid_niov;
> +    }
> +
> +    if (tail_len) {
> +        p->iov_base = tail_buf;
> +        p->iov_len = tail_len;
> +    }
> +}

but I'm not familiar enough with the code to be able to tell
if it's correct or if it's just getting confused. Could
somebody have a look? (It's possible it's getting confused
because the calculation of 'total_niov' uses 'mid_niov',
but the condition guarding the code that fills in that part
of the vector is 'mid_len', so it thinks it can take the
"total_niov == 1" codepath and also the "head_len == true"
and "mid_len != 0" paths; in which case using "if (mid_niov)"
instead might make it happier.)

thanks
-- PMM