| Message ID | 5be977d1.kOQ/st5GzBzGJ5of%rmy@frippery.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | var: ensure variables are fully initialised when unset | expand |
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,
diff --git a/src/var.c b/src/var.c index 0d7e1db..d4d8bd2 100644 --- a/src/var.c +++ b/src/var.c @@ -207,8 +207,8 @@ struct var *setvar(const char *name, const char *val, int flags) } INTOFF; p = mempcpy(nameeq = ckmalloc(namelen + vallen + 2), name, namelen); + *p++ = '='; if (val) { - *p++ = '='; p = mempcpy(p, val, vallen); } *p = '\0';
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(-)