* [PATCH 1/4] virtiofsd: Remove fuse_req_getgroups
2020-02-04 11:04 [PATCH 0/4] virtiofsd coverity fixes Dr. David Alan Gilbert (git)
@ 2020-02-04 11:04 ` Dr. David Alan Gilbert (git)
2020-02-04 11:59 ` Philippe Mathieu-Daudé
2020-02-04 11:04 ` [PATCH 2/4] virtiofsd: fv_create_listen_socket error path socket leak Dr. David Alan Gilbert (git)
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2020-02-04 11:04 UTC (permalink / raw)
To: qemu-devel, stefanha
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/4] virtiofsd: Remove fuse_req_getgroups
2020-02-04 11:04 ` [PATCH 1/4] virtiofsd: Remove fuse_req_getgroups Dr. David Alan Gilbert (git)
@ 2020-02-04 11:59 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-04 11:59 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel, stefanha
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
> *
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] virtiofsd: fv_create_listen_socket error path socket leak
2020-02-04 11:04 [PATCH 0/4] virtiofsd coverity fixes Dr. David Alan Gilbert (git)
2020-02-04 11:04 ` [PATCH 1/4] virtiofsd: Remove fuse_req_getgroups Dr. David Alan Gilbert (git)
@ 2020-02-04 11:04 ` Dr. David Alan Gilbert (git)
2020-02-04 12:00 ` Philippe Mathieu-Daudé
2020-02-04 11:05 ` [PATCH 3/4] virtiofsd: load_capng missing unlock Dr. David Alan Gilbert (git)
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2020-02-04 11:04 UTC (permalink / raw)
To: qemu-devel, stefanha
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] virtiofsd: fv_create_listen_socket error path socket leak
2020-02-04 11:04 ` [PATCH 2/4] virtiofsd: fv_create_listen_socket error path socket leak Dr. David Alan Gilbert (git)
@ 2020-02-04 12:00 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-04 12:00 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel, stefanha
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] virtiofsd: load_capng missing unlock
2020-02-04 11:04 [PATCH 0/4] virtiofsd coverity fixes Dr. David Alan Gilbert (git)
2020-02-04 11:04 ` [PATCH 1/4] virtiofsd: Remove fuse_req_getgroups Dr. David Alan Gilbert (git)
2020-02-04 11:04 ` [PATCH 2/4] virtiofsd: fv_create_listen_socket error path socket leak Dr. David Alan Gilbert (git)
@ 2020-02-04 11:05 ` Dr. David Alan Gilbert (git)
2020-02-04 12:05 ` Philippe Mathieu-Daudé
2020-02-04 11:05 ` [PATCH 4/4] virtiofsd: do_read missing NULL check Dr. David Alan Gilbert (git)
2020-02-05 14:31 ` [PATCH 0/4] virtiofsd coverity fixes Stefan Hajnoczi
4 siblings, 1 reply; 12+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2020-02-04 11:05 UTC (permalink / raw)
To: qemu-devel, stefanha
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/4] virtiofsd: load_capng missing unlock
2020-02-04 11:05 ` [PATCH 3/4] virtiofsd: load_capng missing unlock Dr. David Alan Gilbert (git)
@ 2020-02-04 12:05 ` Philippe Mathieu-Daudé
2020-02-04 15:44 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-04 12:05 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel, stefanha
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,
---
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/4] virtiofsd: load_capng missing unlock
2020-02-04 12:05 ` Philippe Mathieu-Daudé
@ 2020-02-04 15:44 ` Dr. David Alan Gilbert
2020-02-04 16:06 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 12+ messages in thread
From: Dr. David Alan Gilbert @ 2020-02-04 15:44 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, stefanha
* 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/4] virtiofsd: load_capng missing unlock
2020-02-04 15:44 ` Dr. David Alan Gilbert
@ 2020-02-04 16:06 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-04 16:06 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, stefanha
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
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/4] virtiofsd: do_read missing NULL check
2020-02-04 11:04 [PATCH 0/4] virtiofsd coverity fixes Dr. David Alan Gilbert (git)
` (2 preceding siblings ...)
2020-02-04 11:05 ` [PATCH 3/4] virtiofsd: load_capng missing unlock Dr. David Alan Gilbert (git)
@ 2020-02-04 11:05 ` Dr. David Alan Gilbert (git)
2020-02-04 12:03 ` Philippe Mathieu-Daudé
2020-02-05 14:31 ` [PATCH 0/4] virtiofsd coverity fixes Stefan Hajnoczi
4 siblings, 1 reply; 12+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2020-02-04 11:05 UTC (permalink / raw)
To: qemu-devel, stefanha
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 4/4] virtiofsd: do_read missing NULL check
2020-02-04 11:05 ` [PATCH 4/4] virtiofsd: do_read missing NULL check Dr. David Alan Gilbert (git)
@ 2020-02-04 12:03 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-04 12:03 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel, stefanha
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;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/4] virtiofsd coverity fixes
2020-02-04 11:04 [PATCH 0/4] virtiofsd coverity fixes Dr. David Alan Gilbert (git)
` (3 preceding siblings ...)
2020-02-04 11:05 ` [PATCH 4/4] virtiofsd: do_read missing NULL check Dr. David Alan Gilbert (git)
@ 2020-02-05 14:31 ` Stefan Hajnoczi
4 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2020-02-05 14:31 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, stefanha
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread