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

[Pino Toscano]

Hi,

On Thursday 18 June 2015 05:14:46 Chen, Hanxiao wrote:
> > -----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?

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