[PATCH 0/3] env functions overhaul

[PATCH 0/3] env functions overhaul

[Alexander Monakov]

Hello,

recently, fixing benign UB in unsetenv unearthed bigger issues in putenv, and
prompted an overhaul. I still have to improve env testing on libc-test side,
but I think I'm done with code tuning on the musl side, and pretty happy with
the result. Patches 2 and 3 are followups to stuff discussed on IRC; I feel
they are not essential. Patch 1 is the overhaul itself. I hope that new code
is easy to follow. It should also improve code size nicely; on x86-64 I got
(.text size change after patch 1/3 only; .bss grows by one pointer):

   text   text   filename
     17     17   obj/src/env/clearenv.o
    140     97   obj/src/env/getenv.o
    491    334   obj/src/env/putenv.o
    215    284   obj/src/env/setenv.o
    283    140   obj/src/env/unsetenv.o

Thanks.
Alexander

[PATCH 1/3] overhaul environment functions

[Alexander Monakov]

Rewrite environment access functions to slim down code, fix bugs and
avoid invoking undefined behavior.

* avoid using int-typed iterators where size_t would be correct;
* use strncmp instead of memcmp consistently;
* tighten prologues by invoking __strchrnul;
* handle NULL environ.

putenv:
* handle "=value" input via unsetenv too (will return -1/EINVAL);
* rewrite and simplify __putenv; fix the leak caused by failure to
  deallocate entry added by preceding setenv when called from putenv.

setenv:
* move management of libc-allocated entries to this translation unit,
  and use no-op weak symbols in putenv/unsetenv;
* no longer check for NULL first argument (per resolution to
  Austin Group issue #185);

unsetenv:
* rewrite; this fixes UB caused by testing a free'd pointer against
  NULL on entry to subsequent loops.

Not changed:
Failure to extend allocation tracking array (previously __env_map, now
env_alloced) is ignored rather than causing to report -1/ENOMEM to the
caller; the worst-case consequence is leaking this allocation when it
is removed or replaced in a subsequent environment access.

Initially UB in unsetenv was reported by Alexander Cherepanov.
Using a weak alias to avoid pulling in malloc via unsetenv was
suggested by Rich Felker.
---
 src/env/getenv.c   |  15 ++++----
 src/env/putenv.c   | 105 ++++++++++++++++++++++++-----------------------------
 src/env/setenv.c   |  40 +++++++++++++-------
 src/env/unsetenv.c |  58 ++++++++++++++---------------
 4 files changed, 108 insertions(+), 110 deletions(-)
 rewrite src/env/putenv.c (88%)
 rewrite src/env/unsetenv.c (77%)

diff --git a/src/env/getenv.c b/src/env/getenv.c
index 00c1bce..cf34672 100644
--- a/src/env/getenv.c
+++ b/src/env/getenv.c
@@ -2,13 +2,14 @@
 #include <string.h>
 #include "libc.h"
 
+char *__strchrnul(const char *, int);
+
 char *getenv(const char *name)
 {
-	int i;
-	size_t l = strlen(name);
-	if (!__environ || !*name || strchr(name, '=')) return NULL;
-	for (i=0; __environ[i] && (strncmp(name, __environ[i], l)
-		|| __environ[i][l] != '='); i++);
-	if (__environ[i]) return __environ[i] + l+1;
-	return NULL;
+	size_t l = __strchrnul(name, '=') - name;
+	if (l && !name[l] && __environ)
+		for (char **e = __environ; *e; e++)
+			if (!strncmp(name, *e, l) && l[*e] == '=')
+				return *e + l+1;
+	return 0;
 }
diff --git a/src/env/putenv.c b/src/env/putenv.c
dissimilarity index 88%
index 7153042..39a71be 100644
--- a/src/env/putenv.c
+++ b/src/env/putenv.c
@@ -1,58 +1,47 @@
-#include <stdlib.h>
-#include <string.h>
-
-extern char **__environ;
-char **__env_map;
-
-int __putenv(char *s, int a)
-{
-	int i=0, j=0;
-	char *z = strchr(s, '=');
-	char **newenv = 0;
-	char **newmap = 0;
-	static char **oldenv;
-
-	if (!z) return unsetenv(s);
-	if (z==s) return -1;
-	for (; __environ[i] && memcmp(s, __environ[i], z-s+1); i++);
-	if (a) {
-		if (!__env_map) {
-			__env_map = calloc(2, sizeof(char *));
-			if (__env_map) __env_map[0] = s;
-		} else {
-			for (; __env_map[j] && __env_map[j] != __environ[i]; j++);
-			if (!__env_map[j]) {
-				newmap = realloc(__env_map, sizeof(char *)*(j+2));
-				if (newmap) {
-					__env_map = newmap;
-					__env_map[j] = s;
-					__env_map[j+1] = NULL;
-				}
-			} else {
-				free(__env_map[j]);
-				__env_map[j] = s;
-			}
-		}
-	}
-	if (!__environ[i]) {
-		newenv = malloc(sizeof(char *)*(i+2));
-		if (!newenv) {
-			if (a && __env_map) __env_map[j] = 0;
-			return -1;
-		}
-		memcpy(newenv, __environ, sizeof(char *)*i);
-		newenv[i] = s;
-		newenv[i+1] = 0;
-		__environ = newenv;
-		free(oldenv);
-		oldenv = __environ;
-	}
-
-	__environ[i] = s;
-	return 0;
-}
-
-int putenv(char *s)
-{
-	return __putenv(s, 0);
-}
+#include <stdlib.h>
+#include <string.h>
+#include "libc.h"
+
+char *__strchrnul(const char *, int);
+
+static void dummy(char *p, char *r) {}
+weak_alias(dummy, __env_change);
+
+int __putenv(char *s, size_t l, char *r)
+{
+	size_t i=0;
+	if (__environ)
+		for (; __environ[i]; i++)
+			if (!strncmp(__environ[i], s, l+1)) {
+				char *tmp = __environ[i];
+				__environ[i] = s;
+				__env_change(tmp, r);
+				return 0;
+			}
+	static char **oldenv;
+	char **newenv;
+	if (__environ == oldenv) {
+		newenv = realloc(oldenv, sizeof *newenv * (i+2));
+		if (!newenv) goto oom;
+	} else {
+		newenv = malloc(sizeof *newenv * (i+2));
+		if (!newenv) goto oom;
+		if (i) memcpy(newenv, __environ, sizeof *newenv * i);
+		free(oldenv);
+	}
+	newenv[i] = s;
+	newenv[i+1] = 0;
+	__environ = oldenv = newenv;
+	if (r) __env_change(0, r);
+	return 0;
+oom:
+	free(r);
+	return -1;
+}
+
+int putenv(char *s)
+{
+	size_t l = __strchrnul(s, '=') - s;
+	if (!l || !s[l]) return unsetenv(s);
+	return __putenv(s, l, 0);
+}
diff --git a/src/env/setenv.c b/src/env/setenv.c
index 76e8ee1..6984237 100644
--- a/src/env/setenv.c
+++ b/src/env/setenv.c
@@ -2,29 +2,41 @@
 #include <string.h>
 #include <errno.h>
 
-int __putenv(char *s, int a);
+char *__strchrnul(const char *, int);
+int __putenv(char *, size_t, char *);
+
+void __env_change(char *p, char *r)
+{
+	static char **env_alloced;
+	static size_t env_alloced_n;
+	for (size_t i=0; i < env_alloced_n; i++)
+		if (env_alloced[i] == p) {
+			env_alloced[i] = r;
+			free(p);
+			return;
+		}
+	if (!r) return;
+	char **new_ea = realloc(env_alloced, sizeof *new_ea * (env_alloced_n+1));
+	if (!new_ea) return;
+	new_ea[env_alloced_n++] = r;
+	env_alloced = new_ea;
+}
 
 int setenv(const char *var, const char *value, int overwrite)
 {
 	char *s;
-	int l1, l2;
-
-	if (!var || !*var || strchr(var, '=')) {
+	size_t l1 = __strchrnul(var, '=') - var, l2;
+	if (!l1 || var[l1]) {
 		errno = EINVAL;
 		return -1;
 	}
 	if (!overwrite && getenv(var)) return 0;
 
-	l1 = strlen(var);
 	l2 = strlen(value);
 	s = malloc(l1+l2+2);
-	if (s) {
-		memcpy(s, var, l1);
-		s[l1] = '=';
-		memcpy(s+l1+1, value, l2);
-		s[l1+l2+1] = 0;
-		if (!__putenv(s, 1)) return 0;
-	}
-	free(s);
-	return -1;
+	if (!s) return -1;
+	memcpy(s, var, l1);
+	s[l1] = '=';
+	memcpy(s+l1+1, value, l2+1);
+	return __putenv(s, l1, s);
 }
diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c
dissimilarity index 77%
index 3569335..86873cd 100644
--- a/src/env/unsetenv.c
+++ b/src/env/unsetenv.c
@@ -1,31 +1,27 @@
-#include <stdlib.h>
-#include <string.h>
-#include <errno.h>
-
-extern char **__environ;
-extern char **__env_map;
-
-int unsetenv(const char *name)
-{
-	int i, j;
-	size_t l = strlen(name);
-
-	if (!*name || strchr(name, '=')) {
-		errno = EINVAL;
-		return -1;
-	}
-again:
-	for (i=0; __environ[i] && (memcmp(name, __environ[i], l) || __environ[i][l] != '='); i++);
-	if (__environ[i]) {
-		if (__env_map) {
-			for (j=0; __env_map[j] && __env_map[j] != __environ[i]; j++);
-			free (__env_map[j]);
-			for (; __env_map[j]; j++)
-				__env_map[j] = __env_map[j+1];
-		}
-		for (; __environ[i]; i++)
-			__environ[i] = __environ[i+1];
-		goto again;
-	}
-	return 0;
-}
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include "libc.h"
+
+char *__strchrnul(const char *, int);
+
+static void dummy(char *p, char *r) {}
+weak_alias(dummy, __env_change);
+
+int unsetenv(const char *name)
+{
+	size_t l = __strchrnul(name, '=') - name;
+	if (!l || name[l]) {
+		errno = EINVAL;
+		return -1;
+	}
+	if (!__environ) return 0;
+	for (char **e = __environ; *e; e++)
+		while (*e && !strncmp(name, *e, l) && l[*e] == '=') {
+			char **ee = e, *tmp = *e;
+			do *ee = *(ee+1);
+			while (*++ee);
+			__env_change(tmp, 0);
+		}
+	return 0;
+}
-- 
2.1.3

Re: [PATCH 1/3] overhaul environment functions

[Rich Felker]

Finally some review; sorry for the delay:

On Sun, Mar 13, 2016 at 09:53:48PM +0300, Alexander Monakov wrote:
> Rewrite environment access functions to slim down code, fix bugs and
> avoid invoking undefined behavior.
> 
> * avoid using int-typed iterators where size_t would be correct;

Very good.

> * use strncmp instead of memcmp consistently;

Agreed; removing assumptions about memcmp's implementation is a good
idea.

> * tighten prologues by invoking __strchrnul;

Generally I prefer to avoid impl-internal functions when a public one
with do without significant additional cost, but these functions
already have a fair amount of impl-internal stuff themselves so I'm
rather indifferent here.

BTW strcspn would be nice and idiomaic when the result you want is a
count anyway, rather than a pointer, but it would add a little bit to
the code size these functions depend on.

> * handle NULL environ.

OK.

> putenv:
> * handle "=value" input via unsetenv too (will return -1/EINVAL);

What happens now? It adds an env var with zero-length name?

> * rewrite and simplify __putenv; fix the leak caused by failure to
>   deallocate entry added by preceding setenv when called from putenv.

Sounds good.

> setenv:
> * move management of libc-allocated entries to this translation unit,
>   and use no-op weak symbols in putenv/unsetenv;

:)

> * no longer check for NULL first argument (per resolution to
>   Austin Group issue #185);

I suspect we'll get complaints about this change, but I'm not
necessarily against it. However since the old standard specified this
case we should probably not break software written to the old
standard.

> unsetenv:
> * rewrite; this fixes UB caused by testing a free'd pointer against
>   NULL on entry to subsequent loops.

Good.

> Not changed:
> Failure to extend allocation tracking array (previously __env_map, now
> env_alloced) is ignored rather than causing to report -1/ENOMEM to the
> caller; the worst-case consequence is leaking this allocation when it
> is removed or replaced in a subsequent environment access.

I'd like to fix this too but it can wait. I think it should be trivial
to do as a small patch on top.

> Initially UB in unsetenv was reported by Alexander Cherepanov.
> Using a weak alias to avoid pulling in malloc via unsetenv was
> suggested by Rich Felker.
> ---
>  src/env/getenv.c   |  15 ++++----
>  src/env/putenv.c   | 105 ++++++++++++++++++++++++-----------------------------
>  src/env/setenv.c   |  40 +++++++++++++-------
>  src/env/unsetenv.c |  58 ++++++++++++++---------------
>  4 files changed, 108 insertions(+), 110 deletions(-)
>  rewrite src/env/putenv.c (88%)
>  rewrite src/env/unsetenv.c (77%)
> 
> diff --git a/src/env/getenv.c b/src/env/getenv.c
> index 00c1bce..cf34672 100644
> --- a/src/env/getenv.c
> +++ b/src/env/getenv.c
> @@ -2,13 +2,14 @@
>  #include <string.h>
>  #include "libc.h"
>  
> +char *__strchrnul(const char *, int);
> +
>  char *getenv(const char *name)
>  {
> -	int i;
> -	size_t l = strlen(name);
> -	if (!__environ || !*name || strchr(name, '=')) return NULL;
> -	for (i=0; __environ[i] && (strncmp(name, __environ[i], l)
> -		|| __environ[i][l] != '='); i++);
> -	if (__environ[i]) return __environ[i] + l+1;
> -	return NULL;
> +	size_t l = __strchrnul(name, '=') - name;
> +	if (l && !name[l] && __environ)
> +		for (char **e = __environ; *e; e++)
> +			if (!strncmp(name, *e, l) && l[*e] == '=')

l[*e], nice. :-P

> +				return *e + l+1;
> +	return 0;
>  }
> diff --git a/src/env/putenv.c b/src/env/putenv.c
> dissimilarity index 88%
> index 7153042..39a71be 100644
> --- a/src/env/putenv.c
> +++ b/src/env/putenv.c
> @@ -1,58 +1,47 @@
> -#include <stdlib.h>
> -#include <string.h>
> -
> -extern char **__environ;
> -char **__env_map;
> -
> -int __putenv(char *s, int a)
> -{
> -	int i=0, j=0;
> -	char *z = strchr(s, '=');
> -	char **newenv = 0;
> -	char **newmap = 0;
> -	static char **oldenv;
> -
> -	if (!z) return unsetenv(s);
> -	if (z==s) return -1;
> -	for (; __environ[i] && memcmp(s, __environ[i], z-s+1); i++);
> -	if (a) {
> -		if (!__env_map) {
> -			__env_map = calloc(2, sizeof(char *));
> -			if (__env_map) __env_map[0] = s;
> -		} else {
> -			for (; __env_map[j] && __env_map[j] != __environ[i]; j++);
> -			if (!__env_map[j]) {
> -				newmap = realloc(__env_map, sizeof(char *)*(j+2));
> -				if (newmap) {
> -					__env_map = newmap;
> -					__env_map[j] = s;
> -					__env_map[j+1] = NULL;
> -				}
> -			} else {
> -				free(__env_map[j]);
> -				__env_map[j] = s;
> -			}
> -		}
> -	}
> -	if (!__environ[i]) {
> -		newenv = malloc(sizeof(char *)*(i+2));
> -		if (!newenv) {
> -			if (a && __env_map) __env_map[j] = 0;
> -			return -1;
> -		}
> -		memcpy(newenv, __environ, sizeof(char *)*i);
> -		newenv[i] = s;
> -		newenv[i+1] = 0;
> -		__environ = newenv;
> -		free(oldenv);
> -		oldenv = __environ;
> -	}
> -
> -	__environ[i] = s;
> -	return 0;
> -}
> -
> -int putenv(char *s)
> -{
> -	return __putenv(s, 0);
> -}
> +#include <stdlib.h>
> +#include <string.h>
> +#include "libc.h"
> +
> +char *__strchrnul(const char *, int);
> +
> +static void dummy(char *p, char *r) {}
> +weak_alias(dummy, __env_change);
> +
> +int __putenv(char *s, size_t l, char *r)
> +{
> +	size_t i=0;
> +	if (__environ)
> +		for (; __environ[i]; i++)
> +			if (!strncmp(__environ[i], s, l+1)) {
> +				char *tmp = __environ[i];
> +				__environ[i] = s;
> +				__env_change(tmp, r);
> +				return 0;
> +			}

As far as I can tell, this leaves multiple definitions in place. Am I
missing something? Maybe it's only unset and not replacement that's
safe against multiple-definition madness?

> +	static char **oldenv;
> +	char **newenv;
> +	if (__environ == oldenv) {
> +		newenv = realloc(oldenv, sizeof *newenv * (i+2));
> +		if (!newenv) goto oom;
> +	} else {
> +		newenv = malloc(sizeof *newenv * (i+2));
> +		if (!newenv) goto oom;
> +		if (i) memcpy(newenv, __environ, sizeof *newenv * i);
> +		free(oldenv);
> +	}

Rather than using malloc when __environ != oldenv, I think we should
use realloc on oldenv, so that we don't leak internally-allocated
environ arrays if the program repeatedly does environ=0 or calls your
new clearenv. Perhaps we should also store the allocated size and grow
it exponentially rather than assuming it's only the filled size and
calling realloc every time, but maybe it's actually better to resize
up/down. Do you have an opinion?

> diff --git a/src/env/setenv.c b/src/env/setenv.c
> index 76e8ee1..6984237 100644
> --- a/src/env/setenv.c
> +++ b/src/env/setenv.c
> @@ -2,29 +2,41 @@
>  #include <string.h>
>  #include <errno.h>
>  
> -int __putenv(char *s, int a);
> +char *__strchrnul(const char *, int);
> +int __putenv(char *, size_t, char *);
> +
> +void __env_change(char *p, char *r)
> +{
> +	static char **env_alloced;
> +	static size_t env_alloced_n;
> +	for (size_t i=0; i < env_alloced_n; i++)
> +		if (env_alloced[i] == p) {
> +			env_alloced[i] = r;
> +			free(p);
> +			return;
> +		}
> +	if (!r) return;
> +	char **new_ea = realloc(env_alloced, sizeof *new_ea * (env_alloced_n+1));
> +	if (!new_ea) return;
> +	new_ea[env_alloced_n++] = r;
> +	env_alloced = new_ea;
> +}

This could also be a candidate for exponential realloc strategy.

>  int setenv(const char *var, const char *value, int overwrite)
>  {
>  	char *s;
> -	int l1, l2;
> -
> -	if (!var || !*var || strchr(var, '=')) {
> +	size_t l1 = __strchrnul(var, '=') - var, l2;
> +	if (!l1 || var[l1]) {
>  		errno = EINVAL;
>  		return -1;
>  	}

As mentioned above we should probably keep the POSIX-old behavior of
accepting a null pointer and treating it as a diagnosed error.

>  	if (!overwrite && getenv(var)) return 0;
>  
> -	l1 = strlen(var);
>  	l2 = strlen(value);
>  	s = malloc(l1+l2+2);
> -	if (s) {
> -		memcpy(s, var, l1);
> -		s[l1] = '=';
> -		memcpy(s+l1+1, value, l2);
> -		s[l1+l2+1] = 0;
> -		if (!__putenv(s, 1)) return 0;
> -	}
> -	free(s);
> -	return -1;
> +	if (!s) return -1;
> +	memcpy(s, var, l1);
> +	s[l1] = '=';
> +	memcpy(s+l1+1, value, l2+1);
> +	return __putenv(s, l1, s);
>  }

This leaks when __putenv fails. It's no worse than before but should
probably be fixed.

> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include "libc.h"
> +
> +char *__strchrnul(const char *, int);
> +
> +static void dummy(char *p, char *r) {}
> +weak_alias(dummy, __env_change);
> +
> +int unsetenv(const char *name)
> +{
> +	size_t l = __strchrnul(name, '=') - name;
> +	if (!l || name[l]) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +	if (!__environ) return 0;
> +	for (char **e = __environ; *e; e++)
> +		while (*e && !strncmp(name, *e, l) && l[*e] == '=') {
> +			char **ee = e, *tmp = *e;
> +			do *ee = *(ee+1);
> +			while (*++ee);
> +			__env_change(tmp, 0);
> +		}
> +	return 0;
> +}

I think this looks ok.

Rich

Re: [PATCH 1/3] overhaul environment functions

[Alexander Monakov]

On Sat, 19 Mar 2016, Rich Felker wrote:
> > putenv:
> > * handle "=value" input via unsetenv too (will return -1/EINVAL);
> 
> What happens now? It adds an env var with zero-length name?

Returns -1 without modifying errno.

> > Not changed:
> > Failure to extend allocation tracking array (previously __env_map, now
> > env_alloced) is ignored rather than causing to report -1/ENOMEM to the
> > caller; the worst-case consequence is leaking this allocation when it
> > is removed or replaced in a subsequent environment access.
> 
> I'd like to fix this too but it can wait. I think it should be trivial
> to do as a small patch on top.

I thought about that, but decided to avoid doing that in the first cut
(__env_change return type changes to 'int', dummy definitions need to be
adjusted accordingly, and oom handling in __env_change is not beautiful
either)

> > --- a/src/env/putenv.c
> > +++ b/src/env/putenv.c
> > @@ -1,58 +1,47 @@
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include "libc.h"
> > +
> > +char *__strchrnul(const char *, int);
> > +
> > +static void dummy(char *p, char *r) {}
> > +weak_alias(dummy, __env_change);
> > +
> > +int __putenv(char *s, size_t l, char *r)
> > +{
> > +	size_t i=0;
> > +	if (__environ)
> > +		for (; __environ[i]; i++)
> > +			if (!strncmp(__environ[i], s, l+1)) {
> > +				char *tmp = __environ[i];
> > +				__environ[i] = s;
> > +				__env_change(tmp, r);
> > +				return 0;
> > +			}
> 
> As far as I can tell, this leaves multiple definitions in place. Am I
> missing something? Maybe it's only unset and not replacement that's
> safe against multiple-definition madness?

Removing multiple definitions is what patch 2/3 does. This patch just
keeps the status quo (only unsetenv looks at duplicate definitions).

> 
> > +	static char **oldenv;
> > +	char **newenv;
> > +	if (__environ == oldenv) {
> > +		newenv = realloc(oldenv, sizeof *newenv * (i+2));
> > +		if (!newenv) goto oom;
> > +	} else {
> > +		newenv = malloc(sizeof *newenv * (i+2));
> > +		if (!newenv) goto oom;
> > +		if (i) memcpy(newenv, __environ, sizeof *newenv * i);
> > +		free(oldenv);
> > +	}
> 
> Rather than using malloc when __environ != oldenv, I think we should
> use realloc on oldenv, so that we don't leak internally-allocated
> environ arrays if the program repeatedly does environ=0 or calls your
> new clearenv.

How can we leak internally allocated environ here? If there's one, oldenv
points to it, and we free it right after memcpy.

I think realloc can be used if the program does not modify environ, but if
it does something funky like 'environ++[2] = 0;' then memcpy'ing after realloc
is not safe (unlike doing malloc-memcpy-free as above).

> Perhaps we should also store the allocated size and grow
> it exponentially rather than assuming it's only the filled size and
> calling realloc every time, but maybe it's actually better to resize
> up/down. Do you have an opinion?

Some fraction of times realloc will keep it in place due to binning, right?
I think taking 'simplicity' in efficiency-simplicity tradeoff here is
justified, on the basis that majority of software does not repeatedly change
environment, and for shells this libc facility is of little help anyway.

> >  int setenv(const char *var, const char *value, int overwrite)
> >  {
> >  	char *s;
> > -	int l1, l2;
> > -
> > -	if (!var || !*var || strchr(var, '=')) {
> > +	size_t l1 = __strchrnul(var, '=') - var, l2;
> > +	if (!l1 || var[l1]) {
> >  		errno = EINVAL;
> >  		return -1;
> >  	}
> 
> As mentioned above we should probably keep the POSIX-old behavior of
> accepting a null pointer and treating it as a diagnosed error.

OK; in an old revision I had 'if (!var || !(l1 = __strchrnul(var, '=')) ...'

> >  	if (!overwrite && getenv(var)) return 0;
> >  
> > -	l1 = strlen(var);
> >  	l2 = strlen(value);
> >  	s = malloc(l1+l2+2);
> > -	if (s) {
> > -		memcpy(s, var, l1);
> > -		s[l1] = '=';
> > -		memcpy(s+l1+1, value, l2);
> > -		s[l1+l2+1] = 0;
> > -		if (!__putenv(s, 1)) return 0;
> > -	}
> > -	free(s);
> > -	return -1;
> > +	if (!s) return -1;
> > +	memcpy(s, var, l1);
> > +	s[l1] = '=';
> > +	memcpy(s+l1+1, value, l2+1);
> > +	return __putenv(s, l1, s);
> >  }
> 
> This leaks when __putenv fails. It's no worse than before but should
> probably be fixed.

Only when __env_change, specifically, "fails"; not when __putenv OOMs.

> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include "libc.h"
> > +
> > +char *__strchrnul(const char *, int);
> > +
> > +static void dummy(char *p, char *r) {}
> > +weak_alias(dummy, __env_change);
> > +
> > +int unsetenv(const char *name)
> > +{
> > +	size_t l = __strchrnul(name, '=') - name;
> > +	if (!l || name[l]) {
> > +		errno = EINVAL;
> > +		return -1;
> > +	}
> > +	if (!__environ) return 0;
> > +	for (char **e = __environ; *e; e++)
> > +		while (*e && !strncmp(name, *e, l) && l[*e] == '=') {
> > +			char **ee = e, *tmp = *e;
> > +			do *ee = *(ee+1);
> > +			while (*++ee);
> > +			__env_change(tmp, 0);
> > +		}
> > +	return 0;
> > +}
> 
> I think this looks ok.

I'd like to add a minor tweak here: instead of retesting '*e' in 'while' loop
header, do 'if (!*e) return 0;' after __env_change; this helps the compiler to
generate slightly cleaner code.

Thanks for the review!
Alexander

Re: [PATCH 1/3] overhaul environment functions

[Rich Felker]

On Sat, Mar 19, 2016 at 09:11:16PM +0300, Alexander Monakov wrote:
> On Sat, 19 Mar 2016, Rich Felker wrote:
> > > putenv:
> > > * handle "=value" input via unsetenv too (will return -1/EINVAL);
> > 
> > What happens now? It adds an env var with zero-length name?
> 
> Returns -1 without modifying errno.

OK, so not that bad, but a good fix.

> > > Not changed:
> > > Failure to extend allocation tracking array (previously __env_map, now
> > > env_alloced) is ignored rather than causing to report -1/ENOMEM to the
> > > caller; the worst-case consequence is leaking this allocation when it
> > > is removed or replaced in a subsequent environment access.
> > 
> > I'd like to fix this too but it can wait. I think it should be trivial
> > to do as a small patch on top.
> 
> I thought about that, but decided to avoid doing that in the first cut
> (__env_change return type changes to 'int', dummy definitions need to be
> adjusted accordingly, and oom handling in __env_change is not beautiful
> either)

OK.

> > > --- a/src/env/putenv.c
> > > +++ b/src/env/putenv.c
> > > @@ -1,58 +1,47 @@
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include "libc.h"
> > > +
> > > +char *__strchrnul(const char *, int);
> > > +
> > > +static void dummy(char *p, char *r) {}
> > > +weak_alias(dummy, __env_change);
> > > +
> > > +int __putenv(char *s, size_t l, char *r)
> > > +{
> > > +	size_t i=0;
> > > +	if (__environ)
> > > +		for (; __environ[i]; i++)
> > > +			if (!strncmp(__environ[i], s, l+1)) {
> > > +				char *tmp = __environ[i];
> > > +				__environ[i] = s;
> > > +				__env_change(tmp, r);
> > > +				return 0;
> > > +			}
> > 
> > As far as I can tell, this leaves multiple definitions in place. Am I
> > missing something? Maybe it's only unset and not replacement that's
> > safe against multiple-definition madness?
> 
> Removing multiple definitions is what patch 2/3 does. This patch just
> keeps the status quo (only unsetenv looks at duplicate definitions).

Ah yes, somehow I missed that.

> > > +	static char **oldenv;
> > > +	char **newenv;
> > > +	if (__environ == oldenv) {
> > > +		newenv = realloc(oldenv, sizeof *newenv * (i+2));
> > > +		if (!newenv) goto oom;
> > > +	} else {
> > > +		newenv = malloc(sizeof *newenv * (i+2));
> > > +		if (!newenv) goto oom;
> > > +		if (i) memcpy(newenv, __environ, sizeof *newenv * i);
> > > +		free(oldenv);
> > > +	}
> > 
> > Rather than using malloc when __environ != oldenv, I think we should
> > use realloc on oldenv, so that we don't leak internally-allocated
> > environ arrays if the program repeatedly does environ=0 or calls your
> > new clearenv.
> 
> How can we leak internally allocated environ here? If there's one, oldenv
> points to it, and we free it right after memcpy.

for (;;) { clearenv(); putenv("FOO=BAR"); }

Since __environ!=oldenv at the time of every putenv call, each call
allocates a new environment array and the old one is leaked.

> I think realloc can be used if the program does not modify environ, but if
> it does something funky like 'environ++[2] = 0;' then memcpy'ing after realloc
> is not safe (unlike doing malloc-memcpy-free as above).

I see. I don't think that should be permitted, but maybe it is, and it
could be caught as a special case if we want: we can check if
__environ points into the internally allocated array (either by
invalid comparison of pointers into different arrays, or by an O(n)
search through the internal array to see if any of its slots have
address equal to __environ) and just leak in that case.

Alternatively we could do malloc+memcpy+free in all cases instead of
realloc.

I probably have not yet thought enough about this to have a good idea
on what's the best thing to do, so perhaps just leave the leak for
now.

> > Perhaps we should also store the allocated size and grow
> > it exponentially rather than assuming it's only the filled size and
> > calling realloc every time, but maybe it's actually better to resize
> > up/down. Do you have an opinion?
> 
> Some fraction of times realloc will keep it in place due to binning, right?
> I think taking 'simplicity' in efficiency-simplicity tradeoff here is
> justified, on the basis that majority of software does not repeatedly change
> environment, and for shells this libc facility is of little help anyway.

Increasingly doing realloc by 1 slot each time is classic
"accidentally quadratic". I agree that it probably doesn't matter in
practice though.

> > >  int setenv(const char *var, const char *value, int overwrite)
> > >  {
> > >  	char *s;
> > > -	int l1, l2;
> > > -
> > > -	if (!var || !*var || strchr(var, '=')) {
> > > +	size_t l1 = __strchrnul(var, '=') - var, l2;
> > > +	if (!l1 || var[l1]) {
> > >  		errno = EINVAL;
> > >  		return -1;
> > >  	}
> > 
> > As mentioned above we should probably keep the POSIX-old behavior of
> > accepting a null pointer and treating it as a diagnosed error.
> 
> OK; in an old revision I had 'if (!var || !(l1 = __strchrnul(var, '=')) ...'

Yes that's probably best, albeit uglier.

> > >  	if (!overwrite && getenv(var)) return 0;
> > >  
> > > -	l1 = strlen(var);
> > >  	l2 = strlen(value);
> > >  	s = malloc(l1+l2+2);
> > > -	if (s) {
> > > -		memcpy(s, var, l1);
> > > -		s[l1] = '=';
> > > -		memcpy(s+l1+1, value, l2);
> > > -		s[l1+l2+1] = 0;
> > > -		if (!__putenv(s, 1)) return 0;
> > > -	}
> > > -	free(s);
> > > -	return -1;
> > > +	if (!s) return -1;
> > > +	memcpy(s, var, l1);
> > > +	s[l1] = '=';
> > > +	memcpy(s+l1+1, value, l2+1);
> > > +	return __putenv(s, l1, s);
> > >  }
> > 
> > This leaks when __putenv fails. It's no worse than before but should
> > probably be fixed.
> 
> Only when __env_change, specifically, "fails"; not when __putenv OOMs.

Why? s points to memory allocated by malloc, and I don't see anywhere
it's freed if __putenv fails.

> 
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <errno.h>
> > > +#include "libc.h"
> > > +
> > > +char *__strchrnul(const char *, int);
> > > +
> > > +static void dummy(char *p, char *r) {}
> > > +weak_alias(dummy, __env_change);
> > > +
> > > +int unsetenv(const char *name)
> > > +{
> > > +	size_t l = __strchrnul(name, '=') - name;
> > > +	if (!l || name[l]) {
> > > +		errno = EINVAL;
> > > +		return -1;
> > > +	}
> > > +	if (!__environ) return 0;
> > > +	for (char **e = __environ; *e; e++)
> > > +		while (*e && !strncmp(name, *e, l) && l[*e] == '=') {
> > > +			char **ee = e, *tmp = *e;
> > > +			do *ee = *(ee+1);
> > > +			while (*++ee);
> > > +			__env_change(tmp, 0);
> > > +		}
> > > +	return 0;
> > > +}
> > 
> > I think this looks ok.
> 
> I'd like to add a minor tweak here: instead of retesting '*e' in 'while' loop
> header, do 'if (!*e) return 0;' after __env_change; this helps the compiler to
> generate slightly cleaner code.

OK.

Rich

Re: [PATCH 1/3] overhaul environment functions

[Alexander Monakov]

On Sat, 19 Mar 2016, Rich Felker wrote:
> > > > +	static char **oldenv;
> > > > +	char **newenv;
> > > > +	if (__environ == oldenv) {
> > > > +		newenv = realloc(oldenv, sizeof *newenv * (i+2));
> > > > +		if (!newenv) goto oom;
> > > > +	} else {
> > > > +		newenv = malloc(sizeof *newenv * (i+2));
> > > > +		if (!newenv) goto oom;
> > > > +		if (i) memcpy(newenv, __environ, sizeof *newenv * i);
> > > > +		free(oldenv);
> > > > +	}
> > > 
> > > Rather than using malloc when __environ != oldenv, I think we should
> > > use realloc on oldenv, so that we don't leak internally-allocated
> > > environ arrays if the program repeatedly does environ=0 or calls your
> > > new clearenv.
> > 
> > How can we leak internally allocated environ here? If there's one, oldenv
> > points to it, and we free it right after memcpy.
> 
> for (;;) { clearenv(); putenv("FOO=BAR"); }
> 
> Since __environ!=oldenv at the time of every putenv call, each call
> allocates a new environment array and the old one is leaked.

But the old one is not leaked: like I said, 'free(oldenv)' frees it.

> > > >  	if (!overwrite && getenv(var)) return 0;
> > > >  
> > > > -	l1 = strlen(var);
> > > >  	l2 = strlen(value);
> > > >  	s = malloc(l1+l2+2);
> > > > -	if (s) {
> > > > -		memcpy(s, var, l1);
> > > > -		s[l1] = '=';
> > > > -		memcpy(s+l1+1, value, l2);
> > > > -		s[l1+l2+1] = 0;
> > > > -		if (!__putenv(s, 1)) return 0;
> > > > -	}
> > > > -	free(s);
> > > > -	return -1;
> > > > +	if (!s) return -1;
> > > > +	memcpy(s, var, l1);
> > > > +	s[l1] = '=';
> > > > +	memcpy(s+l1+1, value, l2+1);
> > > > +	return __putenv(s, l1, s);
> > > >  }
> > > 
> > > This leaks when __putenv fails. It's no worse than before but should
> > > probably be fixed.
> > 
> > Only when __env_change, specifically, "fails"; not when __putenv OOMs.
> 
> Why? s points to memory allocated by malloc, and I don't see anywhere
> it's freed if __putenv fails.

In old code there's explicit 'free(s)' on fallthrough path if __putenv
returned -1; in new code __putenv frees its last argument on oom itself
(at 'oom:' label).

Alexander

Re: [PATCH 1/3] overhaul environment functions

[Rich Felker]

On Sun, Mar 20, 2016 at 07:15:52AM +0300, Alexander Monakov wrote:
> On Sat, 19 Mar 2016, Rich Felker wrote:
> > > > > +	static char **oldenv;
> > > > > +	char **newenv;
> > > > > +	if (__environ == oldenv) {
> > > > > +		newenv = realloc(oldenv, sizeof *newenv * (i+2));
> > > > > +		if (!newenv) goto oom;
> > > > > +	} else {
> > > > > +		newenv = malloc(sizeof *newenv * (i+2));
> > > > > +		if (!newenv) goto oom;
> > > > > +		if (i) memcpy(newenv, __environ, sizeof *newenv * i);
> > > > > +		free(oldenv);
> > > > > +	}
> > > > 
> > > > Rather than using malloc when __environ != oldenv, I think we should
> > > > use realloc on oldenv, so that we don't leak internally-allocated
> > > > environ arrays if the program repeatedly does environ=0 or calls your
> > > > new clearenv.
> > > 
> > > How can we leak internally allocated environ here? If there's one, oldenv
> > > points to it, and we free it right after memcpy.
> > 
> > for (;;) { clearenv(); putenv("FOO=BAR"); }
> > 
> > Since __environ!=oldenv at the time of every putenv call, each call
> > allocates a new environment array and the old one is leaked.
> 
> But the old one is not leaked: like I said, 'free(oldenv)' frees it.

You're right; I missed this. So it looks to me like you're doing
exactly the right thing.

> > > > >  	if (!overwrite && getenv(var)) return 0;
> > > > >  
> > > > > -	l1 = strlen(var);
> > > > >  	l2 = strlen(value);
> > > > >  	s = malloc(l1+l2+2);
> > > > > -	if (s) {
> > > > > -		memcpy(s, var, l1);
> > > > > -		s[l1] = '=';
> > > > > -		memcpy(s+l1+1, value, l2);
> > > > > -		s[l1+l2+1] = 0;
> > > > > -		if (!__putenv(s, 1)) return 0;
> > > > > -	}
> > > > > -	free(s);
> > > > > -	return -1;
> > > > > +	if (!s) return -1;
> > > > > +	memcpy(s, var, l1);
> > > > > +	s[l1] = '=';
> > > > > +	memcpy(s+l1+1, value, l2+1);
> > > > > +	return __putenv(s, l1, s);
> > > > >  }
> > > > 
> > > > This leaks when __putenv fails. It's no worse than before but should
> > > > probably be fixed.
> > > 
> > > Only when __env_change, specifically, "fails"; not when __putenv OOMs.
> > 
> > Why? s points to memory allocated by malloc, and I don't see anywhere
> > it's freed if __putenv fails.
> 
> In old code there's explicit 'free(s)' on fallthrough path if __putenv
> returned -1; in new code __putenv frees its last argument on oom itself
> (at 'oom:' label).

I missed that. Should be OK, then.

After debunking most of my concerns, are there any things left to
change in patch 1, or should I go ahead and apply it?

Rich

Re: [PATCH 1/3] overhaul environment functions

[Alexander Monakov]

On Sun, 20 Mar 2016, Rich Felker wrote:
> After debunking most of my concerns, are there any things left to
> change in patch 1, or should I go ahead and apply it?

Let me send a v2 with two changes: reinstating setenv EINVAL on null arg,
and tweaking unsetenv like I mentioned. Should be good with those changes,
I think.

Thanks.
Alexander

Re: [PATCH 1/3] overhaul environment functions

[Alexander Monakov]

On Sun, 20 Mar 2016, Alexander Monakov wrote:
> On Sun, 20 Mar 2016, Rich Felker wrote:
> > After debunking most of my concerns, are there any things left to
> > change in patch 1, or should I go ahead and apply it?
> 
> Let me send a v2 with two changes: reinstating setenv EINVAL on null arg,
> and tweaking unsetenv like I mentioned. Should be good with those changes,
> I think.

I've attempted another go at testing, and it turns out the new code still has
a memory leak, although a subtler and smaller one. It is caused by failure to
use existing null slots in __env_change when it is called with p != 0, but no
slot in env_alloced matches p (so the early-out by replacing p by r and
freeing p does not happen).

I'm quite surprised I didn't catch this sooner, because the testcase for the
existing leak (alternating setenv/putenv calls) also exposes this one, albeit
at a slower rate. I've probably made some mistake like misinterpreting the
exit status; not sure.

The following incremental diff shows how I have addressed this issue. I'm
leaving it for comments, and will send an updated patch with two changes
mentioned above, plus __env_change renamed to __env_rm_add, some time later.

Alexander

diff --git a/src/env/setenv.c b/src/env/setenv.c
index db9d72a..051c8d0 100644
--- a/src/env/setenv.c
+++ b/src/env/setenv.c
@@ -9,17 +9,23 @@ void __env_change(char *p, char *r)
 {
        static char **env_alloced;
        static size_t env_alloced_n;
+       char **nullslot = 0;
        for (size_t i=0; i < env_alloced_n; i++)
                if (env_alloced[i] == p) {
                        env_alloced[i] = r;
                        free(p);
                        return;
+               } else if (!env_alloced[i]) {
+                       nullslot = env_alloced + i;
                }
        if (!r) return;
-       char **new_ea = realloc(env_alloced, sizeof *new_ea * (env_alloced_n+1));
-       if (!new_ea) return;
-       new_ea[env_alloced_n++] = r;
-       env_alloced = new_ea;
+       if (!nullslot) {
+               char **t = realloc(env_alloced, sizeof *t * (env_alloced_n+1));
+               if (!t) return;
+               env_alloced = t;
+               nullslot = env_alloced + env_alloced_n++;
+       }
+       *nullslot = r;
 }
 
 int setenv(const char *var, const char *value, int overwrite)

[PATCH 2/3] env: remove duplicates when adding to environment

[Alexander Monakov]

Potential presence of multiple entries for the same name in the
environment can be problematic for applications that mix libc calls
and direct lookups via 'environ'. Removing duplicates of the entry
being set via setenv/putenv is a simple partial mitigation.
---
This was raised recently on the glibc bugzilla, and Rich commented on IRC that
removing all duplicates in the environment at startup is costly, but we could
consider changing setenv/putenv to remove the duplicates of the entry being
set. I'm afraid that wouldn't help the applications that are actually
affected. However, since both setenv and putenv work via __putenv, the
corresponding patch is not too big, so here's one possible approach.

 src/env/putenv.c   |  3 ++-
 src/env/unsetenv.c | 21 +++++++++++++--------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/src/env/putenv.c b/src/env/putenv.c
index 39a71be..facae1d 100644
--- a/src/env/putenv.c
+++ b/src/env/putenv.c
@@ -3,6 +3,7 @@
 #include "libc.h"
 
 char *__strchrnul(const char *, int);
+int __unsetenv(const char *, size_t, char **);
 
 static void dummy(char *p, char *r) {}
 weak_alias(dummy, __env_change);
@@ -16,7 +17,7 @@ int __putenv(char *s, size_t l, char *r)
 				char *tmp = __environ[i];
 				__environ[i] = s;
 				__env_change(tmp, r);
-				return 0;
+				return __unsetenv(s, l, __environ+i+1);
 			}
 	static char **oldenv;
 	char **newenv;
diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c
index 86873cd..5e5727f 100644
--- a/src/env/unsetenv.c
+++ b/src/env/unsetenv.c
@@ -8,15 +8,9 @@ char *__strchrnul(const char *, int);
 static void dummy(char *p, char *r) {}
 weak_alias(dummy, __env_change);
 
-int unsetenv(const char *name)
+int __unsetenv(const char *name, size_t l, char **e)
 {
-	size_t l = __strchrnul(name, '=') - name;
-	if (!l || name[l]) {
-		errno = EINVAL;
-		return -1;
-	}
-	if (!__environ) return 0;
-	for (char **e = __environ; *e; e++)
+	for (; *e; e++)
 		while (*e && !strncmp(name, *e, l) && l[*e] == '=') {
 			char **ee = e, *tmp = *e;
 			do *ee = *(ee+1);
@@ -25,3 +19,14 @@ int unsetenv(const char *name)
 		}
 	return 0;
 }
+
+int unsetenv(const char *name)
+{
+	size_t l = __strchrnul(name, '=') - name;
+	if (!l || name[l]) {
+		errno = EINVAL;
+		return -1;
+	}
+	if (!__environ) return 0;
+	return __unsetenv(name, l, __environ);
+}
-- 
2.1.3

[PATCH 3/3] env: free allocations in clearenv

[Alexander Monakov]

This aligns clearenv with the Linux man page by setting 'environ'
rather than '*environ' to NULL, and stops it from leaking entries
allocated by the libc.
---
 src/env/clearenv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/env/clearenv.c b/src/env/clearenv.c
index 62d5095..5c16063 100644
--- a/src/env/clearenv.c
+++ b/src/env/clearenv.c
@@ -1,10 +1,14 @@
 #define _GNU_SOURCE
 #include <stdlib.h>
+#include "libc.h"
 
-extern char **__environ;
+static void dummy(char *p, char *r) {}
+weak_alias(dummy, __env_change);
 
 int clearenv()
 {
-	__environ[0] = 0;
+	char **e = __environ;
+	__environ = 0;
+	if (e) while (*e) __env_change(*e++, 0);
 	return 0;
 }
-- 
2.1.3