Libguestfs - Jun 2018 - [PATCH nbdkit 1/2] plugins: nbd: Free h (handle) along error paths.

There are a few other issues that Coverity found, but I believe
all can be ignored ... except one:

We don't set umask anywhere inside nbdkit.  Coverity complains that
this is a problem where we create temporary files, since the result of
mkstemp depends implicitly on the umask value.  I think we might
consider setting umask anyway (eg. to 022) just to make plugin
behaviour more predictable.  What do you think?

Rich.

Found by Coverity.
---
 plugins/nbd/nbd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index b9e72bc..2b5569b 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -465,6 +465,7 @@ nbd_open (int readonly)
   h->fd = socket (AF_UNIX, SOCK_STREAM, 0);
   if (h->fd < 0) {
     nbdkit_error ("socket: %m");
+    free (h);
     return NULL;
   }
   /* We already validated length during nbd_config_complete */
@@ -559,6 +560,7 @@ nbd_open (int readonly)
 
  err:
   close (h->fd);
+  free (h);
   return NULL;
 }
 
-- 
2.16.2

Found by Coverity.
---
 src/crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/crypto.c b/src/crypto.c
index 17a667b..23c5c8f 100644
--- a/src/crypto.c
+++ b/src/crypto.c
@@ -294,7 +294,7 @@ crypto_send (struct connection *conn, const void *vbuf,
size_t len)
 
   while (len > 0) {
     r = gnutls_record_send (*session, buf, len);
-    if (r == -1) {
+    if (r < 0) {
       if (r == GNUTLS_E_INTERRUPTED || r == GNUTLS_E_AGAIN)
         continue;
       return -1;
-- 
2.16.2

On 06/14/2018 08:36 AM, Richard W.M. Jones wrote:
> Found by Coverity.
> ---
>   plugins/nbd/nbd.c | 2 ++
>   1 file changed, 2 insertions(+)
ACK.
> 
> diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
> index b9e72bc..2b5569b 100644
> --- a/plugins/nbd/nbd.c
> +++ b/plugins/nbd/nbd.c
> @@ -465,6 +465,7 @@ nbd_open (int readonly)
>     h->fd = socket (AF_UNIX, SOCK_STREAM, 0);
>     if (h->fd < 0) {
>       nbdkit_error ("socket: %m");
> +    free (h);
>       return NULL;
>     }
>     /* We already validated length during nbd_config_complete */
> @@ -559,6 +560,7 @@ nbd_open (int readonly)
>   
>    err:
>     close (h->fd);
> +  free (h);
>     return NULL;
>   }
>   
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 06/14/2018 08:36 AM, Richard W.M. Jones wrote:
> There are a few other issues that Coverity found, but I believe
> all can be ignored ... except one:
> 
> We don't set umask anywhere inside nbdkit.  Coverity complains that
> this is a problem where we create temporary files, since the result of
> mkstemp depends implicitly on the umask value.  I think we might
> consider setting umask anyway (eg. to 022) just to make plugin
> behaviour more predictable.  What do you think?
Setting umask() is not threadsafe - it must be done up front before any 
threads can be created (and is therefore unsafe to do in a library that 
might be linked into a larger multithreaded program).  But setting a 
sane umask up front seems reasonable to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org