the environment handling code was showing its age. just because environ

is a char** array doesn't mean we must exclusively operate on such.
convert to a red-black tree, manipulate as desired, then flatten to array.
potentially overkill for the current operations, but reading the tea leaves
i see that more manipulations are desired.
ok tb
(and some thought provoking disagreement from martijn)

4 files changed, 180 insertions, 104 deletions

diff --git a/usr.bin/doas/Makefile b/usr.bin/doas/Makefile
index 809fe0b71cb..7fc38df14c7 100644
--- a/usr.bin/doas/Makefile
+++ b/usr.bin/doas/Makefile
@@ -1,6 +1,6 @@
-#	$OpenBSD: Makefile,v 1.1 2015/07/16 20:44:21 tedu Exp $
+#	$OpenBSD: Makefile,v 1.2 2016/06/16 17:40:30 tedu Exp $
 
-SRCS=	parse.y doas.c
+SRCS=	parse.y doas.c env.c
 
 PROG=	doas
 MAN=	doas.1 doas.conf.5
diff --git a/usr.bin/doas/doas.c b/usr.bin/doas/doas.c
index 2998936ea36..a542a769aa0 100644
--- a/usr.bin/doas/doas.c
+++ b/usr.bin/doas/doas.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: doas.c,v 1.55 2016/06/07 16:49:23 tedu Exp $ */
+/* $OpenBSD: doas.c,v 1.56 2016/06/16 17:40:30 tedu Exp $ */
 /*
  * Copyright (c) 2015 Ted Unangst <tedu@openbsd.org>
  *
@@ -182,105 +182,6 @@ parseconfig(const char *filename, int checkperms)
 		exit(1);
 }
 
-/*
- * Copy the environment variables in safeset from oldenvp to envp.
- */
-static int
-copyenvhelper(const char **oldenvp, const char **safeset, int nsafe,
-    char **envp, int ei)
-{
-	int i;
-
-	for (i = 0; i < nsafe; i++) {
-		const char **oe = oldenvp;
-		while (*oe) {
-			size_t len = strlen(safeset[i]);
-			if (strncmp(*oe, safeset[i], len) == 0 &&
-			    (*oe)[len] == '=') {
-				if (!(envp[ei++] = strdup(*oe)))
-					err(1, "strdup");
-				break;
-			}
-			oe++;
-		}
-	}
-	return ei;
-}
-
-static char **
-copyenv(const char **oldenvp, struct rule *rule)
-{
-	const char *safeset[] = {
-		"DISPLAY", "HOME", "LOGNAME", "MAIL",
-		"PATH", "TERM", "USER", "USERNAME",
-		NULL
-	};
-	const char *badset[] = {
-		"ENV",
-		NULL
-	};
-	char **envp;
-	const char **extra;
-	int ei;
-	int nsafe, nbad;
-	int nextras = 0;
-
-	/* if there was no envvar whitelist, pass all except badset ones */
-	nbad = arraylen(badset);
-	if ((rule->options & KEEPENV) && !rule->envlist) {
-		size_t iold, inew;
-		size_t oldlen = arraylen(oldenvp);
-		envp = reallocarray(NULL, oldlen + 1, sizeof(char *));
-		if (!envp)
-			err(1, "reallocarray");
-		for (inew = iold = 0; iold < oldlen; iold++) {
-			size_t ibad;
-			for (ibad = 0; ibad < nbad; ibad++) {
-				size_t len = strlen(badset[ibad]);
-				if (strncmp(oldenvp[iold], badset[ibad], len) == 0 &&
-				    oldenvp[iold][len] == '=') {
-					break;
-				}
-			}
-			if (ibad == nbad) {
-				if (!(envp[inew] = strdup(oldenvp[iold])))
-					err(1, "strdup");
-				inew++;
-			}
-		}
-		envp[inew] = NULL;
-		return envp;
-	}
-
-	nsafe = arraylen(safeset);
-	if ((extra = rule->envlist)) {
-		size_t isafe;
-		nextras = arraylen(extra);
-		for (isafe = 0; isafe < nsafe; isafe++) {
-			size_t iextras;
-			for (iextras = 0; iextras < nextras; iextras++) {
-				if (strcmp(extra[iextras], safeset[isafe]) == 0) {
-					nextras--;
-					extra[iextras] = extra[nextras];
-					extra[nextras] = NULL;
-					iextras--;
-				}
-			}
-		}
-	}
-
-	envp = reallocarray(NULL, nsafe + nextras + 1, sizeof(char *));
-	if (!envp)
-		err(1, "can't allocate new environment");
-
-	ei = 0;
-	ei = copyenvhelper(oldenvp, safeset, nsafe, envp, ei);
-	ei = copyenvhelper(oldenvp, rule->envlist, nextras, envp, ei);
-	envp[ei] = NULL;
-
-	return envp;
-}
-
 static void __dead
 checkconfig(const char *confpath, int argc, char **argv,
     uid_t uid, gid_t *groups, int ngroups, uid_t target)
@@ -311,6 +212,7 @@ main(int argc, char **argv, char **envp)
 	char *shargv[] = { NULL, NULL };
 	char *sh;
 	const char *cmd;
+	struct env *env;
 	char cmdline[LINE_MAX];
 	char myname[_PW_NAME_LEN + 1];
 	struct passwd *pw;
@@ -471,7 +373,9 @@ main(int argc, char **argv, char **envp)
 	syslog(LOG_AUTHPRIV | LOG_INFO, "%s ran command %s as %s from %s",
 	    myname, cmdline, pw->pw_name, cwd);
 
