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

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

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

This is the same as v1, except that two lines of non-leading TABs
in envlist.c (indenting comments after code) have also been converted
to use equivalent spaces instead of TABs.

Jim Meyering (2):
  envlist.c: convert all TABs 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] [PATCHv2 1/2] envlist.c: convert all TABs 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..be0addb 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] [PATCHv2 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 be0addb..7532091 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] [PATCHv2 1/2] envlist.c: convert all TABs to equivalent spaces

[Peter Maydell]

On 22 May 2012 10:50, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>

If we're going to go to the effort of a complete reindent
patch we should actually reindent to the QEMU coding style
standard, which is four-space, not eight.

-- PMM

Re: [Qemu-devel] [PATCHv2 1/2] envlist.c: convert all TABs to equivalent spaces

[Jim Meyering]

Peter Maydell wrote:
> On 22 May 2012 10:50, Jim Meyering <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@redhat.com>
>
> If we're going to go to the effort of a complete reindent
> patch we should actually reindent to the QEMU coding style
> standard, which is four-space, not eight.

Good point.  V3 is on its way.

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

[Jim Meyering]

Jim Meyering wrote:
> 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 be0addb..7532091 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 */

It seems this has been lost in this list's high volume of patches.
Anyone interested?  Repost desired?

 b/envlist.c |  256 ++++++++++++++++++++++++++++++------------------------------
 envlist.c   |   12 ++
 2 files changed, 138 insertions(+), 130 deletions(-)

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

[Andreas Färber]

Am 17.08.2012 15:35, schrieb Jim Meyering:
> Jim Meyering wrote:
>> 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 be0addb..7532091 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 */
> 
> It seems this has been lost in this list's high volume of patches.
> Anyone interested?  Repost desired?

You announced a v3 but replied to v2? Indentation looks odd in 2/2, too.

Andreas

> 
>  b/envlist.c |  256 ++++++++++++++++++++++++++++++------------------------------
>  envlist.c   |   12 ++
>  2 files changed, 138 insertions(+), 130 deletions(-)
> 


-- 
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] [PATCHv2 2/2] envlist.c: handle strdup failure

[Jim Meyering]

Andreas Färber wrote:

> Am 17.08.2012 15:35, schrieb Jim Meyering:
>> Jim Meyering wrote:
>>> 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 be0addb..7532091 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 */
>>
>> It seems this has been lost in this list's high volume of patches.
>> Anyone interested?  Repost desired?
>
> You announced a v3 but replied to v2?

Here are links to v3:

  [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces
  https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02967.html

  [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure
  https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02966.html

> Indentation looks odd in 2/2, too.

That was fixed in v3.

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

[Andreas Färber]

Am 17.08.2012 17:34, schrieb Jim Meyering:
> Andreas Färber wrote:
> 
>> Am 17.08.2012 15:35, schrieb Jim Meyering:
>>> Jim Meyering wrote:
>>>> 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 be0addb..7532091 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 */
>>>
>>> It seems this has been lost in this list's high volume of patches.
>>> Anyone interested?  Repost desired?
>>
>> You announced a v3 but replied to v2?
> 
> Here are links to v3:
> 
>   [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces
>   https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02967.html
> 
>   [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure
>   https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02966.html

I commented on v3. For practical reasons please ping the latest patches
and prefer Patchwork links so that no <address@hidden> gets committed by
accident.

Thanks for your efforts in reviewing and fixing all these issues!

Andreas

> 
>> Indentation looks odd in 2/2, too.
> 
> That was fixed in v3.
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg