Declare most of the stringsbuf as CLEANUP_FREE_STRINGSBUF, so they are freed completely on stack unwind: use take_stringsbuf() in return places to take away from the stringsbuf its content, and remove all the manual calls to free_stringslen (no more needed now). This requires to not use free_stringslen anymore on failure in the helper functions of stringsbuf, which now leave the content as-is (might be still useful even on error). This allows us to simplify the memory management of stringsbuf's, which are not properly fully freed, fixing memory leaks in some error paths (which were not calling free_stringslen). --- daemon/9p.c | 8 +++----- daemon/available.c | 4 ++-- daemon/blkid.c | 46 ++++++++++++++++++---------------------------- daemon/btrfs.c | 4 ++-- daemon/devsparts.c | 22 +++++----------------- daemon/ext2.c | 4 ++-- daemon/guestfsd.c | 6 ------ daemon/initrd.c | 5 ++--- daemon/inotify.c | 4 ++-- daemon/ldm.c | 10 ++++------ daemon/link.c | 4 ++-- daemon/lvm-filter.c | 17 ++++++----------- daemon/lvm.c | 16 ++++++---------- daemon/md.c | 22 +++++++--------------- daemon/mount.c | 13 ++++--------- 15 files changed, 65 insertions(+), 120 deletions(-) diff --git a/daemon/9p.c b/daemon/9p.c index fefbb71..a9e36d1 100644 --- a/daemon/9p.c +++ b/daemon/9p.c @@ -41,7 +41,7 @@ static char *read_whole_file (const char *filename); char ** do_list_9p (void) { - DECLARE_STRINGSBUF (r); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (r); DIR *dir; @@ -60,7 +60,7 @@ do_list_9p (void) if (end_stringsbuf (&r) == -1) return NULL; - return r.argv; + return take_stringsbuf (&r); } while (1) { @@ -93,7 +93,6 @@ do_list_9p (void) /* Check readdir didn't fail */ if (errno != 0) { reply_with_perror ("readdir: /sys/block"); - free_stringslen (r.argv, r.size); closedir (dir); return NULL; } @@ -101,7 +100,6 @@ do_list_9p (void) /* Close the directory handle */ if (closedir (dir) == -1) { reply_with_perror ("closedir: /sys/block"); - free_stringslen (r.argv, r.size); return NULL; } @@ -113,7 +111,7 @@ do_list_9p (void) if (end_stringsbuf (&r) == -1) return NULL; - return r.argv; + return take_stringsbuf (&r); } /* Read whole file into dynamically allocated array. If there is an diff --git a/daemon/available.c b/daemon/available.c index 9716796..1d0f5a0 100644 --- a/daemon/available.c +++ b/daemon/available.c @@ -53,7 +53,7 @@ char ** do_available_all_groups (void) { size_t i; - DECLARE_STRINGSBUF (groups); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (groups); for (i = 0; optgroups[i].group != NULL; ++i) { if (add_string (&groups, optgroups[i].group) == -1) @@ -63,7 +63,7 @@ do_available_all_groups (void) if (end_stringsbuf (&groups) == -1) return NULL; - return groups.argv; /* caller frees */ + return take_stringsbuf (&groups); /* caller frees */ } /* Search for filesystem in /proc/filesystems, ignoring "nodev". */ diff --git a/daemon/blkid.c b/daemon/blkid.c index 99546b4..1fe5ff9 100644 --- a/daemon/blkid.c +++ b/daemon/blkid.c @@ -138,19 +138,19 @@ blkid_with_p_i_opt (const char *device) int r; CLEANUP_FREE char *out = NULL, *err = NULL; CLEANUP_FREE_STRING_LIST char **lines = NULL; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); r = command (&out, &err, str_blkid, "-c", "/dev/null", "-p", "-i", "-o", "export", device, NULL); if (r == -1) { reply_with_error ("%s", err); - goto error; + return NULL; } /* Split the command output into lines */ lines = split_lines (out); if (lines == NULL) - goto error; + return NULL; /* Parse the output of blkid -p -i -o export: * UUID=b6d83437-c6b4-4bf0-8381-ef3fc3578590 @@ -181,54 +181,44 @@ blkid_with_p_i_opt (const char *device) /* Add the key/value pair to the output */ if (add_string (&ret, line) == -1 || - add_string (&ret, eq) == -1) goto error; + add_string (&ret, eq) == -1) return NULL; } else { fprintf (stderr, "blkid: unexpected blkid output ignored: %s", line); } } - if (end_stringsbuf (&ret) == -1) goto error; - - return ret.argv; - - error: - if (ret.argv) - free_strings (ret.argv); + if (end_stringsbuf (&ret) == -1) return NULL; - return NULL; + return take_stringsbuf (&ret); } static char ** blkid_without_p_i_opt (const char *device) { char *s; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); - if (add_string (&ret, "TYPE") == -1) goto error; + if (add_string (&ret, "TYPE") == -1) return NULL; s = get_blkid_tag (device, "TYPE"); - if (s == NULL) goto error; + if (s == NULL) return NULL; if (add_string_nodup (&ret, s) == -1) - goto error; + return NULL; - if (add_string (&ret, "LABEL") == -1) goto error; + if (add_string (&ret, "LABEL") == -1) return NULL; s = get_blkid_tag (device, "LABEL"); - if (s == NULL) goto error; + if (s == NULL) return NULL; if (add_string_nodup (&ret, s) == -1) - goto error; + return NULL; - if (add_string (&ret, "UUID") == -1) goto error; + if (add_string (&ret, "UUID") == -1) return NULL; s = get_blkid_tag (device, "UUID"); - if (s == NULL) goto error; + if (s == NULL) return NULL; if (add_string_nodup (&ret, s) == -1) - goto error; + return NULL; - if (end_stringsbuf (&ret) == -1) goto error; + if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; - error: - if (ret.argv) - free_stringslen (ret.argv, ret.size); - return NULL; + return take_stringsbuf (&ret); } char ** diff --git a/daemon/btrfs.c b/daemon/btrfs.c index d70565a..811194d 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -1016,7 +1016,7 @@ do_btrfs_subvolume_show (const char *subvolume) CLEANUP_FREE char *err = NULL; CLEANUP_FREE char *out = NULL; char *p, *key = NULL, *value = NULL; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); int r; subvolume_buf = sysroot_path (subvolume); @@ -1147,7 +1147,7 @@ do_btrfs_subvolume_show (const char *subvolume) if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; + return take_stringsbuf (&ret); } int diff --git a/daemon/devsparts.c b/daemon/devsparts.c index 41c728c..c750c64 100644 --- a/daemon/devsparts.c +++ b/daemon/devsparts.c @@ -38,7 +38,7 @@ typedef int (*block_dev_func_t) (const char *dev, struct stringsbuf *r); static char ** foreach_block_device (block_dev_func_t func) { - DECLARE_STRINGSBUF (r); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (r); DIR *dir; int err = 0; struct dirent *d; @@ -89,7 +89,6 @@ foreach_block_device (block_dev_func_t func) /* Check readdir didn't fail */ if (errno != 0) { reply_with_perror ("readdir: /sys/block"); - free_stringslen (r.argv, r.size); closedir (dir); return NULL; } @@ -97,15 +96,11 @@ foreach_block_device (block_dev_func_t func) /* Close the directory handle */ if (closedir (dir) == -1) { reply_with_perror ("closedir: /sys/block"); - free_stringslen (r.argv, r.size); return NULL; } - /* Free the result list on error */ - if (err) { - free_stringslen (r.argv, r.size); + if (err) return NULL; - } /* Sort the devices. */ if (r.size > 0) @@ -116,7 +111,7 @@ foreach_block_device (block_dev_func_t func) return NULL; } - return r.argv; + return take_stringsbuf (&r); } /* Add a device to the list of devices */ @@ -150,7 +145,6 @@ add_partitions (const char *device, struct stringsbuf *r) DIR *dir = opendir (devdir); if (!dir) { reply_with_perror ("opendir: %s", devdir); - free_stringslen (r->argv, r->size); return -1; } @@ -174,7 +168,6 @@ add_partitions (const char *device, struct stringsbuf *r) /* Check if readdir failed */ if (0 != errno) { reply_with_perror ("readdir: %s", devdir); - free_stringslen (r->argv, r->size); closedir (dir); return -1; } @@ -182,7 +175,6 @@ add_partitions (const char *device, struct stringsbuf *r) /* Close the directory handle */ if (closedir (dir) == -1) { reply_with_perror ("closedir: /sys/block/%s", device); - free_stringslen (r->argv, r->size); return -1; } @@ -311,7 +303,7 @@ do_list_disk_labels (void) { DIR *dir = NULL; struct dirent *d; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); dir = opendir (GUESTFSDIR); if (!dir) { @@ -336,14 +328,12 @@ do_list_disk_labels (void) if (asprintf (&path, "%s/%s", GUESTFSDIR, d->d_name) == -1) { reply_with_perror ("asprintf"); - free_stringslen (ret.argv, ret.size); goto error; } rawdev = realpath (path, NULL); if (rawdev == NULL) { reply_with_perror ("realpath: %s", path); - free_stringslen (ret.argv, ret.size); goto error; } @@ -359,14 +349,12 @@ do_list_disk_labels (void) /* Check readdir didn't fail */ if (errno != 0) { reply_with_perror ("readdir: %s", GUESTFSDIR); - free_stringslen (ret.argv, ret.size); goto error; } /* Close the directory handle */ if (closedir (dir) == -1) { reply_with_perror ("closedir: %s", GUESTFSDIR); - free_stringslen (ret.argv, ret.size); dir = NULL; goto error; } @@ -376,7 +364,7 @@ do_list_disk_labels (void) if (end_stringsbuf (&ret) == -1) goto error; - return ret.argv; /* caller frees */ + return take_stringsbuf (&ret); /* caller frees */ error: if (dir) diff --git a/daemon/ext2.c b/daemon/ext2.c index 95a65ae..e556095 100644 --- a/daemon/ext2.c +++ b/daemon/ext2.c @@ -55,7 +55,7 @@ do_tune2fs_l (const char *device) int r; CLEANUP_FREE char *out = NULL, *err = NULL; char *p, *pend, *colon; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); r = command (&out, &err, str_tune2fs, "-l", device, NULL); if (r == -1) { @@ -121,7 +121,7 @@ do_tune2fs_l (const char *device) if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; + return take_stringsbuf (&ret); } int diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index af151bd..15ec6ef 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -510,8 +510,6 @@ add_string_nodup (struct stringsbuf *sb, char *str) new_argv = realloc (sb->argv, sb->alloc * sizeof (char *)); if (new_argv == NULL) { reply_with_perror ("realloc"); - free_stringslen (sb->argv, sb->size); - sb->argv = NULL; free (str); return -1; } @@ -533,8 +531,6 @@ add_string (struct stringsbuf *sb, const char *str) new_str = strdup (str); if (new_str == NULL) { reply_with_perror ("strdup"); - free_stringslen (sb->argv, sb->size); - sb->argv = NULL; return -1; } } @@ -554,8 +550,6 @@ add_sprintf (struct stringsbuf *sb, const char *fs, ...) va_end (args); if (r == -1) { reply_with_perror ("vasprintf"); - free_stringslen (sb->argv, sb->size); - sb->argv = NULL; return -1; } diff --git a/daemon/initrd.c b/daemon/initrd.c index c9fc2dd..21865de 100644 --- a/daemon/initrd.c +++ b/daemon/initrd.c @@ -39,7 +39,7 @@ do_initrd_list (const char *path) { FILE *fp; CLEANUP_FREE char *cmd = NULL; - DECLARE_STRINGSBUF (filenames); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (filenames); CLEANUP_FREE char *filename = NULL; size_t allocsize; ssize_t len; @@ -84,11 +84,10 @@ do_initrd_list (const char *path) ret = WEXITSTATUS (ret); reply_with_error ("pclose: command failed with return code %d", ret); } - free_stringslen (filenames.argv, filenames.size); return NULL; } - return filenames.argv; + return take_stringsbuf (&filenames); } char * diff --git a/daemon/inotify.c b/daemon/inotify.c index 562365b..277c94c 100644 --- a/daemon/inotify.c +++ b/daemon/inotify.c @@ -307,7 +307,7 @@ do_inotify_read (void) char ** do_inotify_files (void) { - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); unsigned int i; FILE *fp = NULL; guestfs_int_inotify_event_list *events; @@ -380,7 +380,7 @@ do_inotify_files (void) goto error; unlink (tempfile); - return ret.argv; + return take_stringsbuf (&ret); error: if (fp != NULL) diff --git a/daemon/ldm.c b/daemon/ldm.c index 71cdf46..0636082 100644 --- a/daemon/ldm.c +++ b/daemon/ldm.c @@ -50,7 +50,7 @@ glob_errfunc (const char *epath, int eerrno) static char ** get_devices (const char *pattern) { - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); glob_t devs; int err; size_t i; @@ -76,12 +76,10 @@ get_devices (const char *pattern) if (end_stringsbuf (&ret) == -1) goto error; globfree (&devs); - return ret.argv; + return take_stringsbuf (&ret); error: globfree (&devs); - if (ret.argv != NULL) - free_stringslen (ret.argv, ret.size); return NULL; } @@ -176,7 +174,7 @@ parse_json (const char *json, const char *func) static char ** json_value_to_string_list (yajl_val node) { - DECLARE_STRINGSBUF (strs); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (strs); yajl_val n; size_t i, len; @@ -194,7 +192,7 @@ json_value_to_string_list (yajl_val node) if (end_stringsbuf (&strs) == -1) return NULL; - return strs.argv; + return take_stringsbuf (&strs); } static char ** diff --git a/daemon/link.c b/daemon/link.c index 43c55f8..3ce54fa 100644 --- a/daemon/link.c +++ b/daemon/link.c @@ -53,7 +53,7 @@ do_internal_readlinklist (const char *path, char *const *names) { int fd_cwd; size_t i; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); CHROOT_IN; fd_cwd = open (path, O_RDONLY|O_DIRECTORY|O_CLOEXEC); @@ -87,7 +87,7 @@ do_internal_readlinklist (const char *path, char *const *names) if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; + return take_stringsbuf (&ret); } int diff --git a/daemon/lvm-filter.c b/daemon/lvm-filter.c index eadc472..6195239 100644 --- a/daemon/lvm-filter.c +++ b/daemon/lvm-filter.c @@ -240,7 +240,7 @@ static char ** make_filter_strings (char *const *devices) { size_t i; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); for (i = 0; devices[i] != NULL; ++i) { /* Because of the way matching works in LVM (yes, they wrote their @@ -254,26 +254,21 @@ make_filter_strings (char *const *devices) size_t slen = strlen (devices[i]); if (add_sprintf (&ret, "a|^%s$|", devices[i]) == -1) - goto error; + return NULL; if (!c_isdigit (devices[i][slen-1])) { /* whole block device */ if (add_sprintf (&ret, "a|^%s[0-9]|", devices[i]) == -1) - goto error; + return NULL; } } if (add_string (&ret, "r|.*|") == -1) - goto error; + return NULL; if (end_stringsbuf (&ret) == -1) - goto error; + return NULL; - return ret.argv; - - error: - if (ret.argv) - free_stringslen (ret.argv, ret.size); - return NULL; + return take_stringsbuf (&ret); } int diff --git a/daemon/lvm.c b/daemon/lvm.c index 2b61357..093e682 100644 --- a/daemon/lvm.c +++ b/daemon/lvm.c @@ -48,7 +48,7 @@ static char ** convert_lvm_output (char *out, const char *prefix) { char *p, *pend; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); size_t len; char buf[256]; char *str; @@ -100,7 +100,7 @@ convert_lvm_output (char *out, const char *prefix) if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; + return take_stringsbuf (&ret); } /* Filter a colon-separated output of @@ -113,7 +113,7 @@ static char ** filter_convert_old_lvs_output (char *out) { char *p, *pend; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); p = out; while (p) { @@ -183,7 +183,7 @@ filter_convert_old_lvs_output (char *out) if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; + return take_stringsbuf (&ret); } char ** @@ -889,7 +889,7 @@ do_lvm_canonical_lv_name (const char *device) char ** do_list_dm_devices (void) { - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); struct dirent *d; DIR *dir; int r; @@ -917,7 +917,6 @@ do_list_dm_devices (void) if (asprintf (&devname, "/dev/mapper/%s", d->d_name) == -1) { reply_with_perror ("asprintf"); - free_stringslen (ret.argv, ret.size); closedir (dir); return NULL; } @@ -925,7 +924,6 @@ do_list_dm_devices (void) /* Ignore dm devices which are LVs. */ r = lv_canonical (devname, NULL); if (r == -1) { - free_stringslen (ret.argv, ret.size); closedir (dir); return NULL; } @@ -942,7 +940,6 @@ do_list_dm_devices (void) /* Did readdir fail? */ if (errno != 0) { reply_with_perror ("readdir: /dev/mapper"); - free_stringslen (ret.argv, ret.size); closedir (dir); return NULL; } @@ -950,7 +947,6 @@ do_list_dm_devices (void) /* Close the directory handle. */ if (closedir (dir) == -1) { reply_with_perror ("closedir: /dev/mapper"); - free_stringslen (ret.argv, ret.size); return NULL; } @@ -962,7 +958,7 @@ do_list_dm_devices (void) if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; + return take_stringsbuf (&ret); } char * diff --git a/daemon/md.c b/daemon/md.c index 5d60b62..66e695a 100644 --- a/daemon/md.c +++ b/daemon/md.c @@ -220,7 +220,7 @@ is_raid_device (const char *dev) char ** do_list_md_devices (void) { - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); glob_t mds; memset (&mds, 0, sizeof mds); @@ -271,12 +271,10 @@ do_list_md_devices (void) if (end_stringsbuf (&ret) == -1) goto error; globfree (&mds); - return ret.argv; + return take_stringsbuf (&ret); error: globfree (&mds); - if (ret.argv != NULL) - free_stringslen (ret.argv, ret.size); return NULL; } @@ -290,19 +288,19 @@ do_md_detail (const char *md) CLEANUP_FREE char *out = NULL, *err = NULL; CLEANUP_FREE_STRING_LIST char **lines = NULL; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); const char *mdadm[] = { str_mdadm, "-D", "--export", md, NULL }; r = commandv (&out, &err, mdadm); if (r == -1) { reply_with_error ("%s", err); - goto error; + return NULL; } /* Split the command output into lines */ lines = split_lines (out); if (lines == NULL) - goto error; + return NULL; /* Parse the output of mdadm -D --export: * MD_LEVEL=raid1 @@ -333,7 +331,7 @@ do_md_detail (const char *md) /* Add the key/value pair to the output */ if (add_string (&ret, line) == -1 || - add_string (&ret, eq) == -1) goto error; + add_string (&ret, eq) == -1) return NULL; } else { /* Ignore lines with no equals sign (shouldn't happen). Log to stderr so * it will show up in LIBGUESTFS_DEBUG. */ @@ -344,13 +342,7 @@ do_md_detail (const char *md) if (end_stringsbuf (&ret) == -1) return NULL; - return ret.argv; - - error: - if (ret.argv != NULL) - free_stringslen (ret.argv, ret.size); - - return NULL; + return take_stringsbuf (&ret); } int diff --git a/daemon/mount.c b/daemon/mount.c index ef51d48..4c83403 100644 --- a/daemon/mount.c +++ b/daemon/mount.c @@ -264,7 +264,7 @@ mounts_or_mountpoints (int mp) { FILE *fp; struct mntent *m; - DECLARE_STRINGSBUF (ret); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (ret); size_t i; int r; @@ -308,10 +308,8 @@ mounts_or_mountpoints (int mp) STRPREFIX (ret.argv[i], "/dev/dm-")) { char *canonical; r = lv_canonical (ret.argv[i], &canonical); - if (r == -1) { - free_stringslen (ret.argv, ret.size); + if (r == -1) return NULL; - } if (r == 1) { free (ret.argv[i]); ret.argv[i] = canonical; @@ -323,7 +321,7 @@ mounts_or_mountpoints (int mp) } } - return ret.argv; + return take_stringsbuf (&ret); } char ** @@ -363,7 +361,7 @@ do_umount_all (void) { FILE *fp; struct mntent *m; - DECLARE_STRINGSBUF (mounts); + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (mounts); size_t i; int r; @@ -417,13 +415,10 @@ do_umount_all (void) r = command (NULL, &err, str_umount, mounts.argv[i], NULL); if (r == -1) { reply_with_error ("umount: %s: %s", mounts.argv[i], err); - free_stringslen (mounts.argv, mounts.size); return -1; } } - free_stringslen (mounts.argv, mounts.size); - return 0; } -- 2.5.5