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

[Chen, Hanxiao]

Hi, Pino

> -----Original Message-----
> From: libguestfs-bounces at redhat.com [mailto:libguestfs-bounces at redhat.com] On
> Behalf Of Pino Toscano
> Sent: Wednesday, June 17, 2015 10:44 PM
> To: libguestfs at 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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs