[Qemu-devel] [PATCHv3 0/2] envlist.c: handle strdup failure

[Qemu-devel] [PATCHv3 0/2] envlist.c: handle strdup failure

[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

[Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces

[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

[Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure

[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

Re: [Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces

[Andreas Färber]

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

Re: [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure

[Andreas Färber]

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

Re: [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure

[Jim Meyering]

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.