[PATCH 0/4] virtiofsd coverity fixes

[PATCH 0/4] virtiofsd coverity fixes

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  This is a set of fixes that fixes things that coverity pointed out.
Only the last one (the NULL check in do_read) is probably important.

Dave

Dr. David Alan Gilbert (4):
  virtiofsd: Remove fuse_req_getgroups
  virtiofsd: fv_create_listen_socket error path socket leak
  virtiofsd: load_capng missing unlock
  virtiofsd: do_read missing NULL check

 tools/virtiofsd/fuse.h           | 20 --------
 tools/virtiofsd/fuse_lowlevel.c  | 81 ++------------------------------
 tools/virtiofsd/fuse_lowlevel.h  | 21 ---------
 tools/virtiofsd/fuse_virtio.c    |  2 +
 tools/virtiofsd/passthrough_ll.c |  1 +
 5 files changed, 7 insertions(+), 118 deletions(-)

-- 
2.24.1

[PATCH 1/4] virtiofsd: Remove fuse_req_getgroups

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Remove fuse_req_getgroups that's unused in virtiofsd; it came in
from libfuse but we don't actually use it.  It was called from
fuse_getgroups which we previously removed (but had left it's header
in).

Coverity had complained about null termination in it, but removing
it is the easiest answer.

Fixes: Coverity CID: 1413117 (String not null terminated)
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse.h          | 20 ---------
 tools/virtiofsd/fuse_lowlevel.c | 77 ---------------------------------
 tools/virtiofsd/fuse_lowlevel.h | 21 ---------
 3 files changed, 118 deletions(-)

diff --git a/tools/virtiofsd/fuse.h b/tools/virtiofsd/fuse.h
index 7a4c713559..aba13fef2d 100644
--- a/tools/virtiofsd/fuse.h
+++ b/tools/virtiofsd/fuse.h
@@ -1006,26 +1006,6 @@ void fuse_exit(struct fuse *f);
  */
 struct fuse_context *fuse_get_context(void);
 
