[PATCH] slim down and avoid undefined behavior in unsetenv

[Alexander Monakov <amonakov@ispras.ru>]

Implementation of unsetenv used to invoke (a benign) undefined
behavior by using pointer value that was passed to free() in
comparisons against NULL in two loops that would remove that pointer
from __env_map and __environ arrays.

Factor out handling of __env_map into a separate function __env_free
and move it to putenv.c, allowing to make __env_map private to that
file. Do not attempt to preserve order in __env_map when removing the
entry (it is not important).
---
On Mon, 4 Jan 2016, Rich Felker wrote:
> Oh. In that case I guess it's unnecessary to rewind, yes.
> 
> BTW what might be best to do is something like:
> 
> 	char *tmp = __environ[i];
> 	for (j=i ; __environ[j]; j++)
> 		__environ[j] = __environ[j+1];
> 	__env_free(tmp);
> 
> where __env_free has a weak def as a nop and gets its strong def from
> setenv.c or putenv.c. This refactoring would make it possible for
> unsetenv not to pull in free, and the reordering might make it less
> likely for dangerous things to happen in a broken program that reads
> the environment concurrently with unsetenv.

Thanks -- here's a patch.

 src/env/putenv.c   | 15 ++++++++++++++-
 src/env/unsetenv.c | 29 +++++++++++++----------------
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/src/env/putenv.c b/src/env/putenv.c
index 4042869..86b2024 100644
--- a/src/env/putenv.c
+++ b/src/env/putenv.c
@@ -2,7 +2,20 @@
 #include <string.h>
 
 extern char **__environ;
-char **__env_map;
+static char **__env_map;
+
+void __env_free(char *p)
+{
+	if (__env_map)
+		for (char **e = __env_map; *e; e++)
+			if (*e == p) {
+				char **ee = e;
+				while (*(ee+1)) ee++;
+				*e = *ee; *ee = 0;
+				free(p);
+				return;
+			}
+}
 
 int __putenv(char *s, int a)
 {
diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c
index 3569335..f0f369f 100644
--- a/src/env/unsetenv.c
+++ b/src/env/unsetenv.c
@@ -1,31 +1,28 @@
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
+#include "libc.h"
 
 extern char **__environ;
-extern char **__env_map;
+
+static void dummy(char *p) {}
+weak_alias(dummy, __env_free);
 
 int unsetenv(const char *name)
 {
-	int i, j;
 	size_t l = strlen(name);
 
-	if (!*name || strchr(name, '=')) {
+	if (!*name || memchr(name, '=', l)) {
 		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;
-	}
+	for (char **e = __environ; *e; )
+		if (!memcmp(name, *e, l) && l[*e] == '=') {
+			char **ee = e, *tmp = *e;
+			do *ee = *(ee+1);
+			while (*++ee);
+			__env_free(tmp);
+		} else
+			e++;
 	return 0;
 }