Skip to content

Commit 361fe1f

Browse files
committed
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 9906535, 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.
1 parent 5e7d335 commit 361fe1f

3 files changed

Lines changed: 21 additions & 1 deletion

File tree

‎NEWS‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ Any uppercase BUG_* names are modernish shell bug IDs.
99

1010
- Fixed a crash when listing indexed arrays.
1111

12+
- Fixed a memory leak when restoring PATH when temporarily setting PATH
13+
for a command (e.g. PATH=/foo/bar command ...) or in a virtual subshell.
14+
1215
2020-07-07:
1316

1417
- Four of the date formats accepted by 'printf %()T' have had their

‎src/cmd/ksh93/sh/path.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1800,7 +1800,9 @@ void path_alias(register Namval_t *np,register Pathcomp_t *pp)
18001800
Pathcomp_t *old;
18011801
nv_offattr(np,NV_NOPRINT);
18021802
nv_stack(np,&talias_init);
1803-
old = np->nvalue.pathcomp;
1803+
old = (Pathcomp_t*)np->nvalue.cp;
1804+
if (old && (--old->refcount <= 0))
1805+
free((void*)old);
18041806
np->nvalue.cp = (char*)pp;
18051807
pp->refcount++;
18061808
nv_setattr(np,NV_TAGGED|NV_NOFREE);

‎src/cmd/ksh93/tests/leaks.sh‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,5 +121,20 @@ after=$(getmem)
121121
(( after > before )) && err_exit 'unset of associative array causes memory leak' \
122122
"(leaked $((after - before)) $unit)"
123123

124+
# ======
125+
# Memory leak when resetting PATH and clearing hash table
126+
# ...steady memory state:
127+
command -v ls >/dev/null # add something to hash table
128+
PATH=/dev/null true # set/restore PATH & clear hash table
129+
# ...test for leak:
130+
before=$(getmem)
131+
for ((i=0; i<16; i++))
132+
do PATH=/dev/null true # set/restore PATH & clear hash table
133+
command -v ls # do PATH search, add to hash table
134+
done >/dev/null
135+
after=$(getmem)
136+
(( after > before )) && err_exit 'memory leak on PATH reset before subshell PATH search' \
137+
"(leaked $((after - before)) $unit)"
138+
124139
# ======
125140
exit $((Errors<125?Errors:125))

0 commit comments

Comments
 (0)