-/**
- * Get the current supplementary group IDs for the current request
- *
- * Similar to the getgroups(2) system call, except the return value is
- * always the total number of group IDs, even if it is larger than the
- * specified size.
- *
- * The current fuse kernel module in linux (as of 2.6.30) doesn't pass
- * the group list to userspace, hence this function needs to parse
- * "/proc/$TID/task/$TID/status" to get the group IDs.
- *
- * This feature may not be supported on all operating systems.  In
- * such a case this function will return -ENOSYS.
- *
- * @param size size of given array
- * @param list array of group IDs to be filled in
- * @return the total number of supplementary group IDs or -errno on failure
- */
-int fuse_getgroups(int size, gid_t list[]);
-
 /**
  * Check if the current request has already been interrupted
  *
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index de2e2e0c65..01c418aade 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2667,83 +2667,6 @@ int fuse_lowlevel_is_virtio(struct fuse_session *se)
     return !!se->virtio_dev;
 }
 
-#ifdef linux
-int fuse_req_getgroups(fuse_req_t req, int size, gid_t list[])
-{
-    char *buf;
-    size_t bufsize = 1024;
-    char path[128];
-    int ret;
-    int fd;
-    unsigned long pid = req->ctx.pid;
-    char *s;
-
-    sprintf(path, "/proc/%lu/task/%lu/status", pid, pid);
-
-retry:
-    buf = malloc(bufsize);
-    if (buf == NULL) {
-        return -ENOMEM;
-    }
-
-    ret = -EIO;
-    fd = open(path, O_RDONLY);
-    if (fd == -1) {
-        goto out_free;
-    }
-
-    ret = read(fd, buf, bufsize);
-    close(fd);
-    if (ret < 0) {
-        ret = -EIO;
-        goto out_free;
-    }
-
-    if ((size_t)ret == bufsize) {
-        free(buf);
-        bufsize *= 4;
-        goto retry;
-    }
-
-    ret = -EIO;
-    s = strstr(buf, "\nGroups:");
-    if (s == NULL) {
-        goto out_free;
-    }
-
-    s += 8;
-    ret = 0;
-    while (1) {
-        char *end;
-        unsigned long val = strtoul(s, &end, 0);
-        if (end == s) {
-            break;
-        }
-
-        s = end;
-        if (ret < size) {
-            list[ret] = val;
-        }
-        ret++;
-    }
-
-out_free:
-    free(buf);
-    return ret;
-}
-#else /* linux */
-/*
- * This is currently not implemented on other than Linux...
- */
-int fuse_req_getgroups(fuse_req_t req, int size, gid_t list[])
-{
-    (void)req;
-    (void)size;
-    (void)list;
-    return -ENOSYS;
-}
-#endif
-
 void fuse_session_exit(struct fuse_session *se)
 {
     se->exited = 1;
diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
index 138041e5f1..8f6d705b5c 100644
--- a/tools/virtiofsd/fuse_lowlevel.h
+++ b/tools/virtiofsd/fuse_lowlevel.h
@@ -1704,27 +1704,6 @@ void *fuse_req_userdata(fuse_req_t req);
  */
 const struct fuse_ctx *fuse_req_ctx(fuse_req_t req);
 
-/**
- * Get the current supplementary group IDs for the specified request
- *
- * Similar to the getgroups(2) system call, except the return value is
- * always the total number of group IDs, even if it is larger than the
- * specified size.
- *
- * The current fuse kernel module in linux (as of 2.6.30) doesn't pass
- * the group list to userspace, hence this function needs to parse
- * "/proc/$TID/task/$TID/status" to get the group IDs.
- *
- * This feature may not be supported on all operating systems.  In
- * such a case this function will return -ENOSYS.
- *
- * @param req request handle
- * @param size size of given array
- * @param list array of group IDs to be filled in
- * @return the total number of supplementary group IDs or -errno on failure
- */
-int fuse_req_getgroups(fuse_req_t req, int size, gid_t list[]);
-
 /**
  * Callback function for an interrupt
  *
-- 
2.24.1

[PATCH 2/4] virtiofsd: fv_create_listen_socket error path socket leak

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

If we fail when bringing up the socket we can leak the listen_fd;
in practice the daemon will exit so it's not really a problem.

Fixes: Coverity CID 1413121
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 80a6e929df..dd1c605dbf 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -916,6 +916,7 @@ static int fv_create_listen_socket(struct fuse_session *se)
     old_umask = umask(0077);
     if (bind(listen_sock, (struct sockaddr *)&un, addr_len) == -1) {
         fuse_log(FUSE_LOG_ERR, "vhost socket bind: %m\n");
+        close(listen_sock);
         umask(old_umask);
         return -1;
     }
@@ -923,6 +924,7 @@ static int fv_create_listen_socket(struct fuse_session *se)
 
     if (listen(listen_sock, 1) == -1) {
         fuse_log(FUSE_LOG_ERR, "vhost socket listen: %m\n");
+        close(listen_sock);
         return -1;
     }
 
-- 
2.24.1

[PATCH 3/4] virtiofsd: load_capng missing unlock

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Missing unlock in error path.

Fixes: Covertiy CID 1413123
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e6f2399efc..c635fc8820 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -232,6 +232,7 @@ static int load_capng(void)
          */
         cap.saved = capng_save_state();
         if (!cap.saved) {
+            pthread_mutex_unlock(&cap.mutex);
             fuse_log(FUSE_LOG_ERR, "capng_save_state (thread)\n");
             return -EINVAL;
         }
-- 
2.24.1

[PATCH 4/4] virtiofsd: do_read missing NULL check

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Missing a NULL check if the argument fetch fails.

Fixes: Coverity CID 1413119
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 01c418aade..704c0369b2 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1116,6 +1116,10 @@ static void do_read(fuse_req_t req, fuse_ino_t nodeid,
         struct fuse_file_info fi;
 
         arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+        if (!arg) {
+            fuse_reply_err(req, EINVAL);
+            return;
+        }
 
         memset(&fi, 0, sizeof(fi));
         fi.fh = arg->fh;
-- 
2.24.1

Re: [PATCH 1/4] virtiofsd: Remove fuse_req_getgroups

[Philippe Mathieu-Daudé]

On 2/4/20 12:04 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Remove fuse_req_getgroups that's unused in virtiofsd; it came in
> from libfuse but we don't actually use it.  It was called from
> fuse_getgroups which we previously removed (but had left it's header
> in).
> 
> Coverity had complained about null termination in it, but removing
> it is the easiest answer.

:)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Fixes: Coverity CID: 1413117 (String not null terminated)
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   tools/virtiofsd/fuse.h          | 20 ---------
>   tools/virtiofsd/fuse_lowlevel.c | 77 ---------------------------------
>   tools/virtiofsd/fuse_lowlevel.h | 21 ---------
>   3 files changed, 118 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse.h b/tools/virtiofsd/fuse.h
> index 7a4c713559..aba13fef2d 100644
> --- a/tools/virtiofsd/fuse.h
> +++ b/tools/virtiofsd/fuse.h
> @@ -1006,26 +1006,6 @@ void fuse_exit(struct fuse *f);
>    */
>   struct fuse_context *fuse_get_context(void);
>   
> -/**
> - * Get the current supplementary group IDs for the current request
> - *
> - * Similar to the getgroups(2) system call, except the return value is
> - * always the total number of group IDs, even if it is larger than the
> - * specified size.
> - *
> - * The current fuse kernel module in linux (as of 2.6.30) doesn't pass
> - * the group list to userspace, hence this function needs to parse
> - * "/proc/$TID/task/$TID/status" to get the group IDs.
> - *
> - * This feature may not be supported on all operating systems.  In
> - * such a case this function will return -ENOSYS.
> - *
> - * @param size size of given array
> - * @param list array of group IDs to be filled in
> - * @return the total number of supplementary group IDs or -errno on failure
> - */
> -int fuse_getgroups(int size, gid_t list[]);
> -
>   /**
>    * Check if the current request has already been interrupted
>    *
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index de2e2e0c65..01c418aade 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -2667,83 +2667,6 @@ int fuse_lowlevel_is_virtio(struct fuse_session *se)
>       return !!se->virtio_dev;
>   }
>   
> -#ifdef linux
> -int fuse_req_getgroups(fuse_req_t req, int size, gid_t list[])
> -{
> -    char *buf;
> -    size_t bufsize = 1024;
> -    char path[128];
> -    int ret;
> -    int fd;
> -    unsigned long pid = req->ctx.pid;
> -    char *s;
> -
> -    sprintf(path, "/proc/%lu/task/%lu/status", pid, pid);
> -
> -retry:
> -    buf = malloc(bufsize);
> -    if (buf == NULL) {
> -        return -ENOMEM;
> -    }
> -
> -    ret = -EIO;
> -    fd = open(path, O_RDONLY);
> -    if (fd == -1) {
> -        goto out_free;
> -    }
> -
> -    ret = read(fd, buf, bufsize);
> -    close(fd);
> -    if (ret < 0) {
> -        ret = -EIO;
> -        goto out_free;
> -    }
> -
> -    if ((size_t)ret == bufsize) {
> -        free(buf);
> -        bufsize *= 4;
> -        goto retry;
> -    }
> -
> -    ret = -EIO;
> -    s = strstr(buf, "\nGroups:");
> -    if (s == NULL) {
> -        goto out_free;
> -    }
> -
> -    s += 8;
> -    ret = 0;
> -    while (1) {
> -        char *end;
> -        unsigned long val = strtoul(s, &end, 0);
> -        if (end == s) {
> -            break;
> -        }
> -
> -        s = end;
> -        if (ret < size) {
> -            list[ret] = val;
> -        }
> -        ret++;
> -    }
> -
> -out_free:
> -    free(buf);
> -    return ret;
> -}
> -#else /* linux */
> -/*
> - * This is currently not implemented on other than Linux...
> - */
> -int fuse_req_getgroups(fuse_req_t req, int size, gid_t list[])
> -{
> -    (void)req;
> -    (void)size;
> -    (void)list;
> -    return -ENOSYS;
> -}
> -#endif
> -
>   void fuse_session_exit(struct fuse_session *se)
>   {
>       se->exited = 1;
> diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> index 138041e5f1..8f6d705b5c 100644
> --- a/tools/virtiofsd/fuse_lowlevel.h
> +++ b/tools/virtiofsd/fuse_lowlevel.h
> @@ -1704,27 +1704,6 @@ void *fuse_req_userdata(fuse_req_t req);
>    */
>   const struct fuse_ctx *fuse_req_ctx(fuse_req_t req);
>   
> -/**
> - * Get the current supplementary group IDs for the specified request
> - *
> - * Similar to the getgroups(2) system call, except the return value is
> - * always the total number of group IDs, even if it is larger than the
> - * specified size.
> - *
> - * The current fuse kernel module in linux (as of 2.6.30) doesn't pass
> - * the group list to userspace, hence this function needs to parse
> - * "/proc/$TID/task/$TID/status" to get the group IDs.
> - *
> - * This feature may not be supported on all operating systems.  In
> - * such a case this function will return -ENOSYS.
> - *
> - * @param req request handle
> - * @param size size of given array
> - * @param list array of group IDs to be filled in
> - * @return the total number of supplementary group IDs or -errno on failure
> - */
> -int fuse_req_getgroups(fuse_req_t req, int size, gid_t list[]);
> -
>   /**
>    * Callback function for an interrupt
>    *
> 

Re: [PATCH 2/4] virtiofsd: fv_create_listen_socket error path socket leak

[Philippe Mathieu-Daudé]

On 2/4/20 12:04 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If we fail when bringing up the socket we can leak the listen_fd;
> in practice the daemon will exit so it's not really a problem.
> 
> Fixes: Coverity CID 1413121
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   tools/virtiofsd/fuse_virtio.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 80a6e929df..dd1c605dbf 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -916,6 +916,7 @@ static int fv_create_listen_socket(struct fuse_session *se)
>       old_umask = umask(0077);
>       if (bind(listen_sock, (struct sockaddr *)&un, addr_len) == -1) {
>           fuse_log(FUSE_LOG_ERR, "vhost socket bind: %m\n");
> +        close(listen_sock);
>           umask(old_umask);
>           return -1;
>       }
> @@ -923,6 +924,7 @@ static int fv_create_listen_socket(struct fuse_session *se)
>   
>       if (listen(listen_sock, 1) == -1) {
>           fuse_log(FUSE_LOG_ERR, "vhost socket listen: %m\n");
> +        close(listen_sock);
>           return -1;
>       }
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 4/4] virtiofsd: do_read missing NULL check

[Philippe Mathieu-Daudé]

On 2/4/20 12:05 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Missing a NULL check if the argument fetch fails.

Surprisingly all other calls to fuse_mbuf_iter_advance() do the check.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Fixes: Coverity CID 1413119
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   tools/virtiofsd/fuse_lowlevel.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 01c418aade..704c0369b2 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1116,6 +1116,10 @@ static void do_read(fuse_req_t req, fuse_ino_t nodeid,
>           struct fuse_file_info fi;
>   
>           arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> +        if (!arg) {
> +            fuse_reply_err(req, EINVAL);
> +            return;
> +        }
>   
>           memset(&fi, 0, sizeof(fi));
>           fi.fh = arg->fh;
> 

Re: [PATCH 3/4] virtiofsd: load_capng missing unlock

[Philippe Mathieu-Daudé]

Hi David,

On 2/4/20 12:05 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Missing unlock in error path.
> 
> Fixes: Covertiy CID 1413123
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   tools/virtiofsd/passthrough_ll.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index e6f2399efc..c635fc8820 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -232,6 +232,7 @@ static int load_capng(void)
>            */
>           cap.saved = capng_save_state();
>           if (!cap.saved) {
> +            pthread_mutex_unlock(&cap.mutex);
>               fuse_log(FUSE_LOG_ERR, "capng_save_state (thread)\n");
>               return -EINVAL;
>           }
> 

What about moving the unlock call?

-- >8 --
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -231,11 +231,11 @@ static int load_capng(void)
           * so make another.
           */
          cap.saved = capng_save_state();
+        pthread_mutex_unlock(&cap.mutex);
          if (!cap.saved) {
              fuse_log(FUSE_LOG_ERR, "capng_save_state (thread)\n");
              return -EINVAL;
          }
-        pthread_mutex_unlock(&cap.mutex);

          /*
           * We want to use the loaded state for our pid,
---

Re: [PATCH 3/4] virtiofsd: load_capng missing unlock

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Hi David,
> 
> On 2/4/20 12:05 PM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Missing unlock in error path.
> > 
> > Fixes: Covertiy CID 1413123
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >   tools/virtiofsd/passthrough_ll.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index e6f2399efc..c635fc8820 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -232,6 +232,7 @@ static int load_capng(void)
> >            */
> >           cap.saved = capng_save_state();
> >           if (!cap.saved) {
> > +            pthread_mutex_unlock(&cap.mutex);
> >               fuse_log(FUSE_LOG_ERR, "capng_save_state (thread)\n");
> >               return -EINVAL;
> >           }
> > 
> 
> What about moving the unlock call?
> 
> -- >8 --
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -231,11 +231,11 @@ static int load_capng(void)
>           * so make another.
>           */
>          cap.saved = capng_save_state();
> +        pthread_mutex_unlock(&cap.mutex);
>          if (!cap.saved) {

I don't think I can legally check cap.saved there if I've already
unlocked

>              fuse_log(FUSE_LOG_ERR, "capng_save_state (thread)\n");
>              return -EINVAL;
>          }
> -        pthread_mutex_unlock(&cap.mutex);
> 
>          /*
>           * We want to use the loaded state for our pid,
> ---
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/4] virtiofsd: load_capng missing unlock

[Philippe Mathieu-Daudé]

On 2/4/20 4:44 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> Hi David,
>>
>> On 2/4/20 12:05 PM, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> Missing unlock in error path.
>>>
>>> Fixes: Covertiy CID 1413123
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>>    tools/virtiofsd/passthrough_ll.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
>>> index e6f2399efc..c635fc8820 100644
>>> --- a/tools/virtiofsd/passthrough_ll.c
>>> +++ b/tools/virtiofsd/passthrough_ll.c
>>> @@ -232,6 +232,7 @@ static int load_capng(void)
>>>             */
>>>            cap.saved = capng_save_state();
>>>            if (!cap.saved) {
>>> +            pthread_mutex_unlock(&cap.mutex);
>>>                fuse_log(FUSE_LOG_ERR, "capng_save_state (thread)\n");
>>>                return -EINVAL;
>>>            }
>>>
>>
>> What about moving the unlock call?
>>
>> -- >8 --
>> --- a/tools/virtiofsd/passthrough_ll.c
>> +++ b/tools/virtiofsd/passthrough_ll.c
>> @@ -231,11 +231,11 @@ static int load_capng(void)
>>            * so make another.
>>            */
>>           cap.saved = capng_save_state();
>> +        pthread_mutex_unlock(&cap.mutex);
>>           if (!cap.saved) {
> 
> I don't think I can legally check cap.saved there if I've already
> unlocked

Sorry I was with low sugar... I read it as a copy.

The patch is fine:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>               fuse_log(FUSE_LOG_ERR, "capng_save_state (thread)\n");
>>               return -EINVAL;
>>           }
>> -        pthread_mutex_unlock(&cap.mutex);
>>
>>           /*
>>            * We want to use the loaded state for our pid,
>> ---
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 

Re: [PATCH 0/4] virtiofsd coverity fixes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

On Tue, Feb 04, 2020 at 11:04:57AM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Hi,
>   This is a set of fixes that fixes things that coverity pointed out.
> Only the last one (the NULL check in do_read) is probably important.
> 
> Dave
> 
> Dr. David Alan Gilbert (4):
>   virtiofsd: Remove fuse_req_getgroups
>   virtiofsd: fv_create_listen_socket error path socket leak
>   virtiofsd: load_capng missing unlock
>   virtiofsd: do_read missing NULL check
> 
>  tools/virtiofsd/fuse.h           | 20 --------
>  tools/virtiofsd/fuse_lowlevel.c  | 81 ++------------------------------
>  tools/virtiofsd/fuse_lowlevel.h  | 21 ---------
>  tools/virtiofsd/fuse_virtio.c    |  2 +
>  tools/virtiofsd/passthrough_ll.c |  1 +
>  5 files changed, 7 insertions(+), 118 deletions(-)
> 
> -- 
> 2.24.1
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]