hush: rework variable storage and environment handling.

More that -100 bytes of code + memory leak plugged.
Added a testcase for it.

4 files changed, 238 insertions, 167 deletions

diff --git a/shell/README b/shell/README
index a9b4346..a09353d 100644
--- a/shell/README
+++ b/shell/README
@@ -1,6 +1,10 @@
 Various bits of what is known about busybox shells, in no particular order.
 
 2007-05-24
+hush: environment-related memory leak plugged, with net code size
+decrease.
+
+2007-05-24
 hush: '( echo ${name )' will show syntax error message, but prompt
 doesn't return (need to press <enter>). Pressing Ctrl-C, <enter>,
 '( echo ${name )' again, Ctrl-C segfaults.
diff --git a/shell/hush.c b/shell/hush.c
index d96615d..0b8e7d6 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -37,7 +37,6 @@
  * across continuation lines.
  *
  * Bash grammar not implemented: (how many of these were in original sh?)
- *      $@ (those sure look like weird quoting rules)
  *      $_
  *      ! negation operator for pipes
  *      &> and >& redirection of stdout+stderr
@@ -279,11 +278,18 @@ struct close_me {
 	int fd;
 };
 
-struct variables {
-	struct variables *next;
-	const char *name;
-	const char *value;
-	smallint flg_export;
+/* On program start, environ points to initial environment.
+ * putenv adds new pointers into it, unsetenv removes them.
+ * Neither of these (de)allocates the strings.
+ * setenv allocates new strings in malloc space and does putenv,
+ * and thus setenv is unusable (leaky) for shell's purposes */
+#define setenv(...) setenv_is_leaky_dont_use()
+struct variable {
+	struct variable *next;
+	char *name;          /* points to "name=" portion */
+	char *value;         /* points directly after "=" */
+	int max_len;         /* if > 0, name is part of initial env; else name is malloced */
+	smallint flg_export; /* putenv should be done on this var */
 	smallint flg_read_only;
 };
 
@@ -322,14 +328,21 @@ enum {
 	CHAR_SPECIAL            = 3, /* example: $ */
 };
 
+#define HUSH_VER_STR "0.02"
 
-/* "Globals" within this file */
+static const char version_str[] = "HUSH_VERSION="HUSH_VER_STR;
 
-#define HUSH_VER_STR "0.02"
-static const struct variables const_shell_ver = {
-	NULL, "HUSH_VERSION", HUSH_VER_STR, 1, 1
+static const struct variable const_shell_ver = {
+	.next = NULL,
+	.name = (char*)version_str,
+	.value = (char*)version_str + sizeof("HUSH_VERSION=")-1,
+	.max_len = 1, /* 0 can provoke free(name) */
+	.flg_export = 1,
+	.flg_read_only = 1,
 };
 
+/* "Globals" within this file */
+
 /* Sorted roughly by size (smaller offsets == smaller code) */
 struct globals {
 #if ENABLE_HUSH_INTERACTIVE
@@ -360,8 +373,8 @@ struct globals {
 	struct close_me *close_me_head;
 	const char *cwd;
 	unsigned last_bg_pid;
-	struct variables *top_vars; /* = &shell_ver (both are set in main()) */
-	struct variables shell_ver; /* = const_shell_ver */
+	struct variable *top_var; /* = &shell_ver (both are set in main()) */
+	struct variable shell_ver; /* = const_shell_ver */
 #if ENABLE_FEATURE_SH_STANDALONE
 	struct nofork_save_area nofork_save;
 #endif
@@ -407,7 +420,7 @@ enum { run_list_level = 0 };
 #define close_me_head    (G.close_me_head   )
 #define cwd              (G.cwd             )
 #define last_bg_pid      (G.last_bg_pid     )
-#define top_vars         (G.top_vars        )
+#define top_var          (G.top_var         )
 #define shell_ver        (G.shell_ver       )
 #if ENABLE_FEATURE_SH_STANDALONE
 #define nofork_save      (G.nofork_save     )
@@ -541,8 +554,8 @@ static char **expand_strvec_to_strvec(char **argv);
 static char *expand_strvec_to_string(char **argv);
 /* used for expansion of right hand of assignments */
 static char *expand_string_to_string(const char *str);
-static const char *get_local_var(const char *var);
-static int set_local_var(const char *s, int flg_export);
+static struct variable *get_local_var(const char *var);
+static int set_local_var(char *s, int flg_export);
 static void unset_local_var(const char *name);
 
 /* Table of built-in functions.  They can be forked or not, depending on
@@ -795,7 +808,7 @@ static int builtin_exit(char **argv)
 /* built-in 'export VAR=value' handler */
 static int builtin_export(char **argv)
 {
-	int res = 0;
+	const char *value;
 	char *name = argv[1];
 
 	if (name == NULL) {
@@ -810,36 +823,23 @@ static int builtin_export(char **argv)
 		return EXIT_SUCCESS;
 	}
 
-	name = xstrdup(name);
-	{
-		const char *value = strchr(name, '=');
-
-		if (!value) {
-			char *tmp;
-			/* They are exporting something without an =VALUE */
-
-			value = get_local_var(name);
-			if (value) {
-				size_t ln = strlen(name);
+	value = strchr(name, '=');
+	if (!value) {
+		/* They are exporting something without a =VALUE */
+		struct variable *var;
 
-				tmp = xrealloc(name, ln+strlen(value)+2);
-				sprintf(tmp+ln, "=%s", value);
-				name = tmp;
-			} else {
-				/* bash does not return an error when trying to export
-				 * an undefined variable.  Do likewise. */
-				res = 1;
-			}
+		var = get_local_var(name);
+		if (var) {
+			var->flg_export = 1;
+			putenv(var->name);
 		}
+		/* bash does not return an error when trying to export
+		 * an undefined variable.  Do likewise. */
+		return EXIT_SUCCESS;
 	}
-	if (res < 0)
-		bb_perror_msg("export");
-	else if (res == 0)
-		res = set_local_var(name, 1);
-	else
-		res = 0;
-	free(name);
-	return res;
+
+	set_local_var(xstrdup(name), 1);
+	return EXIT_SUCCESS;
 }
 
 #if ENABLE_HUSH_JOB
@@ -965,20 +965,20 @@ static int builtin_read(char **argv)
 	/* read string. name_len+1 chars are already used by 'name=' */
 	fgets(p, sizeof(string) - 1 - name_len, stdin);
 	chomp(p);
-	return set_local_var(string, 0);
+	return set_local_var(xstrdup(string), 0);
 }
 
 /* built-in 'set [VAR=value]' handler */
 static int builtin_set(char **argv)
 {
 	char *temp = argv[1];
-	struct variables *e;
+	struct variable *e;
 
 	if (temp == NULL)
-		for (e = top_vars; e; e = e->next)
-			printf("%s=%s\n", e->name, e->value);
+		for (e = top_var; e; e = e->next)
+			puts(e->name);
 	else
-		set_local_var(temp, 0);
+		set_local_var(xstrdup(temp), 0);
 
 	return EXIT_SUCCESS;
 }
@@ -1742,26 +1742,9 @@ static int run_pipe_real(struct pipe *pi)
 		if (i != 0 && argv[i] == NULL) {
 			/* assignments, but no command: set the local environment */
 			for (i = 0; argv[i] != NULL; i++) {
-				/* Ok, this case is tricky.  We have to decide if this is a
-				 * local variable, or an already exported variable.  If it is
-				 * already exported, we have to export the new value.  If it is
-				 * not exported, we need only set this as a local variable.
-				 * This junk is all to decide whether or not to export this
-				 * variable. */
-				int export_me = 0;
-				char *name, *value;
-				name = xstrdup(argv[i]);
-				debug_printf("local environment set: %s\n", name);
-				value = strchr(name, '=');
-				if (value)
-					*value = '\0';
-				if (get_local_var(name)) {
-					export_me = 1;
-				}
-				free(name);
+				debug_printf("local environment set: %s\n", argv[i]);
 				p = expand_string_to_string(argv[i]);
-				set_local_var(p, export_me);
-				free(p);
+				set_local_var(p, 0);
 			}
 			return EXIT_SUCCESS;   /* don't worry about errors in set_local_var() yet */
 		}
@@ -2693,109 +2676,115 @@ static char* expand_strvec_to_string(char **argv)
 }
 
 /* This is used to get/check local shell variables */
-static const char *get_local_var(const char *s)
+static struct variable *get_local_var(const char *s)
 {
-	struct variables *cur;
+	struct variable *cur;
+	int len;
 
 	if (!s)
 		return NULL;
-	for (cur = top_vars; cur; cur = cur->next) {
-		if (strcmp(cur->name, s) == 0)
-			return cur->value;
+	len = strlen(s);
+	for (cur = top_var; cur; cur = cur->next) {
+		if (strncmp(cur->name, s, len) == 0 && cur->name[len] == '=')
+			return cur;
 	}
 	return NULL;
 }
 
-/* This is used to set local shell variables
-   flg_export == 0 if only local (not exporting) variable
-   flg_export == 1 if "new" exporting environ
-   flg_export > 1  if current startup environ (not call putenv()) */
-static int set_local_var(const char *s, int flg_export)
+/* name holds "NAME=VAL" and is expected to be malloced.
+ * We take ownership of it. */
+static int set_local_var(char *name, int flg_export)
 {
-	char *name, *value;
-	int result = 0;
-	struct variables *cur;
-
-	name = xstrdup(s);
+	struct variable *cur;
+	char *value;
+	int name_len;
 
-	/* Assume when we enter this function that we are already in
-	 * NAME=VALUE format.  So the first order of business is to
-	 * split 's' on the '=' into 'name' and 'value' */
 	value = strchr(name, '=');
-	/*if (value == 0 && ++value == 0) ??? -vda */
-	if (value == NULL || value[1] == '\0') {
+	if (!value) { /* not expected to ever happen? */
 		free(name);
 		return -1;
 	}
-	*value++ = '\0';
 
-	for (cur = top_vars; cur; cur = cur->next) {
-		if (strcmp(cur->name, name) == 0) {
-			if (strcmp(cur->value, value) == 0) {
-				if (flg_export && !cur->flg_export)
-					cur->flg_export = flg_export;
-				else
-					result++;
-			} else if (cur->flg_read_only) {
-				bb_error_msg("%s: readonly variable", name);
-				result = -1;
-			} else {
-				if (flg_export > 0 || cur->flg_export > 1)
-					cur->flg_export = 1;
-				free((char*)cur->value);
-				cur->value = xstrdup(value);
+	name_len = value - name;
+	cur = top_var; /* cannot be NULL (we have HUSH_VERSION and it's RO) */
+	while (1) {
+		if (strncmp(cur->name, name, name_len) != 0 || cur->name[name_len] != '=') {
+			if (!cur->next) {
+				/* cur points to last var in linked list */
+				break;
 			}
-			goto skip;
+			cur = cur->next;
+			continue;
 		}
-	}
-
-	cur = xzalloc(sizeof(*cur));
-	/*cur->next = 0;*/
-	cur->name = xstrdup(name);
-	cur->value = xstrdup(value);
-	cur->flg_export = flg_export;
-	/*cur->flg_read_only = 0;*/
-	{
-		struct variables *bottom = top_vars;
-		while (bottom->next)
-			bottom = bottom->next;
-		bottom->next = cur;
-	}
- skip:
-	if (result == 0 && cur->flg_export == 1) {
-		*(value-1) = '=';
-		result = putenv(name);
-	} else {
-		free(name);
-		if (result > 0)            /* equivalent to previous set */
-			result = 0;
-	}
-	return result;
+		/* We already have a var with this name */
+		if (cur->flg_read_only) {
+			bb_error_msg("%s: readonly variable", name);
+			free(name);
+			return -1;
+		}
+		*value = '\0';
+		unsetenv(name); /* just in case */
+		*value++ = '=';
+		if (strcmp(cur->value, value) == 0) {
+ free_and_exp:
+			free(name);
+			goto exp;
+		}
+		if (cur->max_len >= strlen(name)) {
+			/* This one is from startup env, reuse space */
+			strcpy(cur->name, name);
+			goto free_and_exp;
+		}
+		/* max_len == 0 signifies "malloced" var, which we can
+		 * (and has to) free */
+		if (!cur->max_len)
+			free(cur->name);
+		cur->max_len = 0;
+		goto set_name_and_exp;
+	}
+
+	/* Not found - create next variable struct */
+	cur->next = xzalloc(sizeof(*cur));
+	cur = cur->next;
+
+ set_name_and_exp:
+	cur->name = name;
+ exp:
+	cur->value = cur->name + name_len + 1;
+	if (flg_export)
+		cur->flg_export = 1;
+	if (cur->flg_export)
+		return putenv(cur->name);
+	return 0;
 }
 
 static void unset_local_var(const char *name)
 {
-	struct variables *cur, *next;
+	struct variable *cur;
+	struct variable *prev = prev; /* for gcc */
+	int name_len;
 
 	if (!name)
 		return;
-	for (cur = top_vars; cur; cur = cur->next) {
-		if (strcmp(cur->name, name) == 0) {
+	name_len = strlen(name);
+	cur = top_var;
+	while (cur) {
+		if (strncmp(cur->name, name, name_len) == 0 && cur->name[name_len] == '=') {
 			if (cur->flg_read_only) {
 				bb_error_msg("%s: readonly variable", name);
 				return;
 			}
-			if (cur->flg_export)
-				unsetenv(cur->name);
-			free((char*)cur->name);
-			free((char*)cur->value);
-			next = top_vars;
-			while (next->next != cur)
-				next = next->next;
-			next->next = cur->next;
+		/* prev is ok to use here because 1st variable, HUSH_VERSION,
+		 * is ro, and we cannot reach this code on the 1st pass */
+			prev->next = cur->next;
+			unsetenv(cur->name);
+			if (!cur->max_len)
+				free(cur->name);
 			free(cur);
 			return;
 		}
+		prev = cur;
+		cur = cur->next;
 	}
 }
 
@@ -3265,13 +3254,10 @@ static int parse_group(o_string *dest, struct p_context *ctx,
  * see the bash man page under "Parameter Expansion" */
 static const char *lookup_param(const char *src)
 {
-	const char *p = NULL;
-	if (src) {
-		p = getenv(src);
-		if (!p)
-			p = get_local_var(src);
-	}
-	return p;
+	struct variable *var = get_local_var(src);
+	if (var)
+		return var->value;
+	return NULL;
 }
 
 /* return code: 0 for OK, 1 for syntax error */
@@ -3681,10 +3667,29 @@ int hush_main(int argc, char **argv)
 	int opt;
 	FILE *input;
 	char **e;
+	struct variable *cur_var;
 
 	PTR_TO_GLOBALS = xzalloc(sizeof(G));
-	top_vars = &shell_ver;
+
 	shell_ver = const_shell_ver; /* copying struct here */
+	top_var = &shell_ver;
+	/* initialize our shell local variables with the values
+	 * currently living in the environment */
+	e = environ;
+	cur_var = top_var;
+	if (e) while (*e) {
+		char *value = strchr(*e, '=');
+		if (value) { /* paranoia */
+			cur_var->next = xzalloc(sizeof(*cur_var));
+			cur_var = cur_var->next;
+			cur_var->name = *e;
+			cur_var->value = value + 1;
+			cur_var->max_len = strlen(*e);
+			cur_var->flg_export = 1;
+		}
+		e++;
+	}
+	putenv(shell_ver.name);
 
 #if ENABLE_FEATURE_EDITING
 	line_input_state = new_line_input_t(FOR_SHELL);
@@ -3701,14 +3706,8 @@ int hush_main(int argc, char **argv)
 	PS2 = "> ";
 #endif
 
-	/* initialize our shell local variables with the values
-	 * currently living in the environment */
-	e = environ;
-	if (e)
-		while (*e)
-			set_local_var(*e++, 2);   /* without call putenv() */
-
-	last_return_code = EXIT_SUCCESS;
+	if (EXIT_SUCCESS) /* otherwise is already done */
+		last_return_code = EXIT_SUCCESS;
 
 	if (argv[0] && argv[0][0] == '-') {
 		debug_printf("sourcing /etc/profile\n");
@@ -3818,23 +3817,20 @@ int hush_main(int argc, char **argv)
 	input = xfopen(argv[optind], "r");
 	opt = parse_and_run_file(input);
 
+ final_return:
+
 #if ENABLE_FEATURE_CLEAN_UP
 	fclose(input);
 	if (cwd != bb_msg_unknown)
 		free((char*)cwd);
-	{
-		struct variables *cur, *tmp;
-		for (cur = top_vars; cur; cur = tmp) {
-			tmp = cur->next;
-			if (!cur->flg_read_only) {
-				free((char*)cur->name);
-				free((char*)cur->value);
-				free(cur);
-			}
-		}
+	cur_var = top_var->next;
+	while (cur_var) {
+		struct variable *tmp = cur_var;
+		if (!cur_var->max_len)
+			free(cur_var->name);
+		cur_var = cur_var->next;
+		free(tmp);
 	}
 #endif
-
- final_return:
 	hush_exit(opt ? opt : last_return_code);
 }
diff --git a/shell/hush_test/hush-z_slow/leak_var.right b/shell/hush_test/hush-z_slow/leak_var.right
new file mode 100644
index 0000000..7bccc1e
--- /dev/null
+++ b/shell/hush_test/hush-z_slow/leak_var.right
@@ -0,0 +1,2 @@
+Measuring memory leak...
+vsz does not grow
diff --git a/shell/hush_test/hush-z_slow/leak_var.tests b/shell/hush_test/hush-z_slow/leak_var.tests
new file mode 100755
index 0000000..d3ca259
--- /dev/null
+++ b/shell/hush_test/hush-z_slow/leak_var.tests
@@ -0,0 +1,69 @@
+pid=$$
+
+# Warm up
+unset t
+t=111111111111111111111111111111111111111111111111111111111111111111111111
+export t
+unset t
+t=111111111111111111111111111111111111111111111111111111111111111111111111
+export t
+unset t
+t=111111111111111111111111111111111111111111111111111111111111111111111111
+export t
+unset t
+t=111111111111111111111111111111111111111111111111111111111111111111111111
+export t
+unset t
+t=111111111111111111111111111111111111111111111111111111111111111111111111
+export t
+i=1
+if test $i = 1111111111111111111111111111111111111111111111; then i=2; fi
+beg=`ps -o pid,vsz | grep "^ *$pid "`
+
+echo "Measuring memory leak..."
+beg=`ps -o pid,vsz | grep "^ *$pid "`
+i=1
+while test $i != X; do
+    unset t
+    t=111111111111111111111111111111111111111111111111111111111111111111111111
+    export t
+    unset t
+    t=111111111111111111111111111111111111111111111111111111111111111111111111
+    export t
+    unset t
+    t=111111111111111111111111111111111111111111111111111111111111111111111111
+    export t
+    unset t
+    t=111111111111111111111111111111111111111111111111111111111111111111111111
+    export t
+    unset t
+    t=111111111111111111111111111111111111111111111111111111111111111111111111
+    export t
+    i=1$i
+    if test $i = 1111111111111111111111111111111111111111111111; then i=2; fi
+    if test $i = 1111111111111111111111111111111111111111111112; then i=3; fi
+    if test $i = 1111111111111111111111111111111111111111111113; then i=4; fi
+    if test $i = 1111111111111111111111111111111111111111111114; then i=5; fi
+    if test $i = 1111111111111111111111111111111111111111111115; then i=6; fi
+    if test $i = 1111111111111111111111111111111111111111111116; then i=7; fi
+    if test $i = 1111111111111111111111111111111111111111111117; then i=8; fi
+    if test $i = 1111111111111111111111111111111111111111111118; then i=9; fi
+    if test $i = 1111111111111111111111111111111111111111111119; then i=a; fi
+    if test $i = 111111111111111111111111111111111111111111111a; then i=b; fi
+    if test $i = 111111111111111111111111111111111111111111111b; then i=c; fi
+    if test $i = 111111111111111111111111111111111111111111111c; then i=d; fi
+    if test $i = 111111111111111111111111111111111111111111111d; then i=e; fi
+    if test $i = 111111111111111111111111111111111111111111111e; then i=f; fi
+    if test $i = 111111111111111111111111111111111111111111111f; then i=g; fi
+    if test $i = 111111111111111111111111111111111111111111111g; then i=h; fi
+    if test $i = 111111111111111111111111111111111111111111111h; then i=i; fi
+    if test $i = 111111111111111111111111111111111111111111111i; then i=j; fi
+    if test $i = 111111111111111111111111111111111111111111111j; then i=X; fi
+done
+end=`ps -o pid,vsz | grep "^ *$pid "`
+
+if test "$beg" != "$end"; then
+    echo "vsz grows: $beg -> $end"
+else
+    echo "vsz does not grow"
+fi