From b61e13d24741536fc713dab1c10a4925197b2072 Mon Sep 17 00:00:00 2001 From: Denis Vlasenko Date: Tue, 17 Jun 2008 05:11:43 +0000 Subject: [PATCH] hush: fix memory leak. it was actually rather invloved problem. Now finally glob/variable expansion is done IN THE RIGHT ORDER! It opens up a possibility to cleanly fix remaining known bugs. function old new delta o_save_ptr 115 286 +171 o_save_ptr_helper - 115 +115 done_word 591 690 +99 o_get_last_ptr - 31 +31 expand_on_ifs 125 97 -28 add_string_to_strings 28 - -28 run_list 1895 1862 -33 debug_print_strings 42 - -42 add_strings_to_strings 126 - -126 expand_variables 1550 1394 -156 o_debug_list 168 - -168 expand_strvec_to_strvec 388 10 -378 ------------------------------------------------------------------------------ (add/remove: 2/4 grow/shrink: 2/4 up/down: 416/-959) Total: -543 bytes --- shell/hush.c | 313 ++++++++++++++++++++++++++++----------------------- 1 file changed, 175 insertions(+), 138 deletions(-) diff --git a/shell/hush.c b/shell/hush.c index 0e1976443..0b92c29d4 100644 --- a/shell/hush.c +++ b/shell/hush.c @@ -104,6 +104,8 @@ #define debug_printf_exec(...) do {} while (0) #define debug_printf_jobs(...) do {} while (0) #define debug_printf_expand(...) do {} while (0) +#define debug_printf_glob(...) do {} while (0) +#define debug_printf_list(...) do {} while (0) #define debug_printf_clean(...) do {} while (0) #ifndef debug_printf @@ -120,12 +122,27 @@ #ifndef debug_printf_jobs #define debug_printf_jobs(...) fprintf(stderr, __VA_ARGS__) -#define DEBUG_SHELL_JOBS 1 +#define DEBUG_JOBS 1 +#else +#define DEBUG_JOBS 0 #endif #ifndef debug_printf_expand #define debug_printf_expand(...) fprintf(stderr, __VA_ARGS__) #define DEBUG_EXPAND 1 +#else +#define DEBUG_EXPAND 0 +#endif + +#ifndef debug_printf_glob +#define debug_printf_glob(...) fprintf(stderr, __VA_ARGS__) +#define DEBUG_GLOB 1 +#else +#define DEBUG_GLOB 0 +#endif + +#ifndef debug_printf_list +#define debug_printf_list(...) fprintf(stderr, __VA_ARGS__) #endif /* Keep unconditionally on for now */ @@ -143,6 +160,17 @@ static const char *indenter(int i) #define DEBUG_CLEAN 1 #endif +#if DEBUG_EXPAND +static void debug_print_strings(const char *prefix, char **vv) +{ + fprintf(stderr, "%s:\n", prefix); + while (*vv) + fprintf(stderr, " '%s'\n", *vv++); +} +#else +#define debug_print_strings(prefix, vv) ((void)0) +#endif + /* * Leak hunting. Use hush_leaktool.sh for post-processing. @@ -309,6 +337,7 @@ typedef struct { int length; int maxlen; smallint o_quote; + smallint o_glob; smallint nonnull; smallint has_empty_slot; } o_string; @@ -531,6 +560,18 @@ static int set_local_var(char *str, int flg_export); static void unset_local_var(const char *name); +static int glob_needed(const char *s) +{ + while (*s) { + if (*s == '\\') + s++; + if (*s == '*' || *s == '[' || *s == '?') + return 1; + s++; + } + return 0; +} + static char **add_strings_to_strings(int need_xstrdup, char **strings, char **add) { int i; @@ -592,17 +633,6 @@ static char **alloc_ptrs(char **argv) } #endif -#ifdef DEBUG_EXPAND -static void debug_print_strings(const char *prefix, char **vv) -{ - fprintf(stderr, "%s:\n", prefix); - while (*vv) - fprintf(stderr, " '%s'\n", *vv++); -} -#else -#define debug_print_strings(prefix, vv) ((void)0) -#endif - /* Function prototypes for builtins */ static int builtin_cd(char **argv); @@ -1260,7 +1290,33 @@ static void o_addQstr(o_string *o, const char *str, int len) * o_finalize_list() operation post-processes this structure - calculates * and stores actual char* ptrs in list[]. Oh, it NULL terminates it as well. */ -static int o_save_ptr(o_string *o, int n) +#if DEBUG_EXPAND || DEBUG_GLOB +static void debug_print_list(const char *prefix, o_string *o, int n) +{ + char **list = (char**)o->data; + int string_start = ((n + 0xf) & ~0xf) * sizeof(list[0]); + int i = 0; + fprintf(stderr, "%s: list:%p n:%d string_start:%d length:%d maxlen:%d\n", + prefix, list, n, string_start, o->length, o->maxlen); + while (i < n) { + fprintf(stderr, " list[%d]=%d '%s' %p\n", i, (int)list[i], + o->data + (int)list[i] + string_start, + o->data + (int)list[i] + string_start); + i++; + } + if (n) { + const char *p = o->data + (int)list[n - 1] + string_start; + fprintf(stderr, " total_sz:%d\n", (p + strlen(p) + 1) - o->data); + } +} +#else +#define debug_print_list(prefix, o, n) ((void)0) +#endif + +/* n = o_save_ptr_helper(str, n) "starts new string" by storing an index value + * in list[n] so that it points past last stored byte so far. + * It returns n+1. */ +static int o_save_ptr_helper(o_string *o, int n) { char **list = (char**)o->data; int string_start; @@ -1270,26 +1326,27 @@ static int o_save_ptr(o_string *o, int n) string_start = ((n + 0xf) & ~0xf) * sizeof(list[0]); string_len = o->length - string_start; if (!(n & 0xf)) { /* 0, 0x10, 0x20...? */ - //bb_error_msg("list[%d]=%d string_start=%d (growing)", n, string_len, string_start); + debug_printf_list("list[%d]=%d string_start=%d (growing)", n, string_len, string_start); /* list[n] points to string_start, make space for 16 more pointers */ o->maxlen += 0x10 * sizeof(list[0]); o->data = xrealloc(o->data, o->maxlen + 1); list = (char**)o->data; memmove(list + n + 0x10, list + n, string_len); o->length += 0x10 * sizeof(list[0]); - } - //else bb_error_msg("list[%d]=%d string_start=%d", n, string_len, string_start); + } else + debug_printf_list("list[%d]=%d string_start=%d", n, string_len, string_start); } else { /* We have empty slot at list[n], reuse without growth */ string_start = ((n+1 + 0xf) & ~0xf) * sizeof(list[0]); /* NB: n+1! */ string_len = o->length - string_start; - //bb_error_msg("list[%d]=%d string_start=%d (empty slot)", n, string_len, string_start); + debug_printf_list("list[%d]=%d string_start=%d (empty slot)", n, string_len, string_start); o->has_empty_slot = 0; } list[n] = (char*)string_len; return n + 1; } +/* "What was our last o_save_ptr'ed position (byte offset relative o->data)?" */ static int o_get_last_ptr(o_string *o, int n) { char **list = (char**)o->data; @@ -1298,14 +1355,89 @@ static int o_get_last_ptr(o_string *o, int n) return ((int)list[n-1]) + string_start; } +/* Convert every \x to x in-place, return ptr past NUL. */ +static char *unbackslash(char *src) +{ + char *dst = src; + while (1) { + if (*src == '\\') + src++; + if ((*dst++ = *src++) == '\0') + break; + } + return dst; +} + +static int o_glob(o_string *o, int n) +{ + glob_t globdata; + int gr; + char *pattern; + + debug_printf_glob("start o_glob: n:%d o->data:%p", n, o->data); + if (!o->data) + return o_save_ptr_helper(o, n); + pattern = o->data + o_get_last_ptr(o, n); + debug_printf_glob("glob pattern '%s'", pattern); + if (!glob_needed(pattern)) { + literal: + o->length = unbackslash(pattern) - o->data; + debug_printf_glob("glob pattern '%s' is literal", pattern); + return o_save_ptr_helper(o, n); + } + + memset(&globdata, 0, sizeof(globdata)); + gr = glob(pattern, 0, NULL, &globdata); + debug_printf_glob("glob('%s'):%d\n", pattern, gr); + if (gr == GLOB_NOSPACE) + bb_error_msg_and_die("out of memory during glob"); + if (gr == GLOB_NOMATCH) { + globfree(&globdata); + goto literal; + } + if (gr != 0) { /* GLOB_ABORTED ? */ +//TODO: testcase for bad glob pattern behavior + bb_error_msg("glob(3) error %d on '%s'", gr, pattern); + } + if (globdata.gl_pathv && globdata.gl_pathv[0]) { + char **argv = globdata.gl_pathv; + o->length = pattern - o->data; /* "forget" pattern */ + while (1) { + o_addstr(o, *argv, strlen(*argv) + 1); + n = o_save_ptr_helper(o, n); + argv++; + if (!*argv) + break; + } + } + globfree(&globdata); + if (DEBUG_GLOB) + debug_print_list("o_glob returning", o, n); + return n; +} + +/* o_save_ptr_helper + but glob the string so far remembered + * if o->o_glob == 1 */ +static int o_save_ptr(o_string *o, int n) +{ + if (o->o_glob) + return o_glob(o, n); /* o_save_ptr_helper is inside */ + return o_save_ptr_helper(o, n); +} + +/* "Please convert list[n] to real char* ptrs, and NULL terminate it." */ static char **o_finalize_list(o_string *o, int n) { - char **list = (char**)o->data; + char **list; int string_start; - o_save_ptr(o, n); /* force growth for list[n] if necessary */ - string_start = ((n+1 + 0xf) & ~0xf) * sizeof(list[0]); - list[n] = NULL; + n = o_save_ptr(o, n); /* force growth for list[n] if necessary */ + if (DEBUG_EXPAND) + debug_print_list("finalized", o, n); + debug_printf_expand("finalized n:%d", n); + list = (char**)o->data; + string_start = ((n + 0xf) & ~0xf) * sizeof(list[0]); + list[--n] = NULL; while (n) { n--; list[n] = o->data + (int)list[n] + string_start; @@ -1313,28 +1445,6 @@ static char **o_finalize_list(o_string *o, int n) return list; } -#ifdef DEBUG_EXPAND -static void o_debug_list(const char *prefix, o_string *o, int n) -{ - char **list = (char**)o->data; - int string_start = ((n + 0xf) & ~0xf) * sizeof(list[0]); - int i = 0; - fprintf(stderr, "%s: list:%p n:%d string_start:%d length:%d maxlen:%d\n", - prefix, list, n, string_start, o->length, o->maxlen); - while (i < n) { - fprintf(stderr, " list[%d]=%d '%s'\n", i, (int)list[i], - o->data + (int)list[i] + string_start); - i++; - } - if (n) { - const char *p = o->data + (int)list[n] + string_start; - fprintf(stderr, " total_sz:%d\n", (p + strlen(p) + 1) - o->data); - } -} -#else -#define o_debug_list(prefix, o, n) ((void)0) -#endif - /* * in_str support @@ -1782,7 +1892,7 @@ static int checkjobs(struct pipe* fg_pipe) while ((childpid = waitpid(-1, &status, attributes)) > 0) { const int dead = WIFEXITED(status) || WIFSIGNALED(status); -#ifdef DEBUG_SHELL_JOBS +#if DEBUG_JOBS if (WIFSTOPPED(status)) debug_printf_jobs("pid %d stopped by sig %d (exitcode %d)\n", childpid, WSTOPSIG(status), WEXITSTATUS(status)); @@ -2502,11 +2612,11 @@ static int expand_on_ifs(o_string *output, int n, const char *str) if (!*str) /* EOL - do not finalize word */ break; o_addchr(output, '\0'); - o_debug_list("expand_on_ifs", output, n); + debug_print_list("expand_on_ifs", output, n); n = o_save_ptr(output, n); str += strspn(str, ifs); /* skip ifs chars */ } - o_debug_list("expand_on_ifs[1]", output, n); + debug_print_list("expand_on_ifs[1]", output, n); return n; } @@ -2530,9 +2640,9 @@ static int expand_vars_to_list(o_string *output, int n, char *arg, char or_mask) ored_ch = 0; debug_printf_expand("expand_vars_to_list: arg '%s'\n", arg); - o_debug_list("expand_vars_to_list", output, n); + debug_print_list("expand_vars_to_list", output, n); n = o_save_ptr(output, n); - o_debug_list("expand_vars_to_list[0]", output, n); + debug_print_list("expand_vars_to_list[0]", output, n); while ((p = strchr(arg, SPECIAL_VAR_SYMBOL)) != NULL) { #if ENABLE_HUSH_TICK @@ -2540,7 +2650,7 @@ static int expand_vars_to_list(o_string *output, int n, char *arg, char or_mask) #endif o_addQstr(output, arg, p - arg); - o_debug_list("expand_vars_to_list[1]", output, n); + debug_print_list("expand_vars_to_list[1]", output, n); arg = ++p; p = strchr(p, SPECIAL_VAR_SYMBOL); @@ -2575,9 +2685,9 @@ static int expand_vars_to_list(o_string *output, int n, char *arg, char or_mask) /* this argv[] is not empty and not last: * put terminating NUL, start new word */ o_addchr(output, '\0'); - o_debug_list("expand_vars_to_list[2]", output, n); + debug_print_list("expand_vars_to_list[2]", output, n); n = o_save_ptr(output, n); - o_debug_list("expand_vars_to_list[3]", output, n); + debug_print_list("expand_vars_to_list[3]", output, n); } } } else @@ -2589,7 +2699,7 @@ static int expand_vars_to_list(o_string *output, int n, char *arg, char or_mask) if (++i >= global_argc) break; o_addchr(output, '\0'); - o_debug_list("expand_vars_to_list[4]", output, n); + debug_print_list("expand_vars_to_list[4]", output, n); n = o_save_ptr(output, n); } } else { /* quoted $*: add as one word */ @@ -2647,9 +2757,9 @@ static int expand_vars_to_list(o_string *output, int n, char *arg, char or_mask) } /* end of "while (SPECIAL_VAR_SYMBOL is found) ..." */ if (arg[0]) { - o_debug_list("expand_vars_to_list[a]", output, n); + debug_print_list("expand_vars_to_list[a]", output, n); o_addQstr(output, arg, strlen(arg) + 1); - o_debug_list("expand_vars_to_list[b]", output, n); + debug_print_list("expand_vars_to_list[b]", output, n); } else if (output->length == o_get_last_ptr(output, n) /* expansion is empty */ && !(ored_ch & 0x80) /* and all vars were not quoted. */ ) { @@ -2662,106 +2772,33 @@ static int expand_vars_to_list(o_string *output, int n, char *arg, char or_mask) return n; } -static char **expand_variables(char **argv, char or_mask) +static char **expand_variables(char **argv, int or_mask) { int n; char **list; char **v; o_string output = NULL_O_STRING; + if (or_mask & 0x100) + output.o_glob = 1; + n = 0; v = argv; - while (*v) - n = expand_vars_to_list(&output, n, *v++, or_mask); - o_debug_list("expand_variables", &output, n); + while (*v) { + n = expand_vars_to_list(&output, n, *v, (char)or_mask); + v++; + } + debug_print_list("expand_variables", &output, n); /* output.data (malloced in one block) gets returned in "list" */ list = o_finalize_list(&output, n); + debug_print_strings("expand_variables[1]", list); return list; } -/* Remove non-backslashed backslashes and add to "strings" vector. - * XXX broken if the last character is '\\', check that before calling. - */ -static char **add_unq_string_to_strings(char **strings, const char *src) -{ - int cnt; - const char *s; - char *v, *dest; - - for (cnt = 1, s = src; s && *s; s++) { - if (*s == '\\') s++; - cnt++; - } - v = dest = xmalloc(cnt); - for (s = src; s && *s; s++, dest++) { - if (*s == '\\') s++; - *dest = *s; - } - *dest = '\0'; - - return add_string_to_strings(strings, v); -} - -/* XXX broken if the last character is '\\', check that before calling */ -static int glob_needed(const char *s) -{ - for (; *s; s++) { - if (*s == '\\') - s++; - if (strchr("*[?", *s)) - return 1; - } - return 0; -} - -static void xglob(char ***pglob, const char *pattern) -{ - if (glob_needed(pattern)) { - glob_t globdata; - int gr; - - memset(&globdata, 0, sizeof(globdata)); - gr = glob(pattern, 0, NULL, &globdata); - debug_printf("glob returned %d\n", gr); - if (gr == GLOB_NOSPACE) - bb_error_msg_and_die("out of memory during glob"); - if (gr == GLOB_NOMATCH) { - globfree(&globdata); - goto literal; - } - if (gr != 0) { /* GLOB_ABORTED ? */ - bb_error_msg("glob(3) error %d on '%s'", gr, pattern); -//TODO: testcase for bad glob pattern behavior - } - if (globdata.gl_pathv && globdata.gl_pathv[0]) - *pglob = add_strings_to_strings(1, *pglob, globdata.gl_pathv); - globfree(&globdata); - return; - } - - literal: - /* quote removal, or more accurately, backslash removal */ - *pglob = add_unq_string_to_strings(*pglob, pattern); - debug_print_strings("after xglob", *pglob); -} - -//LEAK is here: callers expect result to be free()able, but we -//actually require free_strings(). free() leaks strings. static char **expand_strvec_to_strvec(char **argv) { - char **exp; - char **res = xzalloc(sizeof(res[0])); - - debug_print_strings("expand_strvec_to_strvec: pre expand", argv); - exp = argv = expand_variables(argv, 0); - debug_print_strings("expand_strvec_to_strvec: post expand", argv); - while (*argv) { - xglob(&res, *argv++); - } - free(exp); - debug_print_strings("expand_strvec_to_strvec: res", res); - return res; + return expand_variables(argv, 0x100); } /* used for expansion of right hand of assignments */ -- 2.25.1