On Fri, 17 Mar 2017 12:06:39 -0300 Philippe Mathieu-Daudé <f4...@amsat.org> 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 <gr...@kaod.org> > >> --- > >> 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 > >
pgpRdA2b2_ViU.pgp
Description: OpenPGP digital signature