* [Qemu-devel] [PATCHv3 0/2] envlist.c: handle strdup failure @ 2012-05-22 10:16 Jim Meyering 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces Jim Meyering 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure Jim Meyering 0 siblings, 2 replies; 6+ messages in thread From: Jim Meyering @ 2012-05-22 10:16 UTC (permalink / raw) To: qemu-devel; +Cc: Jim Meyering From: Jim Meyering <meyering@redhat.com> Same as v2, but now with TABs converted using expand --tabs=4. Jim Meyering (2): envlist.c: convert each TAB(width-4) to equivalent spaces envlist.c: handle strdup failure envlist.c | 272 ++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 140 insertions(+), 132 deletions(-) -- 1.7.10.2.552.gaa3bb87 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces 2012-05-22 10:16 [Qemu-devel] [PATCHv3 0/2] envlist.c: handle strdup failure Jim Meyering @ 2012-05-22 10:16 ` Jim Meyering 2012-08-17 15:57 ` Andreas Färber 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure Jim Meyering 1 sibling, 1 reply; 6+ messages in thread From: Jim Meyering @ 2012-05-22 10:16 UTC (permalink / raw) To: qemu-devel; +Cc: Jim Meyering From: Jim Meyering <meyering@redhat.com> Signed-off-by: Jim Meyering <meyering@redhat.com> --- envlist.c | 256 +++++++++++++++++++++++++++++++------------------------------- 1 file changed, 128 insertions(+), 128 deletions(-) diff --git a/envlist.c b/envlist.c index f2303cd..e44889b 100644 --- a/envlist.c +++ b/envlist.c @@ -8,13 +8,13 @@ #include "envlist.h" struct envlist_entry { - const char *ev_var; /* actual env value */ - QLIST_ENTRY(envlist_entry) ev_link; + const char *ev_var; /* actual env value */ + QLIST_ENTRY(envlist_entry) ev_link; }; struct envlist { - QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */ - size_t el_count; /* number of entries */ + QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */ + size_t el_count; /* number of entries */ }; static int envlist_parse(envlist_t *envlist, @@ -27,15 +27,15 @@ static int envlist_parse(envlist_t *envlist, envlist_t * envlist_create(void) { - envlist_t *envlist; + envlist_t *envlist; - if ((envlist = malloc(sizeof (*envlist))) == NULL) - return (NULL); + if ((envlist = malloc(sizeof (*envlist))) == NULL) + return (NULL); - QLIST_INIT(&envlist->el_entries); - envlist->el_count = 0; + QLIST_INIT(&envlist->el_entries); + envlist->el_count = 0; - return (envlist); + return (envlist); } /* @@ -44,18 +44,18 @@ envlist_create(void) void envlist_free(envlist_t *envlist) { - struct envlist_entry *entry; + struct envlist_entry *entry; - assert(envlist != NULL); + assert(envlist != NULL); - while (envlist->el_entries.lh_first != NULL) { - entry = envlist->el_entries.lh_first; - QLIST_REMOVE(entry, ev_link); + while (envlist->el_entries.lh_first != NULL) { + entry = envlist->el_entries.lh_first; + QLIST_REMOVE(entry, ev_link); - free((char *)entry->ev_var); - free(entry); - } - free(envlist); + free((char *)entry->ev_var); + free(entry); + } + free(envlist); } /* @@ -72,7 +72,7 @@ envlist_free(envlist_t *envlist) int envlist_parse_set(envlist_t *envlist, const char *env) { - return (envlist_parse(envlist, env, &envlist_setenv)); + return (envlist_parse(envlist, env, &envlist_setenv)); } /* @@ -84,7 +84,7 @@ envlist_parse_set(envlist_t *envlist, const char *env) int envlist_parse_unset(envlist_t *envlist, const char *env) { - return (envlist_parse(envlist, env, &envlist_unsetenv)); + return (envlist_parse(envlist, env, &envlist_unsetenv)); } /* @@ -97,32 +97,32 @@ static int envlist_parse(envlist_t *envlist, const char *env, int (*callback)(envlist_t *, const char *)) { - char *tmpenv, *envvar; - char *envsave = NULL; - - assert(callback != NULL); - - if ((envlist == NULL) || (env == NULL)) - return (EINVAL); - - /* - * We need to make temporary copy of the env string - * as strtok_r(3) modifies it while it tokenizes. - */ - if ((tmpenv = strdup(env)) == NULL) - return (errno); - - envvar = strtok_r(tmpenv, ",", &envsave); - while (envvar != NULL) { - if ((*callback)(envlist, envvar) != 0) { - free(tmpenv); - return (errno); - } - envvar = strtok_r(NULL, ",", &envsave); - } - - free(tmpenv); - return (0); + char *tmpenv, *envvar; + char *envsave = NULL; + + assert(callback != NULL); + + if ((envlist == NULL) || (env == NULL)) + return (EINVAL); + + /* + * We need to make temporary copy of the env string + * as strtok_r(3) modifies it while it tokenizes. + */ + if ((tmpenv = strdup(env)) == NULL) + return (errno); + + envvar = strtok_r(tmpenv, ",", &envsave); + while (envvar != NULL) { + if ((*callback)(envlist, envvar) != 0) { + free(tmpenv); + return (errno); + } + envvar = strtok_r(NULL, ",", &envsave); + } + + free(tmpenv); + return (0); } /* @@ -134,46 +134,46 @@ envlist_parse(envlist_t *envlist, const char *env, int envlist_setenv(envlist_t *envlist, const char *env) { - struct envlist_entry *entry = NULL; - const char *eq_sign; - size_t envname_len; - - if ((envlist == NULL) || (env == NULL)) - return (EINVAL); - - /* find out first equals sign in given env */ - if ((eq_sign = strchr(env, '=')) == NULL) - return (EINVAL); - envname_len = eq_sign - env + 1; - - /* - * If there already exists variable with given name - * we remove and release it before allocating a whole - * new entry. - */ - for (entry = envlist->el_entries.lh_first; entry != NULL; - entry = entry->ev_link.le_next) { - if (strncmp(entry->ev_var, env, envname_len) == 0) - break; - } - - if (entry != NULL) { - QLIST_REMOVE(entry, ev_link); - free((char *)entry->ev_var); - free(entry); - } else { - envlist->el_count++; - } - - if ((entry = malloc(sizeof (*entry))) == NULL) - return (errno); - if ((entry->ev_var = strdup(env)) == NULL) { - free(entry); - return (errno); - } - QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link); - - return (0); + struct envlist_entry *entry = NULL; + const char *eq_sign; + size_t envname_len; + + if ((envlist == NULL) || (env == NULL)) + return (EINVAL); + + /* find out first equals sign in given env */ + if ((eq_sign = strchr(env, '=')) == NULL) + return (EINVAL); + envname_len = eq_sign - env + 1; + + /* + * If there already exists variable with given name + * we remove and release it before allocating a whole + * new entry. + */ + for (entry = envlist->el_entries.lh_first; entry != NULL; + entry = entry->ev_link.le_next) { + if (strncmp(entry->ev_var, env, envname_len) == 0) + break; + } + + if (entry != NULL) { + QLIST_REMOVE(entry, ev_link); + free((char *)entry->ev_var); + free(entry); + } else { + envlist->el_count++; + } + + if ((entry = malloc(sizeof (*entry))) == NULL) + return (errno); + if ((entry->ev_var = strdup(env)) == NULL) { + free(entry); + return (errno); + } + QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link); + + return (0); } /* @@ -183,34 +183,34 @@ envlist_setenv(envlist_t *envlist, const char *env) int envlist_unsetenv(envlist_t *envlist, const char *env) { - struct envlist_entry *entry; - size_t envname_len; - - if ((envlist == NULL) || (env == NULL)) - return (EINVAL); - - /* env is not allowed to contain '=' */ - if (strchr(env, '=') != NULL) - return (EINVAL); - - /* - * Find out the requested entry and remove - * it from the list. - */ - envname_len = strlen(env); - for (entry = envlist->el_entries.lh_first; entry != NULL; - entry = entry->ev_link.le_next) { - if (strncmp(entry->ev_var, env, envname_len) == 0) - break; - } - if (entry != NULL) { - QLIST_REMOVE(entry, ev_link); - free((char *)entry->ev_var); - free(entry); - - envlist->el_count--; - } - return (0); + struct envlist_entry *entry; + size_t envname_len; + + if ((envlist == NULL) || (env == NULL)) + return (EINVAL); + + /* env is not allowed to contain '=' */ + if (strchr(env, '=') != NULL) + return (EINVAL); + + /* + * Find out the requested entry and remove + * it from the list. + */ + envname_len = strlen(env); + for (entry = envlist->el_entries.lh_first; entry != NULL; + entry = entry->ev_link.le_next) { + if (strncmp(entry->ev_var, env, envname_len) == 0) + break; + } + if (entry != NULL) { + QLIST_REMOVE(entry, ev_link); + free((char *)entry->ev_var); + free(entry); + + envlist->el_count--; + } + return (0); } /* @@ -226,21 +226,21 @@ envlist_unsetenv(envlist_t *envlist, const char *env) char ** envlist_to_environ(const envlist_t *envlist, size_t *count) { - struct envlist_entry *entry; - char **env, **penv; + struct envlist_entry *entry; + char **env, **penv; - penv = env = malloc((envlist->el_count + 1) * sizeof (char *)); - if (env == NULL) - return (NULL); + penv = env = malloc((envlist->el_count + 1) * sizeof (char *)); + if (env == NULL) + return (NULL); - for (entry = envlist->el_entries.lh_first; entry != NULL; - entry = entry->ev_link.le_next) { - *(penv++) = strdup(entry->ev_var); - } - *penv = NULL; /* NULL terminate the list */ + for (entry = envlist->el_entries.lh_first; entry != NULL; + entry = entry->ev_link.le_next) { + *(penv++) = strdup(entry->ev_var); + } + *penv = NULL; /* NULL terminate the list */ - if (count != NULL) - *count = envlist->el_count; + if (count != NULL) + *count = envlist->el_count; - return (env); + return (env); } -- 1.7.10.2.552.gaa3bb87 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces Jim Meyering @ 2012-08-17 15:57 ` Andreas Färber 0 siblings, 0 replies; 6+ messages in thread From: Andreas Färber @ 2012-08-17 15:57 UTC (permalink / raw) To: Jim Meyering; +Cc: Jim Meyering, qemu-devel Am 22.05.2012 12:16, schrieb Jim Meyering: > From: Jim Meyering <meyering@redhat.com> > > > Signed-off-by: Jim Meyering <meyering@redhat.com> > --- > envlist.c | 256 +++++++++++++++++++++++++++++++------------------------------- > 1 file changed, 128 insertions(+), 128 deletions(-) > > diff --git a/envlist.c b/envlist.c > index f2303cd..e44889b 100644 > --- a/envlist.c > +++ b/envlist.c > @@ -8,13 +8,13 @@ > #include "envlist.h" > > struct envlist_entry { > - const char *ev_var; /* actual env value */ > - QLIST_ENTRY(envlist_entry) ev_link; > + const char *ev_var; /* actual env value */ > + QLIST_ENTRY(envlist_entry) ev_link; > }; > > struct envlist { > - QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */ > - size_t el_count; /* number of entries */ > + QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */ > + size_t el_count; /* number of entries */ > }; > > static int envlist_parse(envlist_t *envlist, > @@ -27,15 +27,15 @@ static int envlist_parse(envlist_t *envlist, > envlist_t * > envlist_create(void) > { > - envlist_t *envlist; > + envlist_t *envlist; > > - if ((envlist = malloc(sizeof (*envlist))) == NULL) > - return (NULL); > + if ((envlist = malloc(sizeof (*envlist))) == NULL) > + return (NULL); Please add braces. > > - QLIST_INIT(&envlist->el_entries); > - envlist->el_count = 0; > + QLIST_INIT(&envlist->el_entries); > + envlist->el_count = 0; > > - return (envlist); > + return (envlist); > } > > /* > @@ -44,18 +44,18 @@ envlist_create(void) > void > envlist_free(envlist_t *envlist) > { > - struct envlist_entry *entry; > + struct envlist_entry *entry; > > - assert(envlist != NULL); > + assert(envlist != NULL); > > - while (envlist->el_entries.lh_first != NULL) { > - entry = envlist->el_entries.lh_first; > - QLIST_REMOVE(entry, ev_link); > + while (envlist->el_entries.lh_first != NULL) { > + entry = envlist->el_entries.lh_first; > + QLIST_REMOVE(entry, ev_link); > > - free((char *)entry->ev_var); > - free(entry); > - } > - free(envlist); > + free((char *)entry->ev_var); > + free(entry); > + } > + free(envlist); > } > > /* > @@ -72,7 +72,7 @@ envlist_free(envlist_t *envlist) > int > envlist_parse_set(envlist_t *envlist, const char *env) > { > - return (envlist_parse(envlist, env, &envlist_setenv)); > + return (envlist_parse(envlist, env, &envlist_setenv)); > } > > /* > @@ -84,7 +84,7 @@ envlist_parse_set(envlist_t *envlist, const char *env) > int > envlist_parse_unset(envlist_t *envlist, const char *env) > { > - return (envlist_parse(envlist, env, &envlist_unsetenv)); > + return (envlist_parse(envlist, env, &envlist_unsetenv)); > } > > /* > @@ -97,32 +97,32 @@ static int > envlist_parse(envlist_t *envlist, const char *env, > int (*callback)(envlist_t *, const char *)) > { > - char *tmpenv, *envvar; > - char *envsave = NULL; > - > - assert(callback != NULL); > - > - if ((envlist == NULL) || (env == NULL)) > - return (EINVAL); > - > - /* > - * We need to make temporary copy of the env string > - * as strtok_r(3) modifies it while it tokenizes. > - */ > - if ((tmpenv = strdup(env)) == NULL) > - return (errno); > - > - envvar = strtok_r(tmpenv, ",", &envsave); > - while (envvar != NULL) { > - if ((*callback)(envlist, envvar) != 0) { > - free(tmpenv); > - return (errno); > - } > - envvar = strtok_r(NULL, ",", &envsave); > - } > - > - free(tmpenv); > - return (0); > + char *tmpenv, *envvar; > + char *envsave = NULL; > + > + assert(callback != NULL); > + > + if ((envlist == NULL) || (env == NULL)) > + return (EINVAL); Braces > + > + /* > + * We need to make temporary copy of the env string > + * as strtok_r(3) modifies it while it tokenizes. > + */ > + if ((tmpenv = strdup(env)) == NULL) > + return (errno); Braces > + > + envvar = strtok_r(tmpenv, ",", &envsave); > + while (envvar != NULL) { > + if ((*callback)(envlist, envvar) != 0) { > + free(tmpenv); > + return (errno); > + } > + envvar = strtok_r(NULL, ",", &envsave); > + } > + > + free(tmpenv); > + return (0); > } > > /* > @@ -134,46 +134,46 @@ envlist_parse(envlist_t *envlist, const char *env, > int > envlist_setenv(envlist_t *envlist, const char *env) > { > - struct envlist_entry *entry = NULL; > - const char *eq_sign; > - size_t envname_len; > - > - if ((envlist == NULL) || (env == NULL)) > - return (EINVAL); > - > - /* find out first equals sign in given env */ > - if ((eq_sign = strchr(env, '=')) == NULL) > - return (EINVAL); > - envname_len = eq_sign - env + 1; > - > - /* > - * If there already exists variable with given name > - * we remove and release it before allocating a whole > - * new entry. > - */ > - for (entry = envlist->el_entries.lh_first; entry != NULL; > - entry = entry->ev_link.le_next) { > - if (strncmp(entry->ev_var, env, envname_len) == 0) > - break; > - } > - > - if (entry != NULL) { > - QLIST_REMOVE(entry, ev_link); > - free((char *)entry->ev_var); > - free(entry); > - } else { > - envlist->el_count++; > - } > - > - if ((entry = malloc(sizeof (*entry))) == NULL) > - return (errno); > - if ((entry->ev_var = strdup(env)) == NULL) { > - free(entry); > - return (errno); > - } > - QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link); > - > - return (0); > + struct envlist_entry *entry = NULL; > + const char *eq_sign; > + size_t envname_len; > + > + if ((envlist == NULL) || (env == NULL)) > + return (EINVAL); Braces > + > + /* find out first equals sign in given env */ > + if ((eq_sign = strchr(env, '=')) == NULL) > + return (EINVAL); Braces > + envname_len = eq_sign - env + 1; > + > + /* > + * If there already exists variable with given name > + * we remove and release it before allocating a whole > + * new entry. > + */ > + for (entry = envlist->el_entries.lh_first; entry != NULL; > + entry = entry->ev_link.le_next) { This adds an off-by-one that I believe you fix in 2/2. > + if (strncmp(entry->ev_var, env, envname_len) == 0) > + break; Braces > + } > + > + if (entry != NULL) { > + QLIST_REMOVE(entry, ev_link); > + free((char *)entry->ev_var); > + free(entry); > + } else { > + envlist->el_count++; > + } > + > + if ((entry = malloc(sizeof (*entry))) == NULL) > + return (errno); Braces > + if ((entry->ev_var = strdup(env)) == NULL) { > + free(entry); > + return (errno); > + } > + QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link); > + > + return (0); > } > > /* > @@ -183,34 +183,34 @@ envlist_setenv(envlist_t *envlist, const char *env) > int > envlist_unsetenv(envlist_t *envlist, const char *env) > { > - struct envlist_entry *entry; > - size_t envname_len; > - > - if ((envlist == NULL) || (env == NULL)) > - return (EINVAL); > - > - /* env is not allowed to contain '=' */ > - if (strchr(env, '=') != NULL) > - return (EINVAL); > - > - /* > - * Find out the requested entry and remove > - * it from the list. > - */ > - envname_len = strlen(env); > - for (entry = envlist->el_entries.lh_first; entry != NULL; > - entry = entry->ev_link.le_next) { > - if (strncmp(entry->ev_var, env, envname_len) == 0) > - break; > - } > - if (entry != NULL) { > - QLIST_REMOVE(entry, ev_link); > - free((char *)entry->ev_var); > - free(entry); > - > - envlist->el_count--; > - } > - return (0); > + struct envlist_entry *entry; > + size_t envname_len; > + > + if ((envlist == NULL) || (env == NULL)) > + return (EINVAL); Braces > + > + /* env is not allowed to contain '=' */ > + if (strchr(env, '=') != NULL) > + return (EINVAL); Braces > + > + /* > + * Find out the requested entry and remove > + * it from the list. > + */ > + envname_len = strlen(env); > + for (entry = envlist->el_entries.lh_first; entry != NULL; > + entry = entry->ev_link.le_next) { Or was it this off-by-one? > + if (strncmp(entry->ev_var, env, envname_len) == 0) > + break; Braces > + } > + if (entry != NULL) { > + QLIST_REMOVE(entry, ev_link); > + free((char *)entry->ev_var); > + free(entry); > + > + envlist->el_count--; > + } > + return (0); > } > > /* > @@ -226,21 +226,21 @@ envlist_unsetenv(envlist_t *envlist, const char *env) > char ** > envlist_to_environ(const envlist_t *envlist, size_t *count) > { > - struct envlist_entry *entry; > - char **env, **penv; > + struct envlist_entry *entry; > + char **env, **penv; > > - penv = env = malloc((envlist->el_count + 1) * sizeof (char *)); > - if (env == NULL) > - return (NULL); > + penv = env = malloc((envlist->el_count + 1) * sizeof (char *)); > + if (env == NULL) > + return (NULL); Braces > > - for (entry = envlist->el_entries.lh_first; entry != NULL; > - entry = entry->ev_link.le_next) { > - *(penv++) = strdup(entry->ev_var); > - } > - *penv = NULL; /* NULL terminate the list */ > + for (entry = envlist->el_entries.lh_first; entry != NULL; > + entry = entry->ev_link.le_next) { align? > + *(penv++) = strdup(entry->ev_var); > + } > + *penv = NULL; /* NULL terminate the list */ > > - if (count != NULL) > - *count = envlist->el_count; > + if (count != NULL) > + *count = envlist->el_count; Braces > > - return (env); > + return (env); > } > I've commented on all Coding Style issues I've spotted - basically, the convention has been to make the patch please checkpatch.pl and CODING_STYLE on all added lines. Didn't notice any particular deviation or mismerge so this looks okay for 1.2 if you can send a v4. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure 2012-05-22 10:16 [Qemu-devel] [PATCHv3 0/2] envlist.c: handle strdup failure Jim Meyering 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces Jim Meyering @ 2012-05-22 10:16 ` Jim Meyering 2012-08-17 16:03 ` Andreas Färber 1 sibling, 1 reply; 6+ messages in thread From: Jim Meyering @ 2012-05-22 10:16 UTC (permalink / raw) To: qemu-devel; +Cc: Jim Meyering From: Jim Meyering <meyering@redhat.com> Without this, envlist_to_environ may silently fail to copy all strings into the destination buffer, and both callers would leak any env strings allocated after a failing strdup, because the freeing code stops at the first NULL pointer. Signed-off-by: Jim Meyering <meyering@redhat.com> --- envlist.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/envlist.c b/envlist.c index e44889b..df5c723 100644 --- a/envlist.c +++ b/envlist.c @@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) return (NULL); for (entry = envlist->el_entries.lh_first; entry != NULL; - entry = entry->ev_link.le_next) { - *(penv++) = strdup(entry->ev_var); + entry = entry->ev_link.le_next, penv++) { + *penv = strdup(entry->ev_var); + if (*penv == NULL) { + char **e = env; + while (e <= penv) { + free(*e++); + } + free(env); + return NULL; + } } *penv = NULL; /* NULL terminate the list */ -- 1.7.10.2.552.gaa3bb87 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure Jim Meyering @ 2012-08-17 16:03 ` Andreas Färber 2012-08-17 18:30 ` Jim Meyering 0 siblings, 1 reply; 6+ messages in thread From: Andreas Färber @ 2012-08-17 16:03 UTC (permalink / raw) To: Jim Meyering; +Cc: Jim Meyering, qemu-devel Am 22.05.2012 12:16, schrieb Jim Meyering: > From: Jim Meyering <meyering@redhat.com> > > Without this, envlist_to_environ may silently fail to copy all > strings into the destination buffer, and both callers would leak > any env strings allocated after a failing strdup, because the > freeing code stops at the first NULL pointer. > > Signed-off-by: Jim Meyering <meyering@redhat.com> > --- > envlist.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/envlist.c b/envlist.c > index e44889b..df5c723 100644 > --- a/envlist.c > +++ b/envlist.c > @@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) > return (NULL); > > for (entry = envlist->el_entries.lh_first; entry != NULL; > - entry = entry->ev_link.le_next) { > - *(penv++) = strdup(entry->ev_var); > + entry = entry->ev_link.le_next, penv++) { Scratch my comment on 1/2, there's an added penv++ that I overlooked. Not changing the indentation twice would still be nice. > + *penv = strdup(entry->ev_var); > + if (*penv == NULL) { > + char **e = env; > + while (e <= penv) { > + free(*e++); > + } > + free(env); > + return NULL; > + } > } > *penv = NULL; /* NULL terminate the list */ > This leak fix looks good then. For anyone wondering like me, the "env" here is not the usual CPUArchState *env but a local char **env. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure 2012-08-17 16:03 ` Andreas Färber @ 2012-08-17 18:30 ` Jim Meyering 0 siblings, 0 replies; 6+ messages in thread From: Jim Meyering @ 2012-08-17 18:30 UTC (permalink / raw) To: Andreas Färber; +Cc: qemu-devel Andreas Färber wrote: > Am 22.05.2012 12:16, schrieb Jim Meyering: >> From: Jim Meyering <meyering@redhat.com> >> >> Without this, envlist_to_environ may silently fail to copy all >> strings into the destination buffer, and both callers would leak >> any env strings allocated after a failing strdup, because the >> freeing code stops at the first NULL pointer. >> >> Signed-off-by: Jim Meyering <meyering@redhat.com> >> --- >> envlist.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/envlist.c b/envlist.c >> index e44889b..df5c723 100644 >> --- a/envlist.c >> +++ b/envlist.c >> @@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) >> return (NULL); >> >> for (entry = envlist->el_entries.lh_first; entry != NULL; >> - entry = entry->ev_link.le_next) { >> - *(penv++) = strdup(entry->ev_var); >> + entry = entry->ev_link.le_next, penv++) { > > Scratch my comment on 1/2, there's an added penv++ that I overlooked. > Not changing the indentation twice would still be nice. I posted a V4 that does that, and removes parens around return arguments as well. However, I did not remove assignments from conditionals. IMHO, that would require an inordinate amount of change for the marginal gain of making legacy code conform. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-17 18:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-22 10:16 [Qemu-devel] [PATCHv3 0/2] envlist.c: handle strdup failure Jim Meyering 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces Jim Meyering 2012-08-17 15:57 ` Andreas Färber 2012-05-22 10:16 ` [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure Jim Meyering 2012-08-17 16:03 ` Andreas Färber 2012-08-17 18:30 ` Jim Meyering
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).