The patches are whitespace-damaged. Please resend as attachments.
On Fri, Aug 1, 2025 at 9:59 PM Harry Eaton <[email protected]> wrote: > > I have solved the leak problem. This patch (to busybox-1.37.0) solves the > leaks when compiled with NOMMU > (change the #define BUILD_AS_NOMMU line from 0 to 1 in order to demonstate > that the unpatch code leaks, > while the patched code does not on a system that actually has an MMU since > most developers don't have > an available system that actually lacks an MMU. This code in unpatched hush > with NO_MMU will cause the > leak that can be seen with top: > > while true; do v=$(echo 0); done > > Below is the patch that only fixes the leak. > ------------------------------------------------ diff > ----------------------------------------------------- > -- hush.c.orig 2025-07-28 17:57:34.808579711 -0400 > +++ hush.c 2025-08-01 15:24:14.885718809 -0400 > @@ -961,6 +961,9 @@ > #if ENABLE_HUSH_GETOPTS > unsigned getopt_count; > #endif > +#if ENABLE_HUSH_TICK > + char **to_free; > +#endif > const char *ifs; > char *ifs_whitespace; /* = G.ifs or malloced */ > const char *cwd; > @@ -2128,6 +2131,9 @@ > cur_var = cur_var->next; > free(tmp); > } > + free(G.to_free); > + G.to_free = NULL; > + free(ptr_to_globals); > } > #endif > > @@ -7657,9 +7663,6 @@ > { > pid_t pid; > int channel[2]; > -# if !BB_MMU > - char **to_free = NULL; > -# endif > > xpipe(channel); > pid = BB_MMU ? xfork() : xvfork(); > @@ -7736,7 +7739,7 @@ > * huge=`cat BIG` # was blocking here forever > * echo OK > */ > - re_execute_shell(&to_free, > + re_execute_shell(&G.to_free, > s, > G.global_argv[0], > G.global_argv + 1, > @@ -7754,7 +7757,8 @@ > # endif > enable_restore_tty_pgrp_on_exit(); > # if !BB_MMU > - free(to_free); > + free(G.to_free); > + G.to_free = NULL; > > -------------------------------------------------------------------------------------------------- > > In order to discover these bugs, I had to greatly improve the LEAK_HUNTING > code in hush.c > as well as make a different, better hush_leaktool.sh. > > Below is the patch that includes the improvements to LEAK_HUNTING as well as > the bug fix. > > ------------------------------------------ diff > -------------------------------------------------------- > --- hush.c.orig 2025-08-01 13:31:24.541695883 -0400 > +++ hush.c 2025-08-01 13:37:44.957697171 -0400 > @@ -961,6 +961,9 @@ > #if ENABLE_HUSH_GETOPTS > unsigned getopt_count; > #endif > +#if ENABLE_HUSH_TICK > + char **to_free; > +#endif > const char *ifs; > char *ifs_whitespace; /* = G.ifs or malloced */ > const char *cwd; > @@ -1343,36 +1346,157 @@ > #endif > > > +static char **add_strings_to_strings(char **strings, char **add, int > need_to_dup) > +{ > + int i; > + unsigned count1; > + unsigned count2; > + char **v; > + > + v = strings; > + count1 = 0; > + if (v) { > + while (*v) { > + count1++; > + v++; > + } > + } > + count2 = 0; > + v = add; > + while (*v) { > + count2++; > + v++; > + } > + v = xrealloc(strings, (count1 + count2 + 1) * sizeof(char*)); > + v[count1 + count2] = NULL; > + i = count2; > + while (--i >= 0) > + v[count1 + i] = (need_to_dup ? xstrdup(add[i]) : add[i]); > + return v; > +} > + > +/* Note: takes ownership of "add" ptr (it is not strdup'ed) */ > +static char **add_string_to_strings(char **strings, char *add) > +{ > + char *v[2]; > + v[0] = add; > + v[1] = NULL; > + return add_strings_to_strings(strings, v, /*dup:*/ 0); > +} > + > /* Leak hunting. Use hush_leaktool.sh for post-processing. > */ > + > #if LEAK_HUNTING > -static void *xxmalloc(int lineno, size_t size) > +static char **xxadd_strings_to_strings(int lineno, char **strings, char > **add, int need_to_dup) > +{ > + char **orig = strings; > + char **ptr = add_strings_to_strings(strings, add, need_to_dup); > + if (orig != ptr) { > + fdprintf(2, "%p line %d: add_strings_to_strings\n", ptr, lineno); > + if (orig != NULL) fdprintf(2, "free %p by add_strings_to_strings\n", orig); > + fflush_all(); > + } > + return ptr; > +} > + > +static char **xxadd_string_to_strings(int lineno, char **strings, char *add) > +{ > + char **orig = strings; > + char **ptr = add_string_to_strings(strings, add); > + if (ptr != orig) { > + fdprintf(2, "%p line %d: add_string_to_strings\n", ptr, lineno); > + if (orig != NULL) fdprintf(2, "free %p by add_string_to_string\n", orig);; > + fflush_all(); > + } > + return ptr; > +} > + > +static void *xxmalloc(int lineno, const char *s, size_t size) > { > void *ptr = xmalloc((size + 0xff) & ~0xff); > - fdprintf(2, "line %d: malloc %p\n", lineno, ptr); > + fdprintf(2, "%p line %d: malloc(%s)\n", ptr, lineno, s); > + fflush_all(); > + return ptr; > +} > +static void *xxzalloc(int lineno, const char *s, size_t size) > +{ > + void *ptr = xzalloc((size + 0xff) & ~0xff); > + fdprintf(2, "%p line %d: zalloc(%s)\n", ptr, lineno, s); > + fflush_all(); > return ptr; > } > -static void *xxrealloc(int lineno, void *ptr, size_t size) > +static void *xxrealloc(int lineno, const char *s, void *ptr, size_t size) > { > + void *p = ptr; > ptr = xrealloc(ptr, (size + 0xff) & ~0xff); > - fdprintf(2, "line %d: realloc %p\n", lineno, ptr); > + if (p != ptr) { > + fdprintf(2, "%p line %d: realloc(%s)\n", ptr, lineno, s); > + if (p != NULL) fdprintf(2, "free %p by realloc\n", p); > + fflush_all(); > + } > + return ptr; > +} > +static void *xxrealloc_getcwd_or_warn(int lineno, char *ptr) > +{ > + char *p = ptr; > + ptr = xrealloc_getcwd_or_warn(ptr); > + if (p != ptr) { > + fdprintf(2, "%p line %d: xrealloc_getcwd_or_warn\n", ptr, lineno); > + if (p != NULL) fdprintf(2, "free %p by getcwd_or_warn\n", p); > + fflush_all(); > + } > return ptr; > } > -static char *xxstrdup(int lineno, const char *str) > +static char *xxstrdup(int lineno, const char *s, const char *str) > { > char *ptr = xstrdup(str); > - fdprintf(2, "line %d: strdup %p\n", lineno, ptr); > + fdprintf(2, "%p line %d: strdup(%s) %s\n", ptr, lineno, s, ptr); > + fflush_all(); > + return ptr; > +} > +static char *xxstrndup(int lineno, const char *s, const char *str, int n) > +{ > + char *ptr = xstrndup(str, n); > + fdprintf(2, "%p line %d: strndup(%s) %s\n", ptr, lineno, s, ptr); > + fflush_all(); > return ptr; > } > -static void xxfree(void *ptr) > +static char *xxasprintf(int lineno, const char *f, ...) > { > - fdprintf(2, "free %p\n", ptr); > + int r; > + char *p; > + va_list args; > + va_start(args, f); > + r = vasprintf(&p, f, args); > + va_end(args); > + if (r < 0) > + bb_die_memory_exhausted(); > + fdprintf(2, "%p line %d: xasprintf %s\n", p, lineno, p); > + fflush_all(); > + return p; > +} > +static void xxfree(int lineno, const char *s, void *ptr) > +{ > + if (ptr) { > + fdprintf(2, "free %p line %d (%s)\n", ptr, lineno, s); > + fflush_all(); > free(ptr); > + } else fdprintf(2, "free null line %d (%s)\n", lineno, s); > } > -# define xmalloc(s) xxmalloc(__LINE__, s) > -# define xrealloc(p, s) xxrealloc(__LINE__, p, s) > -# define xstrdup(s) xxstrdup(__LINE__, s) > -# define free(p) xxfree(p) > +#define STRINGIFY(x) #x > +#define add_strings_to_strings(strings, add, need_to_dup) \ > + xxadd_strings_to_strings(__LINE__, strings, add, need_to_dup) > +#define add_string_to_strings(strings, add) \ > + xxadd_string_to_strings(__LINE__, strings, add) > +# define xmalloc(s) xxmalloc(__LINE__, STRINGIFY(s), s) > +# define xzalloc(s) xxzalloc(__LINE__, STRINGIFY(s), s) > +# define xrealloc(p, s) xxrealloc(__LINE__, STRINGIFY(p), p, s ) > +# define xstrdup(s) xxstrdup(__LINE__, STRINGIFY(s), s) > +# define xstrndup(s, n) xxstrndup(__LINE__, STRINGIFY(s), s, n) > +# define xrealloc_getcwd_or_warn(p) xxrealloc_getcwd_or_warn(__LINE__, p) > +# define xasprintf(f, ...) xxasprintf(__LINE__, f, __VA_ARGS__) > +# define free(p) xxfree(__LINE__, STRINGIFY(p), p) > #endif > > > @@ -1496,64 +1620,6 @@ > return dst; > } > > -static char **add_strings_to_strings(char **strings, char **add, int > need_to_dup) > -{ > - int i; > - unsigned count1; > - unsigned count2; > - char **v; > - > - v = strings; > - count1 = 0; > - if (v) { > - while (*v) { > - count1++; > - v++; > - } > - } > - count2 = 0; > - v = add; > - while (*v) { > - count2++; > - v++; > - } > - v = xrealloc(strings, (count1 + count2 + 1) * sizeof(char*)); > - v[count1 + count2] = NULL; > - i = count2; > - while (--i >= 0) > - v[count1 + i] = (need_to_dup ? xstrdup(add[i]) : add[i]); > - return v; > -} > -#if LEAK_HUNTING > -static char **xx_add_strings_to_strings(int lineno, char **strings, char > **add, int need_to_dup) > -{ > - char **ptr = add_strings_to_strings(strings, add, need_to_dup); > - fdprintf(2, "line %d: add_strings_to_strings %p\n", lineno, ptr); > - return ptr; > -} > -#define add_strings_to_strings(strings, add, need_to_dup) \ > - xx_add_strings_to_strings(__LINE__, strings, add, need_to_dup) > -#endif > - > -/* Note: takes ownership of "add" ptr (it is not strdup'ed) */ > -static char **add_string_to_strings(char **strings, char *add) > -{ > - char *v[2]; > - v[0] = add; > - v[1] = NULL; > - return add_strings_to_strings(strings, v, /*dup:*/ 0); > -} > -#if LEAK_HUNTING > -static char **xx_add_string_to_strings(int lineno, char **strings, char *add) > -{ > - char **ptr = add_string_to_strings(strings, add); > - fdprintf(2, "line %d: add_string_to_strings %p\n", lineno, ptr); > - return ptr; > -} > -#define add_string_to_strings(strings, add) \ > - xx_add_string_to_strings(__LINE__, strings, add) > -#endif > - > static void free_strings(char **strings) > { > char **v; > @@ -2128,6 +2194,10 @@ > cur_var = cur_var->next; > free(tmp); > } > + free(G.to_free); > + G.to_free = 0; > + free(ptr_to_globals); > } > #endif > > @@ -7658,7 +7728,7 @@ > pid_t pid; > int channel[2]; > # if !BB_MMU > - char **to_free = NULL; > + G.to_free = NULL; > # endif > > xpipe(channel); > @@ -7736,7 +7806,7 @@ > * huge=`cat BIG` # was blocking here forever > * echo OK > */ > - re_execute_shell(&to_free, > + re_execute_shell(&G.to_free, > s, > G.global_argv[0], > G.global_argv + 1, > @@ -7754,7 +7824,8 @@ > # endif > enable_restore_tty_pgrp_on_exit(); > # if !BB_MMU > - free(to_free); > + free(G.to_free); > + G.to_free = NULL; > # endif > close(channel[1]); > return channel[0]; > ____________________________________________________________________ > > Below is the new hush_leaktool.sh > > ----------------------------------------------- diff > -------------------------------------------------------------- > #!/bin/bash > # hush_leaktool.sh > # works with GNU sed, maybe not busybox's sed. > > # hush's stderr with leak debug enabled > output=output > > grep -v '^free' "$output" > "$output.leaked" > grep '^free 0x' "$output" > "$output.spurious_free" > grep '^0x' "$output" > "$output.alloc" > alloclist=`cut -d' ' -f1 "$output.alloc" | xargs` > > for mem in $alloclist; do > if grep -q $mem "$output.spurious_free"; then > # echo "$mem freed" > # drop first corresponding free > sed -i '0,/'$mem'/{/'$mem'/d}' "$output.spurious_free" > # drop from the leaked list > sed -i '0,/'$mem'/{/'$mem'/d}' "$output.leaked" > fi > done > cat "$output.leaked" > _______________________________________________________________________ > > Finally, mea culpa in my first message: the leak had nothing to do with > looping. I didn't > realize that hush didn't treat {1..10} like bash does. Also my leak hunting > improvements > still had bugs that led to false positives (but that's fixed now), so my > clues were wrong. > I wasn't wrong about the original leak_hunting code being able to hide all > kinds of leaks. > > harry > > _______________________________________________ > busybox mailing list > [email protected] > https://lists.busybox.net/mailman/listinfo/busybox _______________________________________________ busybox mailing list [email protected] https://lists.busybox.net/mailman/listinfo/busybox
