hw/9pfs/9p-proxy.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
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;
}
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; > } > > >
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; > > } > > > > > >
© 2016 - 2024 Red Hat, Inc.