[PATCH nbdkit 0/2] Fix a couple of problems found by Coverity.
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
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
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:
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
participants (2)
-
Eric Blake -
Richard W.M. Jones