Fix memleak invoking functions with trap set in parent scope (#948)

Vincent Mihalkovič (@vmihalko) from Red Hat reports:
> ksh leaks memory when running a loop that combines trap and
> command substitution. Without the trap or command substitution,
> the loop does not leak. The leak is reproducible on upstream
> HEAD.
>
> ### Reproducer
>
>	trap exit INT
>
>	n=10000
>	while [ "$n" -gt 0 ]; do
>		value=`echo a`
>		n=$((n - 1))
>	done
>	exit 0
>
> Steps:
>
>     1. Save as e.g. `reproducer_limited.ksh` and make executable:
>        chmod +x reproducer_limited.ksh
>     2. Run under valgrind:
>        valgrind --leak-check=full -q /path/to/ksh \
>				reproducer_limited.ksh
>     3. Inspect valgrind output for the LEAK SUMMARY and
>        "definitely lost" lines.
>
> Valgrind reports "definitely lost" blocks from the
> trap/command-substitution path. Example (upstream HEAD):
>
>     * 49,995 bytes in 9,999 blocks are definitely lost
>     * Stack points to: malloc → _ast_strdup → sh_strdup (init.c)
>       → sh_subshell (subshell.c) → comsubst (macro.c) → …
>
> Without the 'trap exit INT' line, the same loop does not show
> "definitely lost" (valgrind is clean for that path); the trap is
> required to trigger the leak.
>
> ### References
>
>     * Downstream: RHEL-19580 – ksh leak with traps
>       https://issues.redhat.com/browse/RHEL-19580
>       (Red Hat Jira). Affects: rhel-7.9, rhel-8.8.0, rhel-9.4.
>     * Related:
>       - Bugzilla 1460940
>         https://bugzilla.redhat.com/show_bug.cgi?id=1460940
>	- Bugzilla 1117404
>         https://bugzilla.redhat.com/show_bug.cgi?id=1117404
>         (trap/crash context).

Analysis:

The Red Hat patch backported back in 2020, in 6193c6a, is not
correct. The original 93u+ code mostly made sense; there is no
reason why saved trap action pointers should change in subshells or
ksh function invocations, because we're creating a new scope and
any changes in traps apply to the new scope only, so the original
pointers should never be invalidated. Any such occurrence is a bug.

The Red Hat patch doesn't actually fix any such bug; it papers over
it at the expense of introducing a memory leak problem. So we have
to revert it, then figure out what the actual bug is and find a
proper fix for that bug.

src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- Mostly revert to the 93u+ method for saving/restoring trap table,
  which simply copies over the array of pointers.
- Avoid reintroducing the off-by-one error that was fixed in
  3aee10d and which carried over into the Red Hat patch.
- sh_subshell(): Allocate the buffer for saving the trap action
  pointers on the AST stack, which is automatically reset
  appropriately at the end of every recursive sh_exec() run, which
  removes the need to free the buffer manually while guaranteeing
  the buffer stays available while we need it. (Note that
  sh_funscope() was already using this method in 93u+.)

With these changes applied, the memory leak problem disappears, and
the regression tests appear to pass normally. However, when
compiling with AddressSanitizer, the patch fails one of the two
regression tests that were introduced by referenced commit, the one
that tests a ksh function. This is not unexpected. The failing test
is at src/cmd/ksh93/tests/functions.sh, lines 1368-1383.

(The other one, at subshell.sh lines 932-943, passes, because as of
3d1a472, ksh always forks a subshell when it traps a signal, which
circumvents the bug.)

The regression is caused by a use after free. The offending free
call is in b_trap(), line 180. That code is essentially unchanged
since 93u+. This use after free appears to be the very problem that
the Red Hat patch from the referenced commit works around.

So we need a new and correct solution for that use after free. It
occurs when a trap for a signal is set or ignored in the parent
environment and is then set again in a ksh function. The function
then needs to be called at twice for the use after free to occur.
Reproducer:

  trap "" HUP
  function ksh_fun
  {
	trap ": foo" HUP
	trap ": bar" HUP
  }
  ksh_fun
  ksh_fun

A simplistic fix would be to avoid the free call at trap.c:180 if
we're currently inside a ksh function. But that would introduce a
memory leak when the same signal is trapped locally more than once
in the same ksh function.

The challenge is this: The first time a trap is set in a ksh
function scope, the parent trap should not be freed, because it
will be restored. However, the second and subsequent times, it
should be freed, because it's overwriting the first one. The
following changes make that happen.

src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/bltins/trap.c,
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- Introduce a sh.st.trapnofree bitmask array, stored in the current
  scope sh.st, which contains a bit for every possible signal. By
  default, all those bits are zero and traps are freed as normal.
- Use the NSIG macro (which indicates the number of signals
  supported by the system; non-standard but available on all major
  systems), to determine the size of the bitmask array if possible.
- sh_subshell(), sh_funscope(), sh_exec(): When saving the array of
  trap action pointers, set all trapnofree bits to one.
- b_trap(): Whenever the trap built-in would free a previous
  (potentially parent) trap, skip the free call if the
  corresponding bit is set, but then (whether a free was actually
  executed or not) clear that bit, so that if the same signal trap
  is overwritten within the same scope, the free is executed as
  normal. This fixes the use after free while avoiding the
  introduction of another memory leak.

While we're here, let's fix another, more minor problem: the shell
unnecessarily forks when setting an EXIT trap, as EXIT is the one
pseudosignal that is handled in the same array as the signal traps.

src/cmd/ksh93/bltins/trap.c: b_trap():
- Since EXIT == 0, check for sig > 0 before forking. (re: 3d1a472)

Resolves: #947

Author: McDutchie

NEWS

@@ -2,6 +2,15 @@ This documents significant changes in the dev branch of ksh 93u+m.
 For full details, see the git log at: https://github.com/ksh93/ksh
 Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.
 
+2026-03-20:
+
+- Fixed a memory leak that occurred when any subshell or ksh function with
+  'function name' syntax is invoked while at least one signal or EXIT trap
+  is set in the invoking environment.
+
+- Fixed an issue, introduced on 2022-06-13, causing a subshell to fork
+  unnecessarily if an EXIT pseudosignal trap is set within it.
+
 2026-03-16:
 
 - Fixed a bug in the join(1) built-in command (bound to /opt/ast/bin) that

src/cmd/ksh93/bltins/trap.c

@@ -164,20 +164,25 @@ int	b_trap(int argc,char *argv[],Shbltin_t *context)
 			}
 			else
 			{
+				const int index = sig / 8;
+				const uint8_t sigbit = (uint8_t)1 << sig % 8;
 				/*
-				 * Trap or ignore a real signal. A virtual subshell needs to fork in
-				 * order to receive signals correctly and (because other commands
+				 * Trap or ignore EXIT (0) or a signal. A virtual subshell must fork
+				 * in order to receive signals correctly and (because other commands
 				 * may cause a virtual subshell to fork) to ensure a persistent PID.
 				 */
-				if(sh.subshell && !sh.subshare)
+				if(sig > 0 && sh.subshell && !sh.subshare)
 					sh_subfork();
 				if(sig >= sh.st.trapmax)
 					sh.st.trapmax = sig+1;
 				arg = sh.st.trapcom[sig];
 				sh_sigtrap(sig);
 				sh.st.trapcom[sig] = (sh.sigflag[sig]&SH_SIGOFF) ? Empty : sh_strdup(action);
-				if(arg && arg != Empty)
+				/* free unless nofree bit is set */
+				if(arg && arg != Empty && !(sh.st.trapnofree[index] & sigbit))
 					free(arg);
+				/* clear nofree bit to avoid memory leak if trap is overwritten in same scope */
+				sh.st.trapnofree[index] &= ~sigbit;
 			}
 		}
 		/*

src/cmd/ksh93/include/shell.h

@@ -194,6 +194,12 @@ typedef struct sh_scope
 
 #if _BLD_ksh
 
+#if NSIG
+#define KSH_NSIG	(NSIG)
+#else
+#define KSH_NSIG	128		/* no UN*X system is currently known to have more than 128 signals */
+#endif
+
 /* Private interface to shell scope. The first members must match the public interface. */
 struct sh_scoped
 {
@@ -225,6 +231,7 @@ struct sh_scoped
 	char		**otrapcom;	/* save parent EXIT and signals for v=$(trap) */
 	void		*timetrap;	/* for the 'alarm' built-in */
 	struct Ufunction *real_fun;	/* current 'function name' function */
+	uint8_t		trapnofree[(KSH_NSIG+7)/8];  /* bitmask to stop b_trap() freeing trapcom entries */
 };
 
 struct limits

