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

Greg Kurz posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/148640163738.20116.15256467457494672940.stgit@bahia
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
hw/9pfs/9p-proxy.c |   22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
[Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails
Posted by Greg Kurz 7 years, 1 month ago
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;
 }
 


Re: [Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails
Posted by Philippe Mathieu-Daudé 7 years, 1 month ago
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;
>  }
>
>
>

Re: [Qemu-devel] [PATCH] 9pfs: proxy: assert if unmarshal fails
Posted by Greg Kurz 7 years, 1 month ago
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;
> >  }
> >
> >
> >