Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value

> -----Original Message----- > From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On > Behalf Of Pino Toscano > Sent: Wednesday, June 17, 2015 10:44 PM > To: libguestfs(a)redhat.com > Subject: Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return > value > > On Wednesday 17 June 2015 16:19:32 Chen Hanxiao wrote: > > don't return a value which is to be freed. > > > > v4: use strndup > > v3: v3: fix test case failure > > Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com&gt; > > --- > > daemon/btrfs.c | 26 +++++++++++++++++--------- > > 1 file changed, 17 insertions(+), 9 deletions(-) > > > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > > index 8d03caa..5361984 100644 > > --- a/daemon/btrfs.c > > +++ b/daemon/btrfs.c > > @@ -409,7 +409,7 @@ umount (char *fs_buf, const mountable_t *fs) > > guestfs_int_btrfssubvolume_list * > > do_btrfs_subvolume_list (const mountable_t *fs) > > { > > - char **lines; > > + CLEANUP_FREE_STRING_LIST char **lines = NULL; > > size_t i = 0; > > const size_t MAX_ARGS = 64; > > const char *argv[MAX_ARGS]; > > @@ -472,7 +472,7 @@ do_btrfs_subvolume_list (const mountable_t *fs) > > ret = malloc (sizeof *ret); > > if (!ret) { > > reply_with_perror ("malloc"); > > - goto error; > > + return NULL; > > } > > > > ret->guestfs_int_btrfssubvolume_list_len = nr_subvolumes; > > @@ -480,7 +480,8 @@ do_btrfs_subvolume_list (const mountable_t *fs) > > calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume)); > > if (ret->guestfs_int_btrfssubvolume_list_val == NULL) { > > reply_with_perror ("malloc"); > > - goto error; > > + free (ret); > > + return NULL; > > } > > You don't need the change here, if later in the "error:" label you keep > the "if (ret)" condition. > If we succeeded at malloc(3) but failed at calloc(3), we will goto error. At this time we've got a space with uninitialized data because of malloc(3), but no space for guestfs_int_btrfsqgroup_list_val. When processing in label error, we could not know: ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path is a valid address. 1) One solution is use calloc to replace the first malloc. Then: if (ret-> guestfs_int_btrfssubvolume_list_val) for (...) It costs more codes. 2) use the current solution I think the process in this patch should be a choice. How do you think?