* [Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails
@ 2017-02-06 17:20 Greg Kurz
2017-02-11 4:25 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 3+ messages in thread
From: Greg Kurz @ 2017-02-06 17:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Eric Blake, Greg Kurz, Aneesh Kumar K.V
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 <groug@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);
/*
* 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);
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);
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);
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);
return 0;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails
2017-02-06 17:20 [Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails Greg Kurz
@ 2017-02-11 4:25 ` Philippe Mathieu-Daudé
2017-02-11 9:47 ` Greg Kurz
0 siblings, 1 reply; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-02-11 4:25 UTC (permalink / raw)
To: Greg Kurz, qemu-devel; +Cc: Aneesh Kumar K.V
Hi Greg,
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 <groug@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);
> /*
> * 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));
> 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;
> }
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails
2017-02-11 4:25 ` Philippe Mathieu-Daudé
@ 2017-02-11 9:47 ` Greg Kurz
0 siblings, 0 replies; 3+ messages in thread
From: Greg Kurz @ 2017-02-11 9:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, Aneesh Kumar K.V
[-- Attachment #1: Type: text/plain, Size: 4979 bytes --]
On Sat, 11 Feb 2017 01:25:17 -0300
Philippe Mathieu-Daudé <f4bug@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 <groug@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;
> > }
> >
> >
> >
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-02-11 9:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-06 17:20 [Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails Greg Kurz
2017-02-11 4:25 ` Philippe Mathieu-Daudé
2017-02-11 9:47 ` Greg Kurz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).