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

[Greg Kurz]

On Fri, 17 Mar 2017 12:06:39 -0300
Philippe Mathieu-Daudé <address@hidden> wrote:

> On 03/17/2017 11:43 AM, Daniel P. Berrange wrote:
> > On Fri, Mar 17, 2017 at 03:39:10PM +0100, 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.
> >>
> >> Note that we don't want to use sizeof() in the checks since the value
> >> we want to use depends on the format rather than the size of the buffer.
> >> Short marshal formats can be handled with numerical values. Size macros
> >> are introduced for bigger ones (ProxyStat and ProxyStatFS).  
> 
> :) maybe this comment should go somewhere in <hw/9pfs/9p-proxy.h>
> 
> >> 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>
> >> ---
> >> v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros
> >>     - updated changelog
> >> ---
> >>  hw/9pfs/9p-proxy.c |   24 +++++++++++++-----------
> >>  hw/9pfs/9p-proxy.h |   10 ++++++++--
> >>  2 files changed, 21 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> >> index f4aa7a9d70f8..363bea66035e 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);
> >>      /*
> >>       * if response size > PROXY_MAX_IO_SZ, read the response but ignore 
> >> it and
> >>       * return -ENOBUFS
> >> @@ -194,15 +195,14 @@ 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;
> >>      }
> >>
> >>      switch (type) {
> >>      case T_LSTAT: {
> >>          ProxyStat prstat;
> >> +        QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ);  
> >
> > I'd just put this assert at the struct declaration
> >
> > ..snip...
> >  
> >> diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h
> >> index b84301d001b0..918c45016a3c 100644
> >> --- a/hw/9pfs/9p-proxy.h
> >> +++ b/hw/9pfs/9p-proxy.h
> >> @@ -79,7 +79,10 @@ typedef struct {
> >>      uint64_t st_mtim_nsec;
> >>      uint64_t st_ctim_sec;
> >>      uint64_t st_ctim_nsec;
> >> -} ProxyStat;
> >> +} QEMU_PACKED ProxyStat;
> >> +
> >> +#define PROXY_STAT_SZ \
> >> +    (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)  
> >
> > eg.
> >
> >   QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
> >       (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8));  
> 
> or
> 
>      QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
>          proxy_unmarshal_calcsize("qqqdddqqqqqqqqqq"));
> 

The purpose of QEMU_BUILD_BUG_ON(x) is to break the build if the condition
is false.

It expands to something like:

typedef struct { int:(cond) ? -1 : 1; } qemu_build_bug_on__5 
__attribute__((unused));

and causes gcc to fail if cond is 0:

error: negative width in bit-field ‘<anonymous>’

This really only makes sense if cond is constant, otherwise gcc complains with:

error: bit-field ‘<anonymous>’ width not an integer constant

And I can't think of any way of implementing proxy_unmarshal_calcsize() so
that it boils down to a constant at build time. Also, the protocol is stable
and won't ever change: it is ok to compute the sizes once and for all.

> or eventually stricter using some gcc/clang Statement Expressions:
> 
> #define proxy_unmarshal_strict(in_sg, in_sg_sz, offset, fmt, args...) \
>      ({ \
>          ssize_t retval; \
>          retval = v9fs_iov_unmarshal(in_sg, 1, offset, 0, fmt, ##args); \
>          QEMU_BUILD_BUG_ON(in_sg_sz != proxy_unmarshal_calcsize(fmt)); \

This should be an assert() actually, and, again, proxy_unmarshal_calcsize()
cannot be a constant, and I certainly don't want to do anything but comparing
two integers here at runtime.

>          retval; \
>      })
> 
> so we could use:
> 
>      retval = proxy_unmarshal_strict(reply, sizeof(reply), 0, "dd",
>                                      &header.type, &header.size);
> 

I wanted to do something like this initially but I decided not to because
of the reasons exposed above. But if you have a solution, I'll gladly
give a try. :)

Thanks

--
Greg


> >
> >
> > Regards,
> > Daniel
> >

pgppzcVJh2uW8.pgp
Description: OpenPGP digital signature