sh memory leak with fix MR

[GangyiTian]

Dear author:

Hi, I'm coming here again! This time I tried to provide some MR to fix sh memory leak, but I'm not sure that will cause another issue. So I just raised those questions. If you have time, could you please take a look?

My test script

my_pid=$$

while true; do
    top -p $my_pid -t 1 | grep "bash" >> test.log

    sleep 1 
done

1. arg_add xrealloc

==2533643== 264 bytes in 1 blocks are definitely lost in loss record 26 of 27
==2533643==    at 0x48487A9: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2533643==    by 0x121F6C: xrealloc (xwrap.c:89)
==2533643==    by 0x13E1EE: arg_add (sh.c:439)
==2533643==    by 0x14149F: wildcard_add_files (sh.c:1735)
==2533643==    by 0x14537B: expand_arg_nobrace (sh.c:2281)
==2533643==    by 0x145A47: expand_arg (sh.c:2388)
==2533643==    by 0x1462A6: expand_redir (sh.c:2557)
==2533643==    by 0x146A46: run_command (sh.c:2793)
==2533643==    by 0x142F27: run_lines (sh.c:3742)
==2533643==    by 0x142F27: do_source (sh.c:4123)
==2533643==    by 0x1476A0: sh_main (sh.c:4326)
==2533643==    by 0x1241BB: toy_exec_which (main.c:232)
==2533643==    by 0x124269: toybox_main (main.c:256)

If we look at arg_add

static void arg_add(struct sh_arg *arg, char *data)
{
  // expand with stride 32. Micro-optimization: don't realloc empty stack
  if (!(arg->c&31) && (arg->c || !arg->v))
    arg->v = xrealloc(arg->v, sizeof(char *)*(arg->c+33));
  arg->v[arg->c++] = data;
  arg->v[arg->c] = 0;
}

We can find that xrealloc() is used to allocate a memory area to arg->v. arg->v is an array of pointers used to manage saved parameter strings. When the command is executed, the string will be released, but arg->v is not released. We can find it in free_process()

static int free_process(struct sh_process *pp)
{
  int rc;

  if (!pp) return 127;
  rc = pp->exit;
  llist_traverse(pp->delete, llist_free_arg);
  free(pp);

  return rc;
}

Therefore, I added free code in there, and the memory leak here disappeared (valgrind passed).

static int free_process(struct sh_process *pp)
{
  int rc;

  if (!pp) return 127;
  rc = pp->exit;
  llist_traverse(pp->delete, llist_free_arg);
  if (pp->arg.v) free(pp->arg.v); //Here
  free(pp);

  return rc;
}

2. setvar xmprintf

==2995276== 63 bytes in 12 blocks are definitely lost in loss record 24 of 45
==2995276==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2995276==    by 0x121F39: xmalloc (xwrap.c:71)
==2995276==    by 0x1220E7: xmprintf (xwrap.c:138)
==2995276==    by 0x1402FD: setvarval (sh.c:1016)
==2995276==    by 0x147026: run_command (sh.c:2939)
==2995276==    by 0x142F3C: run_lines (sh.c:3765)
==2995276==    by 0x142F3C: do_source (sh.c:4140)
==2995276==    by 0x14768F: sh_main (sh.c:4342)
==2995276==    by 0x1241C7: toy_exec_which (main.c:232)
==2995276==    by 0x124275: toybox_main (main.c:256)
==2995276==    by 0x11A823: main (main.c:303)

When executing the command, setvar() will be called to save some environment variables. However, in the while loop, xmprintf() will be called each time to allocate memory to store these variables. It looks like these duplicate variables are not being released.

Take a look at setvar(). This function calls setvar_long(), which comes with a freeable flag to release the input string. So I turn on this flag. Then the memory leak disappears (valgrind passed)

static struct sh_vars *setvar_long(char *s, int freeable, struct sh_fcall *ff);

//Brfore
static struct sh_vars *setvar(char *str)
{
  return setvar_long(str, 0, TT.ff->prev);
}

//After
static struct sh_vars *setvar(char *str)
{
  return setvar_long(str, 1, TT.ff->prev);
}

3. Fix result

Under these two fixes, I conducted data tests and the effect was very obvious

• System: Linux 20.04.1-Ubuntu
• Toybox Version: 0.8.9

• [fix before][blue line] PID 3002595
• [fix after][orange line] PID 2997147

Could you please help me see if this fix is feasible?