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

[Jean-Christophe Dubois]

On Thursday 03 September 2009 09:19:19 Kirill A. Shutemov wrote:
> On Thu, Sep 3, 2009 at 3:32 AM, 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.
>
> Nak.
>
> Do you really think you can do something useful after memory allocation
> fail? I think you can only do is call abort().

Well, in case of realloc() NULL is returned. It is up to the calling function 
to deal with the return code.

What is really annoying is the fact that the memory is lost and in particular 
all the pointers contained into this memory block are lost. This was already 
the case with the previous implementation, I just made it clearer.

Aborting is one option but it is not necessarily the only one. If this is the 
consensus here so let it be (actually it sounds like the qemu_realloc would 
have already aborted by itself as pointed by Juan...).

JC

>
> > 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) {
> > +        if (!(new->name = qemu_strdup(name)))
> > +            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)) {
> >         struct dirent *dirent;
> >
> >         while ((dirent = readdir(dir)) != NULL) {
> > @@ -67,6 +102,7 @@ static struct pathelem *add_dir_maybe(struct pathelem
> > *path) }
> >         closedir(dir);
> >     }
> > +
> >     return path;
> >  }
> >
> > @@ -74,13 +110,25 @@ static struct pathelem *add_entry(struct pathelem
> > *root, const char *name) {
> >     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) {
> > +        root->entries[root->num_entries-1]
> > +            = add_dir_maybe(new_entry(root->pathname, root, name));
> > +
> > +        if (!root->entries[root->num_entries-1])
> > +            goto fail;
> > +    } else {
> > +        /* if realloc failed, we have leaked some memory and we can't
> > recover it */ +    }
> > +
> >     return root;
> > +
> > +fail:
> > +    free_entry(root);
> > +    qemu_free(root);
> > +    return NULL;
> >  }
> >
> >  /* This needs to be done after tree is stabilized (ie. no more
> > reallocs!). */ @@ -140,10 +188,14 @@ void init_paths(const char *prefix)
> >     } else
> >         pstrcpy(pref_buf, sizeof(pref_buf), prefix + 1);
> >
> > -    base = new_entry("", NULL, pref_buf);
> > -    base = add_dir_maybe(base);
> > +    base = add_dir_maybe(new_entry("", NULL, pref_buf));
> > +
> > +    if (!base)
> > +        abort();
> > +
> >     if (base->num_entries == 0) {
> > -        free (base);
> > +        free_entry(base);
> > +        qemu_free (base);
> >         base = NULL;
> >     } else {
> >         set_parents(base, base);
> > --
> > 1.6.0.4