[PATCH v2] ash: ensure variables are fully initialised when unset

[Denys Vlasenko]

Applied, thanks!
On Mon, Nov 12, 2018 at 10:11 PM Ron Yorston <rmy at pobox.com> wrote:
>
> When a variable is unset by calling setvar(name, NULL, 0) the code
> to initialise the new, empty variable fails to initialise the last
> character of the string.
>
> Attempts to read the contents of the unset variable will result
> in the uninitialised character at the end of the string being
> accessed.
>
> For example, running BusyBox under Valgrind and unsetting PATH:
>
> $ valgrind ./busybox_unstripped sh
> ==21249== Memcheck, a memory error detector
> ==21249== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==21249== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
> ==21249== Command: ./busybox_unstripped sh
> ==21249==
> /data2/git/build_fix_8721 $ unset PATH
> /data2/git/build_fix_8721 $ 0
> ==21249== Conditional jump or move depends on uninitialised value(s)
> ==21249==    at 0x451371: path_advance (ash.c:2555)
> ==21249==    by 0x456E22: find_command (ash.c:13407)
> ==21249==    by 0x458425: evalcommand (ash.c:10139)
> ==21249==    by 0x454CBC: evaltree (ash.c:9131)
> ==21249==    by 0x456C80: cmdloop (ash.c:13164)
>
> Closes https://bugs.busybox.net/show_bug.cgi?id=8721
>
> v2: On the dash mailing list Harald van Dijk was kind enough to point
>     out a flaw in my reasoning and provide an alternative patch.  Sadly
>     his patch adds 2 bytes of bloat.  Using xzalloc to zero the whole
>     string gives a bloat of -3 bytes.
>
> Signed-off-by: Ron Yorston <rmy at pobox.com>
> ---
>  shell/ash.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/shell/ash.c b/shell/ash.c
> index 90eaf6faf..b1f8f15d2 100644
> --- a/shell/ash.c
> +++ b/shell/ash.c
> @@ -2420,13 +2420,12 @@ setvar(const char *name, const char *val, int flags)
>         }
>
>         INT_OFF;
> -       nameeq = ckmalloc(namelen + vallen + 2);
> +       nameeq = ckzalloc(namelen + vallen + 2);
>         p = mempcpy(nameeq, name, namelen);
>         if (val) {
>                 *p++ = '=';
> -               p = mempcpy(p, val, vallen);
> +               memcpy(p, val, vallen);
>         }
> -       *p = '\0';
>         vp = setvareq(nameeq, flags | VNOSAVE);
>         INT_ON;
>
> --
> 2.19.1
>
> _______________________________________________
> busybox mailing list
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox