| Message ID | 20161201211707.4710-1-andreas@gazonk.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
On Thu, Dec 01, 2016 at 10:17:07PM +0100, Andreas Bofjall wrote: > After dash had executed the exit trap handler, the trap was reset but > the pointer was never freed. This leak can be demonstrated by running > dash through valgrind and executing the following shell script: > > foo() { > true > } > trap foo EXIT > > Fix by properly freeing the trap pointer in exitshell(). > > Signed-off-by: Andreas Bofjall <andreas@gazonk.org> This is not needed because we're about to exit so all memory will be going back to the OS. Cheers,
On Tue, 6 Mar 2018, Herbert Xu wrote: > On Thu, Dec 01, 2016 at 10:17:07PM +0100, Andreas Bofjall wrote: >> After dash had executed the exit trap handler, the trap was reset but >> the pointer was never freed. This leak can be demonstrated by running >> dash through valgrind and executing the following shell script: >> >> foo() { >> true >> } >> trap foo EXIT >> >> Fix by properly freeing the trap pointer in exitshell(). >> >> Signed-off-by: Andreas Bofjall <andreas@gazonk.org> > > This is not needed because we're about to exit so all memory will > be going back to the OS. You're correct, but the problem is the pointer is overwritten before returning. That means before dash has exited, it is properly detected as a leak by e.g. valgrind. I was debugging a memory leak and it was very convenient to be able to quickly wrap some test stuff in dash, which is when I stumbled on this because the tests started failing on this memory leak in dash instead of the leaks I was trying to fix. Perhaps it's an exoteric use case, but it would be nice if it was fixed. /Andreas -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/trap.c b/src/trap.c index edb9938..0824223 100644 --- a/src/trap.c +++ b/src/trap.c @@ -378,7 +378,7 @@ void exitshell(void) { struct jmploc loc; - char *p; + char *p = NULL; savestatus = exitstatus; TRACE(("pid %d, exitshell(%d)\n", getpid(), savestatus)); @@ -391,6 +391,9 @@ exitshell(void) evalstring(p, 0); } out: + INTOFF; + ckfree(p); + INTON; /* * Disable job control so that whoever had the foreground before we * started can get it back.
After dash had executed the exit trap handler, the trap was reset but the pointer was never freed. This leak can be demonstrated by running dash through valgrind and executing the following shell script: foo() { true } trap foo EXIT Fix by properly freeing the trap pointer in exitshell(). Signed-off-by: Andreas Bofjall <andreas@gazonk.org> --- v2: move ckfree() to out path src/trap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)