| Message ID | 1251937933-16778-1-git-send-email-jcd@tribudubois.net |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Sep 3, 2009 at 3:32 AM, Jean-Christophe DUBOIS<jcd@tribudubois.net> 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 <jcd@tribudubois.net> > --- > 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 > > > >
On Thursday 03 September 2009 09:19:19 Kirill A. Shutemov wrote: > On Thu, Sep 3, 2009 at 3:32 AM, Jean-Christophe > > DUBOIS<jcd@tribudubois.net> 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 <jcd@tribudubois.net> > > --- > > 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
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 <jcd@tribudubois.net> 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 <jcd@tribudubois.net> > > --- > > 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.
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);
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 <jcd@tribudubois.net> --- path.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 64 insertions(+), 12 deletions(-)