-	envp = copyenv((const char **)envp, rule);
+	env = createenv(envp);
+	env = filterenv(env, rule);
+	envp = flattenenv(env);
 
 	if (rule->cmd) {
 		if (setenv("PATH", safepath, 1) == -1)
diff --git a/usr.bin/doas/doas.h b/usr.bin/doas/doas.h
index 559d72a21f7..36f93b50c68 100644
--- a/usr.bin/doas/doas.h
+++ b/usr.bin/doas/doas.h
@@ -1,4 +1,19 @@
-/* $OpenBSD: doas.h,v 1.6 2016/06/07 16:49:23 tedu Exp $ */
+/* $OpenBSD: doas.h,v 1.7 2016/06/16 17:40:30 tedu Exp $ */
+
+#include <sys/tree.h>
+
+struct envnode {
+	RB_ENTRY(envnode) node;
+	const char *key;
+	const char *value;
+};
+
+struct env {
+	RB_HEAD(envtree, envnode) root;
+	u_int count;
+};
+
+RB_PROTOTYPE(envtree, envnode, node, envcmp)
 
 struct rule {
 	int action;
@@ -16,6 +31,10 @@ extern int parse_errors;
 
 size_t arraylen(const char **);
 
+struct env *createenv(char **);
+struct env *filterenv(struct env *, struct rule *);
+char **flattenenv(struct env *);
+
 #define PERMIT	1
 #define DENY	2
 
diff --git a/usr.bin/doas/env.c b/usr.bin/doas/env.c
new file mode 100644
index 00000000000..03ce3062bfc
--- /dev/null
+++ b/usr.bin/doas/env.c
@@ -0,0 +1,153 @@
+/* $OpenBSD: env.c,v 1.1 2016/06/16 17:40:30 tedu Exp $ */
+/*
+ * Copyright (c) 2016 Ted Unangst <tedu@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <sys/types.h>
+
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <err.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include "doas.h"
+
+int
+envcmp(struct envnode *a, struct envnode *b)
+{
+	return strcmp(a->key, b->key);
+}
+RB_GENERATE(envtree, envnode, node, envcmp)
+
+struct env *
+createenv(char **envp)
+{
+	struct env *env;
+	u_int i;
+
+	env = malloc(sizeof(*env));
+	if (!env)
+		err(1, NULL);
+	RB_INIT(&env->root);
+	env->count = 0;
+
+	for (i = 0; envp[i] != NULL; i++) {
+		struct envnode *node;
+		const char *e, *eq;
+
+		e = envp[i];
+
+		if ((eq = strchr(e, '=')) == NULL || eq == e)
+			continue;
+		node = malloc(sizeof(*node));
+		if (!node)
+			err(1, NULL);
+		node->key = strndup(envp[i], eq - e);
+		node->value = strdup(eq + 1);
+		if (!node->key || !node->value)
+			err(1, NULL);
+		if (RB_FIND(envtree, &env->root, node)) {
+			free((char *)node->key);
+			free((char *)node->value);
+			free(node);
+		} else {
+			RB_INSERT(envtree, &env->root, node);
+			env->count++;
+		}
+	}
+	return env;
+}
+
+char **
+flattenenv(struct env *env)
+{
+	char **envp;
+	struct envnode *node;
+	u_int i;
+
+	envp = reallocarray(NULL, env->count + 1, sizeof(char *));
+	if (!envp)
+		err(1, NULL);
+	i = 0;
+	RB_FOREACH(node, envtree, &env->root) {
+		if (asprintf(&envp[i], "%s=%s", node->key, node->value) == -1)
+			err(1, NULL);
+		i++;
+	}
+	envp[i] = NULL;
+	return envp;
+}
+
+static void
+copyenv(struct env *orig, struct env *copy, const char **envlist)
+{
+	struct envnode *node, key;
+	u_int i;
+
+	for (i = 0; envlist[i]; i++) {
+		key.key = envlist[i];
+		if ((node = RB_FIND(envtree, &orig->root, &key))) {
+			RB_REMOVE(envtree, &orig->root, node);
+			orig->count--;
+			RB_INSERT(envtree, &copy->root, node);
+			copy->count++;
+		}
+	}
+}
+
+struct env *
+filterenv(struct env *orig, struct rule *rule)
+{
+	const char *safeset[] = {
+		"DISPLAY", "HOME", "LOGNAME", "MAIL",
+		"PATH", "TERM", "USER", "USERNAME",
+		NULL
+	};
+	const char *badset[] = {
+		"ENV",
+		NULL
+	};
+	struct env *copy;
+	struct envnode *node, key;
+	u_int i;
+
+	if ((rule->options & KEEPENV) && !rule->envlist) {
+		for (i = 0; badset[i]; i++) {
+			key.key = badset[i];
+			if ((node = RB_FIND(envtree, &orig->root, &key))) {
+				RB_REMOVE(envtree, &orig->root, node);
+				free((char *)node->key);
+				free((char *)node->value);
+				free(node);
+				orig->count--;
+			}
+		}
+		return orig;
+	}
+
+	copy = malloc(sizeof(*copy));
+	if (!copy)
+		err(1, NULL);
+	RB_INIT(&copy->root);
+	copy->count = 0;
+
+	if (rule->envlist)
+		copyenv(orig, copy, rule->envlist);
+	copyenv(orig, copy, safeset);
+
+	return copy;
+}