thr3ads.net - Libguestfs - [Libguestfs] [PATCH 00/13] Fix errors found using Coverity static analyzer. [Jun 2011]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2011-Jun-09 10:16 UTC

[Libguestfs] [PATCH 00/13] Fix errors found using Coverity static analyzer.

I ran the Coverity static analyzer[1] on libguestfs, and fixed many
errors as a result.

Coverity found some errors in gnulib, but it doesn't seem to be worth
following those up since the version of gnulib we are using is so old.

There are a couple more errors (possibly 1 false-positive) which I'm
going to send in a separate email.

BTW all the errors found by Coverity were in the daemon code.  It's
unclear to me if Coverity even ran on the library code (there's no
listing of files that I can find).  However I can't see any reason why
it wouldn't have run on the library files and bindings.

Rich.

[1] http://www.coverity.com/

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 01/13] Coverity: Remove unreachable code.

---
 daemon/base64.c |    2 --
 daemon/debug.c  |    4 ----
 daemon/tar.c    |    2 --
 daemon/upload.c |    4 ----
 4 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/daemon/base64.c b/daemon/base64.c
index f55e1f5..8aca5b7 100644
--- a/daemon/base64.c
+++ b/daemon/base64.c
@@ -86,8 +86,6 @@ do_base64_in (const char *file)
   }
 
   if (pclose (fp) != 0) {
-    if (r == -1)                /* if r == 0, file transfer ended already */
-      cancel_receive ();
     reply_with_error ("base64 subcommand failed on file: %s", file);
     return -1;
   }
diff --git a/daemon/debug.c b/daemon/debug.c
index cd3e8a5..0ade2a4 100644
--- a/daemon/debug.c
+++ b/daemon/debug.c
@@ -549,10 +549,6 @@ do_debug_upload (const char *filename, int mode)
   }
 
   if (close (fd) == -1) {
-    int err = errno;
-    if (r == -1)                /* if r == 0, file transfer ended already */
-      cancel_receive ();
-    errno = err;
     reply_with_perror ("close: %s", filename);
     return -1;
   }
diff --git a/daemon/tar.c b/daemon/tar.c
index 39fce8c..74d7b05 100644
--- a/daemon/tar.c
+++ b/daemon/tar.c
@@ -127,8 +127,6 @@ do_tXz_in (const char *dir, const char *filter)
   }
 
   if (pclose (fp) != 0) {
-    if (r == -1)                /* if r == 0, file transfer ended already */
-      r = cancel_receive ();
     char *errstr = read_error_file ();
     reply_with_error ("tar subcommand failed on directory: %s: %s",
                       dir, errstr);
diff --git a/daemon/upload.c b/daemon/upload.c
index 2c42969..a1a37ae 100644
--- a/daemon/upload.c
+++ b/daemon/upload.c
@@ -103,10 +103,6 @@ upload (const char *filename, int flags, int64_t offset)
   }
 
   if (close (data.fd) == -1) {
-    err = errno;
-    if (r == -1)                /* if r == 0, file transfer ended already */
-      r = cancel_receive ();
-    errno = err;
     reply_with_perror ("close: %s", filename);
     return -1;
   }
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 02/13] Coverity: Don't call free_strings (NULL).

---
 daemon/link.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/daemon/link.c b/daemon/link.c
index 3766d8c..1049640 100644
--- a/daemon/link.c
+++ b/daemon/link.c
@@ -76,7 +76,6 @@ do_readlinklist (const char *path, char *const *names)
     r = readlinkat (fd_cwd, names[i], link, sizeof link);
     if (r >= PATH_MAX) {
       reply_with_perror ("readlinkat: returned link is too long");
-      free_strings (ret);
       close (fd_cwd);
       return NULL;
     }
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 03/13] Coverity: Avoid calling sort_strings (NULL, 0) on empty list.

---
 daemon/devsparts.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/daemon/devsparts.c b/daemon/devsparts.c
index 1781def..c8f0256 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -105,8 +105,9 @@ foreach_block_device (block_dev_func_t func)
     return NULL;
   }
 
-  /* Sort the devices */
-  sort_strings (r, size);
+  /* Sort the devices.  Note that r might be NULL if there are no devices. */
+  if (r != NULL)
+    sort_strings (r, size);
 
   /* NULL terminate the list */
   if (add_string (&r, &size, &alloc, NULL) == -1) {
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 04/13] Coverity: Don't close fd_cwd if fd_cwd == -1.

---
 daemon/realpath.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/daemon/realpath.c b/daemon/realpath.c
index 607381b..592e12c 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -179,7 +179,8 @@ do_case_sensitive_path (const char *path)
     }
   }
 
-  close (fd_cwd);
+  if (fd_cwd >= 0)
+    close (fd_cwd);
 
   ret[next] = '\0';
   char *retp = strdup (ret);
@@ -190,6 +191,8 @@ do_case_sensitive_path (const char *path)
   return retp;                  /* caller frees */
 
  error:
-  close (fd_cwd);
+  if (fd_cwd >= 0)
+    close (fd_cwd);
+
   return NULL;
 }
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 05/13] Coverity: Check return value of malloc.

---
 daemon/glob.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/daemon/glob.c b/daemon/glob.c
index e94e4aa..f2815ec 100644
--- a/daemon/glob.c
+++ b/daemon/glob.c
@@ -40,6 +40,10 @@ do_glob_expand (const char *pattern)
     char **rv;
 
     rv = malloc (sizeof (char *) * 1);
+    if (rv == NULL) {
+      reply_with_perror ("malloc");
+      return NULL;
+    }
     rv[0] = NULL;
     return rv;			/* Caller frees. */
   }
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 06/13] Coverity: Check return value of sysroot_path.

For some reason we were checking the parameter!
---
 daemon/mount.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/daemon/mount.c b/daemon/mount.c
index a379d39..098283a 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -352,7 +352,7 @@ do_mount_loop (const char *file, const char *mountpoint)
   }
 
   buf = sysroot_path (file);
-  if (!file) {
+  if (!buf) {
     reply_with_perror ("malloc");
     free (mp);
     return -1;
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 07/13] Coverity: Don't leak error strings.

---
 daemon/blockdev.c |    2 ++
 daemon/fsck.c     |    1 +
 daemon/mount.c    |    2 ++
 daemon/ntfs.c     |    3 +++
 daemon/parted.c   |    9 ++++++---
 5 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/daemon/blockdev.c b/daemon/blockdev.c
index 3df1807..1afb4b8 100644
--- a/daemon/blockdev.c
+++ b/daemon/blockdev.c
@@ -68,11 +68,13 @@ call_blockdev (const char *device, const char *switc, int
extraarg, int prints)
     if (sscanf (out, "%" SCNi64, &rv) != 1) {
       reply_with_error ("%s: expected output, but got nothing",
argv[0]);
       free (out);
+      free (err);
       return -1;
     }
   }
 
   free (out);
+  free (err);
 
   return rv;
 }
diff --git a/daemon/fsck.c b/daemon/fsck.c
index e452adc..7a36440 100644
--- a/daemon/fsck.c
+++ b/daemon/fsck.c
@@ -39,5 +39,6 @@ do_fsck (const char *fstype, const char *device)
     return -1;
   }
 
+  free (err);
   return r;
 }
diff --git a/daemon/mount.c b/daemon/mount.c
index 098283a..be289da 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -116,6 +116,7 @@ do_mount_vfs (const char *options, const char *vfstype,
     return -1;
   }
 
+  free (error);
   return 0;
 }
 
@@ -367,6 +368,7 @@ do_mount_loop (const char *file, const char *mountpoint)
     return -1;
   }
 
+  free (error);
   return 0;
 }
 
diff --git a/daemon/ntfs.c b/daemon/ntfs.c
index a25fc38..909ea18 100644
--- a/daemon/ntfs.c
+++ b/daemon/ntfs.c
@@ -56,6 +56,7 @@ do_ntfs_3g_probe (int rw, const char *device)
     return -1;
   }
 
+  free (err);
   return r;
 }
 
@@ -72,6 +73,7 @@ do_ntfsresize (const char *device)
     return -1;
   }
 
+  free (err);
   return 0;
 }
 
@@ -92,5 +94,6 @@ do_ntfsresize_size (const char *device, int64_t size)
     return -1;
   }
 
+  free (err);
   return 0;
 }
diff --git a/daemon/parted.c b/daemon/parted.c
index b9b138e..d52ad2c 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -292,13 +292,16 @@ test_parted_m_opt (void)
   if (r == -1) {
     /* Test failed, eg. missing or completely unusable parted binary. */
     reply_with_error ("could not run 'parted' command");
+    free (err);
     return -1;
   }
 
   if (err && strstr (err, "invalid option -- m"))
-    return result = 0;
-
-  return result = 1;
+    result = 0;
+  else
+    result = 1;
+  free (err);
+  return result;
 }
 
 static char *
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 08/13] Coverity: Don't leak argv arrays.

---
 daemon/lvm.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/daemon/lvm.c b/daemon/lvm.c
index 284e580..dbca20b 100644
--- a/daemon/lvm.c
+++ b/daemon/lvm.c
@@ -223,10 +223,12 @@ do_vgcreate (const char *volgroup, char *const *physvols)
   if (r == -1) {
     reply_with_error ("%s", err);
     free (err);
+    free (argv);
     return -1;
   }
 
   free (err);
+  free (argv);
 
   udev_settle ();
 
@@ -512,10 +514,12 @@ do_vg_activate (int activate, char *const *volgroups)
   if (r == -1) {
     reply_with_error ("vgchange: %s", err);
     free (err);
+    free (argv);
     return -1;
   }
 
   free (err);
+  free (argv);
 
   udev_settle ();
 
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 09/13] Coverity: Close directory handle along error paths.

---
 daemon/devsparts.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/daemon/devsparts.c b/daemon/devsparts.c
index c8f0256..52a6d30 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -89,6 +89,7 @@ foreach_block_device (block_dev_func_t func)
   if(0 != errno) {
     reply_with_perror ("readdir: /sys/block");
     free_stringslen(r, size);
+    closedir (dir);
     return NULL;
   }
 
@@ -175,6 +176,7 @@ add_partitions(const char *device,
   if(0 != errno) {
       reply_with_perror ("readdir: %s", devdir);
       free_stringslen(*r, *size);
+      closedir (dir);
       return -1;
   }
 
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 10/13] Coverity: Ensure fp is closed along all error paths.

---
 daemon/inotify.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/daemon/inotify.c b/daemon/inotify.c
index 8e8b690..c8862e5 100644
--- a/daemon/inotify.c
+++ b/daemon/inotify.c
@@ -314,7 +314,7 @@ do_inotify_files (void)
   char **ret = NULL;
   int size = 0, alloc = 0;
   unsigned int i;
-  FILE *fp;
+  FILE *fp = NULL;
   guestfs_int_inotify_event_list *events;
   char buf[PATH_MAX];
 
@@ -361,13 +361,12 @@ do_inotify_files (void)
     if (len > 0 && buf[len-1] == '\n')
       buf[len-1] = '\0';
 
-    if (add_string (&ret, &size, &alloc, buf) == -1) {
-      fclose (fp);
+    if (add_string (&ret, &size, &alloc, buf) == -1)
       goto error;
-    }
   }
 
   fclose (fp);
+  fp = NULL;
 
   if (add_string (&ret, &size, &alloc, NULL) == -1)
     goto error;
@@ -376,6 +375,9 @@ do_inotify_files (void)
   return ret;
 
  error:
+  if (fp != NULL)
+    fclose (fp);
+
   unlink ("/tmp/inotify");
   return NULL;
 #else
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 11/13] Coverity: Missing return on error path.

---
 daemon/guestfsd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 448f3b4..ceadfdb 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -429,6 +429,7 @@ add_string (char ***argv, int *size, int *alloc, const char
*str)
     if (new_str == NULL) {
       reply_with_perror ("strdup");
       free_strings (*argv);
+      return -1;
     }
   } else
     new_str = NULL;
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 12/13] Coverity: Don't return freed pointers from command* along error path.

If the external command failed to run, we could free up the allocated
*stdoutput and *stderror pointers, but then return those freed
pointers to the caller.  The caller usually tries to print and free
*stderror, so this is a serious error.

