← Commit list

d76c049cc

Denis Vlasenko
Denis Vlasenko
2007-05-25
FAILED Duration: 12s

Subject

hush: rework variable storage and environment handling. More that -100 bytes of code + memory leak plugged. Added a testcase for it.

Binary

No binary (build failed)

Architecture & Compiler Flags

Architecture: x86_64-linux-gnu Compiler: gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0

—

Build Log

(log yok)

Commit Diff GitHub →

shell/README +4 -0shell/hush.c +163 -167shell/hush_test/hush-z_slow/leak_var.right +2 -0shell/hush_test/hush-z_slow/leak_var.tests +69 -0
shell/README+4 -0
@@ -1,5 +1,9 @@
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>,
shell/hush.c+163 -167
@@ -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);
}
shell/hush_test/hush-z_slow/leak_var.right+2 -0
@@ -0,0 +1,2 @@
+Measuring memory leak...
+vsz does not grow
shell/hush_test/hush-z_slow/leak_var.tests+69 -0
@@ -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