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

[Greg Kurz]

On Sat, 11 Feb 2017 01:25:17 -0300
Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
> > ---
> >  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;
> >  }
> >
> >
> >

pgpezvc6k4VW0.pgp
Description: OpenPGP digital signature