[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails |
Date: |
Fri, 17 Mar 2017 17:34:29 +0100 |
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