On Thu, 12 Mar 2026 at 16:13, Kevin Wolf <[email protected]> wrote:
>
> 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.
Ah, so it does. The glib docs document this for g_malloc() but
don't mention it for g_try_malloc(), so I missed it.
> > > + 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).
Mmm. Let me know if you'd prefer me to mark this issue in
Coverity as a false positive rather than changing the code.
-- PMM
