From e87dbebebdc168dd8118c365fd9458d558ecb503 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Sat, 19 Feb 2022 12:55:35 -0800 Subject: [PATCH] Fix use after free bug when using += (re: 75796a9c) (#466) The previous fix for the += operator introduced a use-after-free bug that could result in a variable pointing to random garbage: $ foo=bar $ foo+=_foo true $ typeset -p foo foo=V V The use after free issue occurs because when nv_clone creates a copy of $foo in the true command's invocation-local scope, it does not duplicate the string $foo points to. As a result, the $foo variable in the parent scope points to the same string as $foo in the invocation-local scope, which causes the use after free bug when cloned $foo variable is freed from memory. src/cmd/ksh93/sh/nvdisc.c: - To fix the use after free bug, allow nv_clone to duplicate the string with memdup or strdup when no flags are passed. src/cmd/ksh93/tests/variables.sh: - Add a regression test for using the += operator with regular commands. src/cmd/ksh93/tests/leaks.sh: - Add a regression test to ensure the bugfix doesn't introduce any memory leaks. --- NEWS | 10 ++++++++-- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/nvdisc.c | 2 +- src/cmd/ksh93/tests/leaks.sh | 17 +++++++++++++++++ src/cmd/ksh93/tests/variables.sh | 9 +++++++++ 5 files changed, 36 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 5c28ac7ae..ddf09b3e4 100644 --- a/NEWS +++ b/NEWS @@ -3,7 +3,13 @@ For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0 Any uppercase BUG_* names are modernish shell bug IDs. -2022-01-17: +2022-02-18: + +- Fixed a regression introduced on 2021-04-11 that caused the += operator in + invocation-local assignments to crash the shell or modify variables outside + of the invocation-local scope. + +2022-02-17: - Fixed a crash, introduced on 2021-01-19, that occurred when using 'cd' in a subshell with the PWD variable unset. @@ -11,7 +17,7 @@ Any uppercase BUG_* names are modernish shell bug IDs. - Fixed a crash that could occur when or after entering the suspend character (Ctrl+Z) while the shell was blocked trying to write to a FIFO special file. -2022-01-16: +2022-02-16: - Backported minor additions to the 'read' built-in command from ksh 93v-: '-a' is now the same as '-A' and '-u p' is the same as '-p'. This is for diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 70c073027..d5d23c617 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -21,7 +21,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-beta.2" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2022-02-17" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2022-02-18" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh/nvdisc.c b/src/cmd/ksh93/sh/nvdisc.c index 189dd6db2..114ccd2b8 100644 --- a/src/cmd/ksh93/sh/nvdisc.c +++ b/src/cmd/ksh93/sh/nvdisc.c @@ -972,7 +972,7 @@ int nv_clone(Namval_t *np, Namval_t *mp, int flags) mp->nvflag |= (np->nvflag&NV_MINIMAL); if(mp->nvalue.cp==val && !nv_isattr(np,NV_INTEGER)) { - if(np->nvalue.cp && np->nvalue.cp!=Empty && (flags&NV_COMVAR) && !(flags&NV_MOVE)) + if(np->nvalue.cp && np->nvalue.cp!=Empty && (!flags || ((flags&NV_COMVAR) && !(flags&NV_MOVE)))) { if(size) mp->nvalue.cp = (char*)sh_memdup(np->nvalue.cp,size); diff --git a/src/cmd/ksh93/tests/leaks.sh b/src/cmd/ksh93/tests/leaks.sh index 5d4184002..cdef3a755 100755 --- a/src/cmd/ksh93/tests/leaks.sh +++ b/src/cmd/ksh93/tests/leaks.sh @@ -424,5 +424,22 @@ DO ulimit --man 2>/dev/null DONE +# ====== +# Possible memory leak when using the += operator on variables +# in an invocation-local scope. +bintrue=$(whence -p true) +testfunc() { true; } +alias testalias=testfunc +TEST title='+= operator used before command' +DO + baz=bar + baz+=baz : # Special builtin + baz+=foo true # Regular builtin + baz+=foo "$bintrue" # External command + baz+=foo testfunc # Function + baz+=foo testalias # Alias + unset baz +DONE + # ====== exit $((Errors<125?Errors:125)) diff --git a/src/cmd/ksh93/tests/variables.sh b/src/cmd/ksh93/tests/variables.sh index 2d2f9a7f1..48f3cd039 100755 --- a/src/cmd/ksh93/tests/variables.sh +++ b/src/cmd/ksh93/tests/variables.sh @@ -1356,5 +1356,14 @@ got=$("$SHELL" -c 'echo $SHLVL; "$SHELL" -c "echo \$SHLVL"; exec "$SHELL" -c "ec [[ $got == "$exp" ]] || err_exit "SHLVL not increased correctly" \ "(expected $(printf %q "$exp"), got $(printf %q "$got"))" +# ====== +# The += operator should not free variables outside of its +# scope when used in an invocation-local assignment. +exp='baz_foo +baz' +got=$("$SHELL" -c $'foo=baz; foo+=_foo "$SHELL" -c \'print $foo\'; print $foo') +[[ $exp == "$got" ]] || err_exit "using the += operator for invocation-local assignments changes variables outside of the invocation-local scope" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" + # ====== exit $((Errors<125?Errors:125))