On Sat, 11 Feb 2017 01:25:17 -0300 Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> Hi Greg,
>
Hi Philippe,
> On 02/06/2017 02: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. Let's make this clear with 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 <gr...@kaod.org>
> > ---
> > 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..4ad42a1ad158 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 == 8);
>
> I'd be more specific:
>
> assert(retval == PROXY_HDR_SZ);
>
I deliberately chose to open code values according to the format string. No
matter what could happen to the code, "dd" means two 32-bit integers, i.e.
8 bytes... and to be honest, I don't find this PROXY_HDR_SZ macro that
useful, the code should use 8 as well, the same way we use 7 for the size
of the 9P header (size[4] type[1] tag[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);
>
> assert(ret == sizeof(status));
>
If status is converted to a larger type, this assertion will trigger, even
though 4 is really what we expect when passing "d".
Cheers.
--
Greg
> > 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 == sizeof(prstat));
> > 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 == sizeof(prstfs));
> > 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);
>
> assert(retval == PROXY_HDR_SZ);
>
> > 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 == 8);
>
> same
>
> > 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);
>
> assert(ret == sizeof(status));
>
> > return 0;
> > }
> >
> >
> >
pgp5kAmS2oABV.pgp
Description: OpenPGP digital signature
