Libguestfs - Jun 2015 - Re: [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

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

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