[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/
--- 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
--- 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
--- 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
--- 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
--- 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
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
--- 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
--- 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
--- 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
--- 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
--- 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
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
--- 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
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
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/
participants (1)
-
Richard W.M. Jones