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

[Kirill A. Shutemov]

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().

>
> 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
>
>
>
>