Pino Toscano
2015-Jun-17 14:44 UTC
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@cn.fujitsu.com> > --- > 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.> const char *errptr; > @@ -530,20 +531,27 @@ do_btrfs_subvolume_list (const mountable_t *fs) > > #undef XSTRTOU64 > > - memmove (line, line + ovector[6], ovector[7] - ovector[6] + 1); > - this->btrfssubvolume_path = line; > + this->btrfssubvolume_path > + strndup (line + ovector[6], ovector[7] - ovector[6]); > + if (this->btrfssubvolume_path == NULL) > + goto error; > } > > - free (lines); > pcre_free (re); > > return ret; > > error: > - free_stringslen (lines, nr_subvolumes); > - if (ret) free (ret->guestfs_int_btrfssubvolume_list_val); > + for (i = 0; i < nr_subvolumes; ++i) { > + struct guestfs_int_btrfssubvolume *this > + &ret->guestfs_int_btrfssubvolume_list_val[i]; > + free (this->btrfssubvolume_path);No need to save "this", just free (ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path); should be enough. Thanks, -- Pino Toscano
Chen, Hanxiao
2015-Jun-18 05:14 UTC
Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
Hi, Pino> -----Original Message----- > From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com] On > Behalf Of Pino Toscano > Sent: Wednesday, June 17, 2015 10:44 PM > To: libguestfs@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@cn.fujitsu.com> > > --- > > 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? Regards, - Chen> > const char *errptr; > > @@ -530,20 +531,27 @@ do_btrfs_subvolume_list (const mountable_t *fs) > > > > #undef XSTRTOU64 > > > > - memmove (line, line + ovector[6], ovector[7] - ovector[6] + 1); > > - this->btrfssubvolume_path = line; > > + this->btrfssubvolume_path > > + strndup (line + ovector[6], ovector[7] - ovector[6]); > > + if (this->btrfssubvolume_path == NULL) > > + goto error; > > } > > > > - free (lines); > > pcre_free (re); > > > > return ret; > > > > error: > > - free_stringslen (lines, nr_subvolumes); > > - if (ret) free (ret->guestfs_int_btrfssubvolume_list_val); > > + for (i = 0; i < nr_subvolumes; ++i) { > > + struct guestfs_int_btrfssubvolume *this > > + &ret->guestfs_int_btrfssubvolume_list_val[i]; > > + free (this->btrfssubvolume_path); > > No need to save "this", just > free (ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path); > should be enough. > > Thanks, > -- > Pino Toscano > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs
Pino Toscano
2015-Jun-18 08:32 UTC
Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
Hi, On Thursday 18 June 2015 05:14:46 Chen, Hanxiao wrote:> > -----Original Message----- > > From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com] On > > Behalf Of Pino Toscano > > Sent: Wednesday, June 17, 2015 10:44 PM > > To: libguestfs@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@cn.fujitsu.com> > > > --- > > > 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?If calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume)) fails, then ret->guestfs_int_btrfssubvolume_list_val is already a null pointer, which means you can just check for it as you do in (1) above, with no need to switch from malloc to calloc. The other alternative is to use more labels for error conditions in a symmetric way, like: ptr1 = malloc (...); if (ptr1 == NULL) goto error1; ptr1->subptr1 = malloc (...); if (ptr1->subptr1 == NULL) goto error2; ptr1->subptr2 = malloc (...); if (ptr1->subptr2 == NULL) goto error3; ... error3: free (ptr1->subptr1); error2: free (ptr1); error1: ... -- Pino Toscano
Apparently Analagous Threads
- Re: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
- Re: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
- [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
- [PATCH] btrfs: Fix btrfs_subvolume_list on F18
- [PATCH] btrfs: keep calloc and its error message match