Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben:
> On Tue, 10 Mar 2026 at 16:30, Kevin Wolf <[email protected]> wrote:
> >
> > From: Peter Lieven <[email protected]>
> >
> > libnfs v6 added a new api structure for read and write requests.
> >
> > This effectively also adds zero copy read support for cases where
> > the qiov coming from the block layer has only one vector.
> >
> > The .brdv_refresh_limits implementation is needed because libnfs v6
> > silently dropped support for splitting large read/write request into
> > chunks.
> >
> > Signed-off-by: Ronnie Sahlberg <[email protected]>
> > Signed-off-by: Peter Lieven <[email protected]>
> > Message-ID: <[email protected]>
> > Reviewed-by: Kevin Wolf <[email protected]>
> > Signed-off-by: Kevin Wolf <[email protected]>
>
>
> Hi; Coverity reports a potential issue with this code
> (CID 1645631):
>
> > @@ -266,13 +270,36 @@ static int coroutine_fn
> > nfs_co_preadv(BlockDriverState *bs, int64_t offset,
> > {
> > NFSClient *client = bs->opaque;
> > NFSRPC task;
> > + char *buf = NULL;
> > + bool my_buffer = false;
> >
> > nfs_co_init_task(bs, &task);
> > - task.iov = iov;
> > +
> > +#ifdef LIBNFS_API_V2
> > + if (iov->niov != 1) {
> > + buf = g_try_malloc(bytes);
> > + if (bytes && buf == NULL) {
> > + return -ENOMEM;
> > + }
> > + my_buffer = true;
>
> Here we have code that takes the "read zero bytes" case, and
> still tries to malloc a 0-length buffer (which is of dubious
> portability). Then it will continue...
g_try_malloc() always returns NULL for allocating 0 bytes, so I don't
think portability is a problem here.
> > + } else {
> > + buf = iov->iov[0].iov_base;
> > + }
> > +#endif
> >
> > WITH_QEMU_LOCK_GUARD(&client->mutex) {
> > +#ifdef LIBNFS_API_V2
> > + if (nfs_pread_async(client->context, client->fh,
> > + buf, bytes, offset,
> > + nfs_co_generic_cb, &task) != 0) {
> > +#else
> > + task.iov = iov;
> > if (nfs_pread_async(client->context, client->fh,
> > offset, bytes, nfs_co_generic_cb, &task) != 0)
> > {
> > +#endif
> > + if (my_buffer) {
> > + g_free(buf);
> > + }
> > return -ENOMEM;
> > }
> >
> > @@ -280,6 +307,13 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState
> > *bs, int64_t offset,
> > }
> > qemu_coroutine_yield();
> >
> > + if (my_buffer) {
> > + if (task.ret > 0) {
> > + qemu_iovec_from_buf(iov, 0, buf, task.ret);
>
> ...and down here we use 'buf', but Coverity thinks it might be NULL
> because we only returned -ENOMEM above for a NULL buffer if bytes == 0.
> So we might be here with bytes == 0 and buf == NULL.
Yes, buf might be NULL, but copying 0 bytes from NULL isn't a problem
because you don't actually dereference the pointer then.
I think the part that Coverity doesn't understand is probably that
task.ret is limited to bytes (i.e. 0 in this case).
> Maybe we can't get here, but maybe it would be simpler to handle
> the "asked to read 0 bytes" case directly without calling into the
> nfs library or allocating a 0 byte buffer?
We could have an 'if (!bytes) { return 0; }' at the start of the
function if we want to.
Kevin
