From e3a1dda93ae75fed9b110c0bb8a1b118a38b7842 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Sat, 29 Jan 2022 09:37:48 -0800 Subject: [PATCH] Fix memory leak in defpathinit() (#441) Currently, running ksh under ASan without the ASAN_OPTIONS variable set to 'detect_leaks=0' usually ends with ASan complaining about a memory leak in defpathinit() (this leak doesn't grow in a loop, so no regression test was added to leaks.sh). Reproducer: $ ENV=/dev/null arch/*/bin/ksh $ cp -? cp: invalid option -- '?' Try 'cp --help' for more information. $ exit ================================================================= ==225132==ERROR: LeakSanitizer: detected memory leaks Direct leak of 85 byte(s) in 1 object(s) allocated from: #0 0x7f6dab42d459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x5647b77fe144 in sh_calloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:265 #2 0x5647b788fea9 in path_addcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1567 #3 0x5647b78911ed in path_addpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1705 #4 0x5647b7888e82 in defpathinit /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:442 #5 0x5647b78869f3 in ondefpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:67 --- cut --- SUMMARY: AddressSanitizer: 174 byte(s) leaked in 2 allocation(s). Analysis: The previous code was leaking memory because defpathinit() returns a pointer from path_addpath(), which has memory allocated to it in path_addcomp(). This is the code ASan traced as having allocated memory: 442: return(path_addpath((Pathcomp_t*)0,(defpath),PATH_PATH)); In path_addpath(): 1705: first = path_addcomp(first,old,cp,type); [...] 1729: return(first); In path_addcomp(): 1567: pp = sh_newof((Pathcomp_t*)0,Pathcomp_t,1,len+1); The ondefpath() function doesn't save a reference to the pointer returned by defpathinit(), which causes the memory leak: 66: if(!defpath) 67: defpathinit(); The changes in this commit avoid this problem by setting the defpath variable without also calling path_addpath(). src/cmd/ksh93/sh/path.c: - Move the code for allocating defpath from defpathinit() into its own dedicated function called std_path(). This function is called by defpathinit() and ondefpath() to obtain the current string stored in the defpath variable. This bugfix is adapted from a fork of ksh2020: https://github.com/l0stman/ksh/commit/db5c83a6 --- NEWS | 3 +++ src/cmd/ksh93/sh/path.c | 25 +++++++++++++------------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/NEWS b/NEWS index e26687e14..ae1c17648 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,9 @@ Any uppercase BUG_* names are modernish shell bug IDs. shell when suspending (Ctrl+Z) an external command invoked by a dot script or POSIX shell function, or via 'eval'. +- Fixed a memory leak that occurred when running a command that was found on + the PATH. + 2022-01-26: - On Cygwin, ksh now executes scripts that do not have a #! path itself, diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c index eef684508..9df05bf13 100644 --- a/src/cmd/ksh93/sh/path.c +++ b/src/cmd/ksh93/sh/path.c @@ -58,14 +58,21 @@ static int checkdotpaths(Pathcomp_t*,Pathcomp_t*,Pathcomp_t*,int); static void checkdup(register Pathcomp_t*); static Pathcomp_t *defpathinit(void); -static const char *defpath; /* default path that finds standard utilities */ +static const char *std_path(void) +{ + static const char *defpath; /* default path that finds standard utilities */ + if(!defpath) + { + if(!(defpath = astconf("PATH",NIL(char*),NIL(char*)))) + abort(); + defpath = sh_strdup(defpath); /* the value returned by astconf() is short-lived */ + } + return(defpath); +} static int ondefpath(const char *name) { - const char *cp; - if(!defpath) - defpathinit(); - cp = defpath; + const char *cp = std_path(); if(cp) { const char *sp; @@ -433,13 +440,7 @@ Pathcomp_t *path_nextcomp(register Pathcomp_t *pp, const char *name, Pathcomp_t static Pathcomp_t* defpathinit(void) { - if(!defpath) - { - if(!(defpath = astconf("PATH",NIL(char*),NIL(char*)))) - abort(); - defpath = sh_strdup(defpath); /* the value returned by astconf() is short-lived */ - } - return(path_addpath((Pathcomp_t*)0,(defpath),PATH_PATH)); + return(path_addpath((Pathcomp_t*)0,std_path(),PATH_PATH)); } static void pathinit(void)