Instead, return *stdoutput as NULL, and *stderror pointing to a
generic error message.
---
 daemon/guestfsd.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index ceadfdb..116a6b9 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -779,8 +779,20 @@ commandrvf (char **stdoutput, char **stderror, int flags,
 
       perror ("select");
     quit:
-      if (stdoutput) free (*stdoutput);
-      if (stderror) free (*stderror);
+      if (stdoutput) {
+        free (*stdoutput);
+        *stdoutput = NULL;
+      }
+      if (stderror) {
+        free (*stderror);
+        /* Need to return non-NULL *stderror here since most callers
+         * will try to print and then free the err string.
+         * Unfortunately recovery from strdup failure here is not
+         * possible.
+         */
+        *stderror = strdup ("error running external command, "
+                            "see debug output for details");
+      }
       close (so_fd[0]);
       close (se_fd[0]);
       waitpid (pid, NULL, 0);
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:17 UTC

head link

[Libguestfs] [PATCH 13/13] daemon: Keep Coverity happy by ignoring some return values.

---
 daemon/command.c  |   18 +++++++++++++-----
 daemon/guestfsd.c |    2 +-
 daemon/swap.c     |    4 +++-
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/daemon/command.c b/daemon/command.c
index 5a194a4..ef23695 100644
--- a/daemon/command.c
+++ b/daemon/command.c
@@ -26,6 +26,14 @@
 #include "daemon.h"
 #include "actions.h"
 
+#include "ignore-value.h"
+
+static inline void
+umount_ignore_fail (const char *path)
+{
+  ignore_value (command (NULL, NULL, "umount", path, NULL));
+}
+
 char *
 do_command (char *const *argv)
 {
@@ -88,11 +96,11 @@ do_command (char *const *argv)
   r = commandv (&out, &err, (const char * const *) argv);
   CHROOT_OUT;
 
-  if (sys_ok) command (NULL, NULL, "umount", sysroot_sys, NULL);
-  if (selinux_ok) command (NULL, NULL, "umount", sysroot_selinux,
NULL);
-  if (proc_ok) command (NULL, NULL, "umount", sysroot_proc, NULL);
-  if (dev_pts_ok) command (NULL, NULL, "umount", sysroot_dev_pts,
NULL);
-  if (dev_ok) command (NULL, NULL, "umount", sysroot_dev, NULL);
+  if (sys_ok) umount_ignore_fail (sysroot_sys);
+  if (selinux_ok) umount_ignore_fail (sysroot_selinux);
+  if (proc_ok) umount_ignore_fail (sysroot_proc);
+  if (dev_pts_ok) umount_ignore_fail (sysroot_dev_pts);
+  if (dev_ok) umount_ignore_fail (sysroot_dev);
 
   free (sysroot_dev);
   free (sysroot_dev_pts);
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 116a6b9..eb1e82b 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -689,7 +689,7 @@ commandrvf (char **stdoutput, char **stderror, int flags,
       close (stdin_fd[1]);
     } else {
       /* Set stdin to /dev/null (ignore failure) */
-      open ("/dev/null", O_RDONLY);
+      ignore_value (open ("/dev/null", O_RDONLY));
     }
     close (so_fd[0]);
     close (se_fd[0]);
diff --git a/daemon/swap.c b/daemon/swap.c
index 9077814..99fb563 100644
--- a/daemon/swap.c
+++ b/daemon/swap.c
@@ -28,6 +28,8 @@
 #include "actions.h"
 #include "optgroups.h"
 
+#include "ignore-value.h"
+
 /* Confirmed this is true for Linux swap partitions from the Linux sources. */
 #define SWAP_LABEL_MAX 16
 
@@ -42,7 +44,7 @@ optgroup_linuxfsuuid_available (void)
   int av;
 
   /* Ignore return code - mkswap --help *will* fail. */
-  command (NULL, &err, "mkswap", "--help", NULL);
+  ignore_value (command (NULL, &err, "mkswap",
"--help", NULL));
 
   av = strstr (err, "-U") != NULL;
   free (err);
-- 
1.7.5.1

Richard W.M. Jones

2011-Jun-09 10:36 UTC

head link

[Libguestfs] [PATCH 00/13] Fix errors found using Coverity static analyzer.

On Thu, Jun 09, 2011 at 11:16:59AM +0100, Richard W.M. Jones
wrote:> BTW all the errors found by Coverity were in the daemon code.  It's
> unclear to me if Coverity even ran on the library code (there's no
> listing of files that I can find).  However I can't see any reason why
> it wouldn't have run on the library files and bindings.
The analysis log file does mention many functions in the library and
bindings, so it does appear to be running over that part of the code.
I'm not sure why there are absolutely no errors found though -- I'm
sure we're not that good.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

Richard W.M. Jones

2011-Jun-09 11:19 UTC

head link

[Libguestfs] [PATCH 00/13] Fix errors found using Coverity static analyzer.

The test suite passes and they all seem like obvious fixes, so
I'm going to push all these now.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/

Reasonably Related Threads

Search for more seemingly similar threads

Libguestfs - Jun 2011 - [PATCH 00/13] Fix errors found using Coverity static analyzer.

[Libguestfs] [PATCH 00/13] Fix errors found using Coverity static analyzer.

[Libguestfs] [PATCH 01/13] Coverity: Remove unreachable code.

[Libguestfs] [PATCH 02/13] Coverity: Don't call free_strings (NULL).

[Libguestfs] [PATCH 03/13] Coverity: Avoid calling sort_strings (NULL, 0) on empty list.

[Libguestfs] [PATCH 04/13] Coverity: Don't close fd_cwd if fd_cwd == -1.

[Libguestfs] [PATCH 05/13] Coverity: Check return value of malloc.

[Libguestfs] [PATCH 06/13] Coverity: Check return value of sysroot_path.

[Libguestfs] [PATCH 07/13] Coverity: Don't leak error strings.

[Libguestfs] [PATCH 08/13] Coverity: Don't leak argv arrays.

[Libguestfs] [PATCH 09/13] Coverity: Close directory handle along error paths.

[Libguestfs] [PATCH 10/13] Coverity: Ensure fp is closed along all error paths.

[Libguestfs] [PATCH 11/13] Coverity: Missing return on error path.

[Libguestfs] [PATCH 12/13] Coverity: Don't return freed pointers from command* along error path.

[Libguestfs] [PATCH 13/13] daemon: Keep Coverity happy by ignoring some return values.

[Libguestfs] [PATCH 00/13] Fix errors found using Coverity static analyzer.

[Libguestfs] [PATCH 00/13] Fix errors found using Coverity static analyzer.

Reasonably Related Threads