var: ensure variables are fully initialised when unset

When a variable is unset by calling setvar(name, 0, 0) the code
to initialise the new, empty variable omits the trailing '='.

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 dash under Valgrind and unsetting PATH:

  $ valgrind ./src/dash
  ==9117== Memcheck, a memory error detector
  ==9117== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
  ==9117== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
  ==9117== Command: ./src/dash
  ==9117==
  $ unset PATH
  ==9117== Conditional jump or move depends on uninitialised value(s)
  ==9117==    at 0x40642C: changepath (exec.c:578)
  ==9117==    by 0x411EEB: setvareq (var.c:269)
  ==9117==    by 0x41201B: setvar (var.c:215)
  ==9117==    by 0x4128D4: unsetvar (var.c:628)

This issue was reported for BusyBox ash:

   https://bugs.busybox.net/show_bug.cgi?id=8721

Signed-off-by: Ron Yorston <rmy@pobox.com>
---
 src/var.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 12/11/2018 12:53, Ron Yorston wrote:
> When a variable is unset by calling setvar(name, 0, 0) the code
> to initialise the new, empty variable omits the trailing '='.

It's supposed to. A trailing = means the variable is set to an empty 
string. That's different from unset. You can see the difference with
set -u, or with ${var+set}. However, ...

> Attempts to read the contents of the unset variable will result
> in the uninitialised character at the end of the string being
> accessed.

...this is indeed a bug which I've noticed as well. The code needs two 
trailing null bytes, not just one. Because of glibc internals, the 
out-of-bounds byte being read will almost certainly be zero on x86-64, 
but it's not a guarantee, and it could probably break more easily on 
other platforms.

It only affects shell-internal uses of variables, only for variables 
explicitly unset by a script (rather than unset by default), only for 
uses where the code does not explicitly check for unset beforehand. As 
far as scripts go, that just means PATH (as you found) I think, for 
interactive shells there are some more variables such as PS1/PS2/PS4/MAIL.

My patch is attached.

Cheers,
Harald van Dijk
diff --git a/src/var.c b/src/var.c
index 0d7e1db0..9163cca9 100644
--- a/src/var.c
+++ b/src/var.c
@@ -206,9 +206,9 @@ struct var *setvar(const char *name, const char *val, int flags)
 		vallen = strlen(val);
 	}
 	INTOFF;
-	p = mempcpy(nameeq = ckmalloc(namelen + vallen + 2), name, namelen);
+       p = mempcpy(nameeq = ckmalloc(namelen + vallen + 2), name, namelen + 1);
 	if (val) {
-		*p++ = '=';
+               p[-1] = '=';
 		p = mempcpy(p, val, vallen);
 	}
 	*p = '\0';

Harald,

Thanks for pointing out the flaw in my reasoning.

I can confirm your alternative patch is effective in eradicating
the bug.

Cheers,

Ron

Harald van Dijk <harald@gigawatt.nl> wrote:
> [-- text/plain, encoding 7bit, charset: utf-8, 29 lines --]
> 
> On 12/11/2018 12:53, Ron Yorston wrote:
>> When a variable is unset by calling setvar(name, 0, 0) the code
>> to initialise the new, empty variable omits the trailing '='.
> 
> It's supposed to. A trailing = means the variable is set to an empty 
> string. That's different from unset. You can see the difference with
> set -u, or with ${var+set}. However, ...
> 
>> Attempts to read the contents of the unset variable will result
>> in the uninitialised character at the end of the string being
>> accessed.
> 
> ...this is indeed a bug which I've noticed as well. The code needs two 
> trailing null bytes, not just one. Because of glibc internals, the 
> out-of-bounds byte being read will almost certainly be zero on x86-64, 
> but it's not a guarantee, and it could probably break more easily on 
> other platforms.
> 
> It only affects shell-internal uses of variables, only for variables 
> explicitly unset by a script (rather than unset by default), only for 
> uses where the code does not explicitly check for unset beforehand. As 
> far as scripts go, that just means PATH (as you found) I think, for 
> interactive shells there are some more variables such as PS1/PS2/PS4/MAIL.
> 
> My patch is attached.

Thanks for the patch Harald!

Could you please repost it with a new Subject line? patchwork
is no longer picking up patches posted as a reply.

Please also add a Signed-off-by tag.

Cheers,