Repository navigation
Fix #947: memory leak invoking subshells/functions with trap set in parent environment - #948
Merged
Merged
Conversation
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 clear that bit, so that if the same signal trap is overwritten within the same scope, the free is executed as normal. 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: ksh93#947
Author
|
@vmihalko, @JohnoKing: I would be grateful if you could test and review this pull request. Please try hard to break it. |
JohnoKing
reviewed
Mar 19, 2026
McDutchie
changed the title
|
The current version doesn't crash in my testing, though there is still some memory leaking. Test case: trap "" HUP
function foo
{
trap ": foo" HUP
trap ": bar" HUP
ksh_fun()
(
trap ": foo" HUP
trap ": bar" HUP
)
ksh_fun
ksh_fun
}
foo
fooASan output (prints 4 times, so 24 bytes total): The same reproducer run without the PR has more leaks (64 bytes total), so the current changes improve things nonetheless. |
Author
|
Sigh. Any ideas on where that sh.st.trapcom[SIGHUP] pointer might be getting lost? The ksh_fun function in the reproducer forks due to the parentheses. I can't reproduce the leak without the fork. Is there a way to tell if the leak occurs in the forked child or in the parent? |
|
It's the child: What likely happening is that the parent function's HUP trap isn't getting freed in the child after it changes in the child process (I'd bet it has the nofree bit, so it's left alone). It reminds me of the leaking file descriptor bug fix in 65df83f; resources belonging to the parent that the child doesn't end up using don't get cleaned up in forked subshells, leading to memory leaks in the forked subshells. |
|
Potential fix (perhaps insufficient if more scopes are involved): diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index dff9ee374..8fc7e6984 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -490,6 +490,7 @@ int sh_validate_subpwdfd(void)
void sh_clear_subshell_pwdfd(void)
{
struct subshell *sp = subshell_data;
+ int nsig;
#if _lib_openat
int prevfd = sh.pwdfd;
while(sp)
@@ -513,6 +514,18 @@ void sh_clear_subshell_pwdfd(void)
sp = sp->prev;
}
#endif /* _lib_openat */
+ nsig = sh.st.trapmax;
+ if(nsig)
+ {
+ int isig;
+ for(isig = 0; isig < nsig; ++isig)
+ {
+ const int index = isig / 8;
+ const uint8_t sigbit = (uint8_t)1 << isig % 8;
+ if(sh.st.trapnofree[index] & sigbit)
+ sh.st.trapnofree[index] &= ~sigbit;
+ }
+ }
}
/*
|
Author
Experimental patch (against this PR) to test that theory: diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index dff9ee374..7e5a3c425 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -170,6 +170,7 @@ void sh_subfork(void)
else
{
/* this is the child part of the fork */
+ memset(sh.st.trapnofree, 0, sizeof sh.st.trapnofree);
/*
* In a virtual subshell, $RANDOM is not reseeded until it's used, so if that
* hasn't happened yet, invalidate the seed. This allows sh_save_rand_seed()It does seem to help. I don't know if it doesn't introduce a use after free somewhere else though. |
|
The |
Author
Yes. When sh_subfork() is called, the parent cleans up the virtual subshell and waits for the child. The child simply resets the subshell variables including sh.subshell, continues execution of the formerly virtual subshell in the fork, then returns to sh_subshell, where it simply exits the child process. No cleanup of parent scopes is ever done in the child, they just stay unused. Which makes me think that the I do think sh_subfork() is probably the more correct location for it, as sh_fork() (which calls sh_clear_subshell_pwdfd) is used for more than just forking subshells. |
Author
|
Actually, both of our patches are wrong, because they miss the real issue. The real issue is an error in logic of mine in the b_trap() changes in this PR. b_trap() clears the nofree bit only if there is a freeable parent trap. But it should clear the nofree bit regardless of that. If we're setting a sh.st.trapcom trap action in the current scope, its nofree bit should simply always be cleared. Commit coming up... |
Author
|
So, AFAICT, that leak is gone with the latest commit, can you confirm? |
|
The leak is gone on my end. |
|
Full internal test suite passed with no regressions (tested against the PR version prior to the latest commit). |
Author
|
Thanks for testing, both. In it goes. |
McDutchie
added a commit
that referenced
this pull request
Mar 20, 2026
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: 50db00e) Resolves: #947
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vincent Mihalkovič (@vmihalko) from Red Hat reported a memory leak. See #947.
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:
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:
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:
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():
Resolves: #947