Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
Hi, Pino
-----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>
> ---
> 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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs