[PATCH 1/3] overhaul environment functions

[Alexander Monakov <amonakov@ispras.ru>]

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