Re: [Qemu-devel] [PATCH v3] 9pfs: proxy: assert if unmarshal fails

[Philippe Mathieu-Daudé]

On 03/17/2017 08:20 PM, Greg Kurz wrote:
> Replies from the virtfs proxy are made up of a fixed-size header (8 bytes)
> and a payload of variable size (maximum 64kb). When receiving a reply,
> the proxy backend first reads the whole header and then unmarshals it.
> If the header is okay, it then does the same operation with the payload.
>
> Since the proxy backend uses a pre-allocated buffer which has enough room
> for a header and the maximum payload size, marshalling should never fail
> with fixed size arguments. Any error here is likely to result from a more
> serious corruption in QEMU and we'd better dump core right away.
>
> This patch adds error checks where they are missing and converts the
> associated error paths into assertions.
>
> This should also address Coverity's complaints CID 1348519 and CID 1348520,
> about not always checking the return value of proxy_unmarshal().
>
> Signed-off-by: Greg Kurz <address@hidden>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> ---
> v3: - completely get rid of sizeof() and QEMU_BUILD_BUG_ON() as it was
>       confusing. Only use literal values in checks finally.
> ---
>  hw/9pfs/9p-proxy.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index f4aa7a9d70f8..28b20a7c3dfa 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>          return retval;
>      }
>      reply->iov_len = PROXY_HDR_SZ;
> -    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> +    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> +    assert(retval == 4 * 2);
>      /*
>       * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and
>       * return -ENOBUFS
> @@ -194,9 +195,7 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>      if (header.type == T_ERROR) {
>          int ret;
>          ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
> -        if (ret < 0) {
> -            *status = ret;
> -        }
> +        assert(ret == 4);
>          return 0;
>      }
>
> @@ -213,6 +212,7 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>                                   &prstat.st_atim_sec, &prstat.st_atim_nsec,
>                                   &prstat.st_mtim_sec, &prstat.st_mtim_nsec,
>                                   &prstat.st_ctim_sec, &prstat.st_ctim_nsec);
> +        assert(retval == 8 * 3 + 4 * 3 + 8 * 10);
>          prstat_to_stat(response, &prstat);
>          break;
>      }
> @@ -225,6 +225,7 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>                                   &prstfs.f_files, &prstfs.f_ffree,
>                                   &prstfs.f_fsid[0], &prstfs.f_fsid[1],
>                                   &prstfs.f_namelen, &prstfs.f_frsize);
> +        assert(retval == 8 * 11);
>          prstatfs_to_statfs(response, &prstfs);
>          break;
>      }
> @@ -246,7 +247,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>          break;
>      }
>      case T_GETVERSION:
> -        proxy_unmarshal(reply, PROXY_HDR_SZ, "q", response);
> +        retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "q", response);
> +        assert(retval == 8);
>          break;
>      default:
>          return -1;
> @@ -274,18 +276,16 @@ static int v9fs_receive_status(V9fsProxy *proxy,
>          return retval;
>      }
>      reply->iov_len = PROXY_HDR_SZ;
> -    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> -    if (header.size != sizeof(int)) {
> -        *status = -ENOBUFS;
> -        return 0;
> -    }
> +    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> +    assert(retval == 4 * 2);
>      retval = socket_read(proxy->sockfd,
>                           reply->iov_base + PROXY_HDR_SZ, header.size);
>      if (retval < 0) {
>          return retval;
>      }
>      reply->iov_len += header.size;
> -    proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
> +    retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
> +    assert(retval == 4);
>      return 0;
>  }