From 361fe1fcc3097568b08d63a5ba3f979eee17c408 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Thu, 9 Jul 2020 18:34:15 +0100 Subject: [PATCH] Fix hash table memory leak when restoring PATH There is a bug in path_alias() that may cause a memory leak when clearing the hash table while setting/restoring PATH. This applies a fix from Siteshwar Vashist: https://www.mail-archive.com/ast-developers@lists.research.att.com/msg01945.html Note that, contrary to Siteshwar's analysis linked above, this bug has nothing directly to do with subshells, forked or otherwise; it can also be reproduced by temporarily setting PATH for a command, for example, 'PATH=/dev/null true', and then doing a PATH search. Modified analysis: ksh maintains the value of PATH as a linked list. When a local scope for PATH is created (e.g. in a virtual subshell or when doing something like PATH=/foo/bar command ...), ksh duplicates PATH by increasing the refcount for every element in the linked list by calling the path_dup() and path_alias() functions. However, when the state of PATH is restored, this refcount is not decreased. Next time when PATH is reset to a new value, ksh calls the path_delete() function to delete the linked list that stored the older path. But the path_delete() function does not free elements whose refcount is greater than 1, causing a memory leak. src/cmd/ksh93/sh/path.c: path_alias(): - Decrease refcount and free old item if needed. (The 'old' variable was already introduced in 99065353, but its value was never used there; this fixes that as well.) src/cmd/ksh93/tests/leaks.sh: - Add regression test. With the bug, setting/restoring PATH (which clears the hash table) and doing a PATH search 16 times causes about 1.5 KiB of memory to be leaked. --- NEWS | 3 +++ src/cmd/ksh93/sh/path.c | 4 +++- src/cmd/ksh93/tests/leaks.sh | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 9ab2d57804e8..e97a6150f487 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,9 @@ Any uppercase BUG_* names are modernish shell bug IDs. - Fixed a crash when listing indexed arrays. +- Fixed a memory leak when restoring PATH when temporarily setting PATH + for a command (e.g. PATH=/foo/bar command ...) or in a virtual subshell. + 2020-07-07: - Four of the date formats accepted by 'printf %()T' have had their diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c index 25ade4073dad..3059bafbe429 100644 --- a/src/cmd/ksh93/sh/path.c +++ b/src/cmd/ksh93/sh/path.c @@ -1800,7 +1800,9 @@ void path_alias(register Namval_t *np,register Pathcomp_t *pp) Pathcomp_t *old; nv_offattr(np,NV_NOPRINT); nv_stack(np,&talias_init); - old = np->nvalue.pathcomp; + old = (Pathcomp_t*)np->nvalue.cp; + if (old && (--old->refcount <= 0)) + free((void*)old); np->nvalue.cp = (char*)pp; pp->refcount++; nv_setattr(np,NV_TAGGED|NV_NOFREE); diff --git a/src/cmd/ksh93/tests/leaks.sh b/src/cmd/ksh93/tests/leaks.sh index 2fd0aacf3aa3..9336289bc503 100755 --- a/src/cmd/ksh93/tests/leaks.sh +++ b/src/cmd/ksh93/tests/leaks.sh @@ -121,5 +121,20 @@ after=$(getmem) (( after > before )) && err_exit 'unset of associative array causes memory leak' \ "(leaked $((after - before)) $unit)" +# ====== +# Memory leak when resetting PATH and clearing hash table +# ...steady memory state: +command -v ls >/dev/null # add something to hash table +PATH=/dev/null true # set/restore PATH & clear hash table +# ...test for leak: +before=$(getmem) +for ((i=0; i<16; i++)) +do PATH=/dev/null true # set/restore PATH & clear hash table + command -v ls # do PATH search, add to hash table +done >/dev/null +after=$(getmem) +(( after > before )) && err_exit 'memory leak on PATH reset before subshell PATH search' \ + "(leaked $((after - before)) $unit)" + # ====== exit $((Errors<125?Errors:125))