Re: [PATCH] hw/misc/ivshmem-pci: Improve error handling

[Peter Maydell]

On Fri, 11 Jul 2025 at 18:04, Markus Armbruster  wrote:
>
> Peter Maydell  writes:
>
> > Coverity points out that the ivshmem-pci code has some error handling
> > cases where it incorrectly tries to use an invalid filedescriptor.
> > These generally happen because ivshmem_recv_msg() calls
> > qemu_chr_fe_get_msgfd(), which might return -1, but the code in
> > process_msg() generally assumes that the file descriptor was provided
> > when it was supposed to be. In particular:
> >  * the error case in process_msg() only needs to close the fd
> >if one was provided
> >  * process_msg_shmem() should fail if no fd was provided

> >  if (msg < -1 || msg > IVSHMEM_MAX_PEERS) {
> >  error_setg(errp, "server sent invalid message %" PRId64, msg);
> > -close(fd);
> > +if (fd >= 0) {
> > +close(fd);
> > +}
>
> Coverity is overly picky.  close(-1) is *fine*.  Just like free(NULL).

I think there is a distinction here. free(NULL) is documented
to be a correct program action which must have no effect.
close() on a bad file descriptor is documented to be an
error, which the implementation is supposed to diagnose.

Also, Coverity flags this kind of thing up because it tends to
indicate that whoever wrote the code was not actually thinking
about the error condition; so when it gets caught later this
is more by luck than judgement.

thanks
-- PMM

Re: [PATCH] hw/misc/ivshmem-pci: Improve error handling

[Markus Armbruster]

Peter Maydell  writes:

> Coverity points out that the ivshmem-pci code has some error handling
> cases where it incorrectly tries to use an invalid filedescriptor.
> These generally happen because ivshmem_recv_msg() calls
> qemu_chr_fe_get_msgfd(), which might return -1, but the code in
> process_msg() generally assumes that the file descriptor was provided
> when it was supposed to be. In particular:
>  * the error case in process_msg() only needs to close the fd
>if one was provided
>  * process_msg_shmem() should fail if no fd was provided

It does even before the patch, because ...

>
> Coverity: CID 1508726
> Signed-off-by: Peter Maydell 
> ---
> Disclaimer: tested only with "make check"
> ---
>  hw/misc/ivshmem-pci.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/ivshmem-pci.c b/hw/misc/ivshmem-pci.c
> index 5a10bca633d..d47ae739d61 100644
> --- a/hw/misc/ivshmem-pci.c
> +++ b/hw/misc/ivshmem-pci.c
> @@ -479,6 +479,11 @@ static void process_msg_shmem(IVShmemState *s, int fd, 
> Error **errp)
>  struct stat buf;
>  size_t size;
>  
> +if (fd < 0) {
> +error_setg(errp, "server didn't provide fd with shared memory 
> message");
> +return;
> +}
> +
>  if (s->ivshmem_bar2) {
>  error_setg(errp, "server sent unexpected shared memory message");
>  close(fd);
   return;
   }

   if (fstat(fd, &buf) < 0) {

... fstat(-1, ...) fails with EBADF.

The additional check gets us a more helpful error message when the
server misbehaves this way.  Fine.

   error_setg_errno(errp, errno,
   "can't determine size of shared memory sent by server");
   close(fd);
   return;
   }

> @@ -553,7 +558,9 @@ static void process_msg(IVShmemState *s, int64_t msg, int 
> fd, Error **errp)
>  
>  if (msg < -1 || msg > IVSHMEM_MAX_PEERS) {
>  error_setg(errp, "server sent invalid message %" PRId64, msg);
> -close(fd);
> +if (fd >= 0) {
> +close(fd);
> +}

Coverity is overly picky.  close(-1) is *fine*.  Just like free(NULL).

>  return;
>  }

Regardless
Reviewed-by: Markus Armbruster