src/cmd/ksh93/include/version.h

@@ -18,7 +18,7 @@
 #include 
 #include "git.h"
 
-#define SH_RELEASE_DATE	"2026-03-16"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_DATE	"2026-03-20"	/* must be in this format for $((.sh.version)) */
 /*
  * This comment keeps SH_RELEASE_DATE a few lines away from SH_RELEASE_SVER to avoid
  * merge conflicts when cherry-picking dev branch commits onto a release branch.

src/cmd/ksh93/sh/subshell.c

@@ -525,11 +525,12 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 {
 	struct subshell sub_data;
 	struct subshell *sp = &sub_data;
-	int jmpval,isig,nsig=0,fatalerror=0,saveerrno=0;
+	int n, jmpval, fatalerror = 0, saveerrno = 0;
 	unsigned int savecurenv = sh.curenv;
 	int savejobpgid = job.curpgid;
 	int *saveexitval = job.exitval;
 	char **savsig;
+	size_t nsig = 0;
 	Sfio_t *iop=0;
 	struct checkpt checkpoint;
 	struct sh_scoped savst;
@@ -597,7 +598,7 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 		}
 		if(sp->pwdfd<0)
 		{
-			int n = sh_open(e_dot,O_SEARCH|O_cloexec);
+			n = sh_open(e_dot,O_SEARCH|O_cloexec);
 			if(n>=0)
 			{
 				sp->pwdfd = n;
@@ -618,33 +619,17 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 #endif /* _lib_openat */
 		sp->mask = sh.mask;
 		sh_stats(STAT_SUBSHELL);
-		/* save trap table */
-		sh.st.otrapcom = 0;
-		sh.st.otrap = savst.trap;
-		if((nsig=sh.st.trapmax)>0 || sh.st.trapcom[0])
-		{
-			savsig = sh_malloc(nsig * sizeof(char*));
-			/*
-			 * the data is, usually, modified in code like:
-			 *	tmp = buf[i]; buf[i] = sh_strdup(tmp); free(tmp);
-			 * so sh.st.trapcom needs a "deep copy" to properly save/restore pointers.
-			 */
-			for (isig = 0; isig < nsig; ++isig)
-			{
-				if(sh.st.trapcom[isig] == Empty)
-					savsig[isig] = Empty;
-				else if(sh.st.trapcom[isig])
-					savsig[isig] = sh_strdup(sh.st.trapcom[isig]);
-				else
-					savsig[isig] = NULL;
-			}
-			/* this is needed for var=$(trap) */
-			sh.st.otrapcom = (char**)savsig;
-		}
 		sp->cpid = sh.cpid;
 		sp->coutpipe = sh.coutpipe;
 		sp->cpipe = sh.cpipe[1];
 		sh.cpid = 0;
+		/* save trap table */
+		memset(sh.st.trapnofree, 0xFF, sizeof sh.st.trapnofree);
+		sh.st.otrap = savst.trap;
+		if((nsig = sh.st.trapmax * sizeof(char**)) > 0)
+			savsig = sh.st.otrapcom = memcpy(stkalloc(sh.stk, nsig), sh.st.trapcom, nsig);
+		else
+			sh.st.otrapcom = NULL;
 		if(sh_isoption(SH_FUNCTRACE) && sh.st.trap[SH_DEBUGTRAP] && *sh.st.trap[SH_DEBUGTRAP])
 			save_debugtrap = sh_strdup(sh.st.trap[SH_DEBUGTRAP]);
 		sh_sigreset(0);
@@ -805,7 +790,6 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 	sh.bckpid = sp->bckpid;
 	if(!sh.subshare)	/* restore environment if saved */
 	{
-		int n;
 		struct rand *rp;
 		sh.options = sp->options;
 		/* Clean up subshell hash table. */
@@ -862,13 +846,7 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 		sh.st = savst;
 		sh.st.otrap = 0;
 		if(nsig)
-		{
-			for (isig = 0; isig < nsig; ++isig)
-				if (sh.st.trapcom[isig] && sh.st.trapcom[isig]!=Empty)
-					free(sh.st.trapcom[isig]);
-			memcpy((char*)&sh.st.trapcom[0],savsig,nsig*sizeof(char*));
-			free(savsig);
-		}
+			memcpy(sh.st.trapcom, savsig, nsig);
 		sh.options = sp->options;
 #if _lib_openat
 		if(sh.pwdfd != sp->pwdfd)
@@ -957,10 +935,10 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 	}
 	sh_sigcheck();
 	sh.trapnote = 0;
-	nsig = sh.savesig;
+	n = sh.savesig;
 	sh.savesig = 0;
-	if(nsig>0)
-		kill(sh.current_pid,nsig);
+	if(n > 0)
+		kill(sh.current_pid, n);
 	if(sp->subpid)
 		job_wait(sp->subpid);
 	sh.comsub = sp->comsub;

src/cmd/ksh93/sh/xec.c

