path: root/shell
diff options
authorGravatar Ron Yorston <rmy@pobox.com>2018-11-12 21:10:54 +0000
committerGravatar Denys Vlasenko <vda.linux@googlemail.com>2018-11-16 17:28:01 +0100
commite6a63bf683f47027d36dc21b62b2f5cc3eb30a30 (patch)
tree726285f34383942ad028d9ba0b85b16f3246fdbf /shell
parent060f0a050a13a37c642f9b1a9d70c1e9861823cf (diff)
ash: ensure variables are fully initialised when unset
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. function old new delta setvar 172 169 -3 Signed-off-by: Ron Yorston <rmy@pobox.com> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
Diffstat (limited to 'shell')
1 files 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)
- 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);