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

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> ---
> 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 <armbru@redhat.com>