@@ -1718,22 +1718,21 @@ int sh_exec(const Shnode_t *t, int flags)
 			flags &= ~ARG_OPTIMIZE;
 			if(!sh.subshell && !sh.st.trapdontexec && (flags&sh_state(SH_NOFORK)))
 			{
-				/* This is the last command, so avoid creating a subshell */
-				char *savsig;
-				int nsig,jmpval;
+				/* This is the last command, so avoid creating a subshell, but still act like one */
+				size_t nsig;
+				int jmpval;
 				struct checkpt *buffp = stkalloc(sh.stk,sizeof(struct checkpt));
-				sh.st.otrapcom = 0;
-				if((nsig=sh.st.trapmax*sizeof(char*))>0 || sh.st.trapcom[0])
-				{
-					nsig += sizeof(char*);
-					savsig = sh_malloc(nsig);
-					memcpy(savsig,(char*)&sh.st.trapcom[0],nsig);
-					sh.st.otrapcom = (char**)savsig;
-				}
-				/* Still act like a subshell: reseed $RANDOM and increment ${.sh.subshell} */
+				/* Save traps for printing, then reset them */
+				memset(sh.st.trapnofree, 0xFF, sizeof sh.st.trapnofree);
+				if ((nsig = sh.st.trapmax * sizeof(char*)) > 0)
+					sh.st.otrapcom = memcpy(stkalloc(sh.stk, nsig), sh.st.trapcom, nsig);
+				else
+					sh.st.otrapcom = NULL;
+				sh_sigreset(0);
+				/* Reseed $RANDOM and increment ${.sh.subshell} */
 				sh_invalidate_rand_seed();
 				sh.realsubshell++;
-				sh_sigreset(0);
+				/* Execute the last command and exit normally, except for SH_JMPSCRIPT */
 				sh_pushcontext(buffp,SH_JMPEXIT);
 				jmpval = sigsetjmp(buffp->buff,0);
 				if(jmpval==0)
@@ -2900,16 +2899,16 @@ Sfdouble_t sh_mathfun(void *fp, int nargs, Sfdouble_t *arg)
 int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 {
 	char			*trap;
-	int			nsig;
 	struct dolnod		*argsav=0,*saveargfor;
 	struct sh_scoped	*savst = stkalloc(sh.stk,sizeof(struct sh_scoped));
 	struct sh_scoped	*prevscope = sh.st.self;
 	struct argnod		*envlist=0;
-	int			isig,jmpval;
+	int			jmpval;
 	volatile int		r = 0;
 	int			posix_fun = 0, save_loopcnt = sh.st.loopcnt;
 	char			save_invoc_local;
 	char 			**savsig;
+	size_t			nsig;
 	struct funenv		*fp = 0;
 	struct checkpt		*buffp = stkalloc(sh.stk,sizeof(struct checkpt));
 	Namval_t		*nspace = sh.namespace;
@@ -2987,24 +2986,9 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 		}
 		sh.st.cmdname = argv[0];
 		/* save trap table */
-		if((nsig=sh.st.trapmax)>0 || sh.st.trapcom[0])
-		{
-			savsig = sh_malloc((size_t)nsig * sizeof(char*));
-			/*
-			 * the data is, usually, modified in code like:
-			 *	tmp = buf[i]; buf[i] = sh_strdup(tmp); free(tmp);
-			 * so sh.st.trapcom needs a "deep copy" to properly save/restore pointers.
-			 */
-			for (isig = 0; isig < nsig; ++isig)
-			{
-				if(sh.st.trapcom[isig] == Empty)
-					savsig[isig] = Empty;
-				else if(sh.st.trapcom[isig])
-					savsig[isig] = sh_strdup(sh.st.trapcom[isig]);
-				else
-					savsig[isig] = NULL;
-			}
-		}
+		memset(sh.st.trapnofree, 0xFF, sizeof sh.st.trapnofree);
+		if((nsig = sh.st.trapmax * sizeof(char**)) > 0)
+			savsig = memcpy(stkalloc(sh.stk, nsig), sh.st.trapcom, nsig);
 		if(!fun && sh_isoption(SH_FUNCTRACE) && sh.st.trap[SH_DEBUGTRAP] && *sh.st.trap[SH_DEBUGTRAP])
 			save_debugtrap = sh_strdup(sh.st.trap[SH_DEBUGTRAP]);
 		sh_sigreset(-1);
@@ -3119,13 +3103,7 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 	sh.topscope = (Shscope_t*)prevscope;
 	nv_getval(sh_scoped(IFSNOD));
 	if(nsig)
-	{
-		for (isig = 0; isig < nsig; ++isig)
-			if (sh.st.trapcom[isig] && sh.st.trapcom[isig]!=Empty)
-				free(sh.st.trapcom[isig]);
-		memcpy((char*)&sh.st.trapcom[0],savsig,nsig*sizeof(char*));
-		free(savsig);
-	}
+		memcpy(sh.st.trapcom, savsig, nsig);
 	sh.trapnote=0;
 	sh.options = save_options;
 	sh.last_root = last_root;

src/cmd/ksh93/tests/leaks.sh

@@ -456,5 +456,15 @@ DO
 	command -px true
 DONE
 
+# ======
+TEST	title='trap and command substitution'
+	trap ': long-enough trap action to detect the leak' USR1
+DO
+	v=`echo a`
+	v=$(echo a)
+	(echo a)
+DONE >/dev/null
+trap - USR1
+
 # ======
 exit $((Errors<125?Errors:125))