[Qemu-devel] Re: [PATCH] Fix error checking and cleaning in path.c

[Jean-Christophe Dubois]

Hello Juan,

Thanks for your comments.

OK, it is true that the qemu_malloc and friends functions are aborting in case 
of error. I did not took this into account.

Although these tests are useless or now, they are not "wrong" and if the 
qemu_malloc() behavior was to change again they would be welcome.

However I will resubmit the patch without the qemu_malloc return value 
checking.

JC

On Thursday 03 September 2009 09:37:38 Juan Quintela wrote:
> Jean-Christophe DUBOIS <address@hidden> wrote:
> > Very little error checking was done in path.c and we were
> > leaking some memories in case of error.
> >
> > Signed-off-by: Jean-Christophe Dubois <address@hidden>
> > ---
> >  path.c |   76
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 files
> > changed, 64 insertions(+), 12 deletions(-)
> >
> > diff --git a/path.c b/path.c
> > index cc9e007..24e8fdd 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -40,15 +40,50 @@ static int strneq(const char *s1, unsigned int n,
> > const char *s2)
> >
> >  static struct pathelem *add_entry(struct pathelem *root, const char
> > *name);
> >
> > +static void free_entry(struct pathelem *ptr)
> > +{
> > +    if(ptr)
> > +    {
> > +        while(ptr->num_entries) {
> > +            if (ptr->entries[ptr->num_entries -1]) {
> > +                free_entry(ptr->entries[ptr->num_entries -1]);
> > +                qemu_free(ptr->entries[ptr->num_entries -1]);
> > +                ptr->entries[ptr->num_entries -1] = NULL;
> > +            }
> > +            ptr->num_entries--;
> > +        }
> > +
> > +        if (ptr->name) {
> > +            qemu_free(ptr->name);
> > +            ptr->name = NULL;
> > +        }
> > +
> > +        if (ptr->pathname) {
> > +            free(ptr->pathname);
> > +            ptr->pathname = NULL;
> > +        }
> > +    }
> > +}
> > +
> >  static struct pathelem *new_entry(const char *root,
> >                                    struct pathelem *parent,
> >                                    const char *name)
> >  {
> > -    struct pathelem *new = malloc(sizeof(*new));
> > -    new->name = strdup(name);
> > -    asprintf(&new->pathname, "%s/%s", root, name);
> > -    new->num_entries = 0;
> > +    struct pathelem *new = qemu_mallocz(sizeof(*new));
> > +
> > +    if (new) {
>
> qemu_malloc*() never returns NULL in qemu.  this test is not needed.
>
> > +        if (!(new->name = qemu_strdup(name)))
>
> same for qemu_strdup()
>
> > +            goto fail;
> > +        if (asprintf(&new->pathname, "%s/%s", root, name) < 0)
> > +            goto fail;
> > +        new->num_entries = 0;
> > +    }
> >      return new;
> > +
> > +fail:
> > +    free_entry(new);
> > +    qemu_free(new);
> > +    return NULL;
> >  }
> >
> >  #define streq(a,b) (strcmp((a), (b)) == 0)
> > @@ -57,7 +92,7 @@ static struct pathelem *add_dir_maybe(struct pathelem
> > *path) {
> >      DIR *dir;
> >
> > -    if ((dir = opendir(path->pathname)) != NULL) {
> > +    if (path && ((dir = opendir(path->pathname)) != NULL)) {
>
> I think that here path can never be NULL.  But didn't check all the
> possible "paths" that call it.
>
> >  {
> >      root->num_entries++;
> >
> > -    root = realloc(root, sizeof(*root)
> > +    root = qemu_realloc(root, sizeof(*root)
> >                     + sizeof(root->entries[0])*root->num_entries);
> >
> > -    root->entries[root->num_entries-1] = new_entry(root->pathname, root,
> > name); -    root->entries[root->num_entries-1]
> > -        = add_dir_maybe(root->entries[root->num_entries-1]);
> > +    if (root) {
>
> Now that you call qemu_realloc() root is never going to be NULL, you
> don't need the check either.
>
> Later, Juan.