Skip to content

Commit

Permalink
Fix command substitution memory leaks (rhbz#982142)
Browse files Browse the repository at this point in the history
This fixes two memory leaks in old-style command substitutions
(one when invoking an alias, one when invoking an autoloaded
function), as well as a possible third leak with an unknown
reproducer, by applying this Red Hat patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-mlikfiks.patch

src/cmd/ksh93/sh/macro.c: comsubst():
- For as-yet unknown reasons, the alias leak did not occur when
  adding a space at the end of the command substitution, as in
  a=`some_alias `. This fix is a workaround that simply writes
  an extra space to the stack. TODO: a real fix.

src/cmd/ksh93/sh/path.c: funload():
- Add missing free() before return. This fixes the leak with
  autoloaded functions.

src/cmd/ksh93/sh/lex.c: alias_exceptf():
- This function is called "whenever an end of string is found with
  alias". This adds a check for an SF_FINAL stream status flag when
  deciding whether to call free(). In sfio.h this is commented as:
      #define SF_FINAL 11 /* closing is done except stream free */
  When I revert this change, none of the regression tests fail, so
  I don't know how to trigger this supposed leak. But it makes some
  sense given the sfio.h comment, so I'll keep it.

src/cmd/ksh93/tests/leaks.sh:
- Add the reproducers from rhbz#982142 as regression tests
  (including an extra one for nested command substitutions that was
  already fixed as of 93u+, but testing is good).
     I replaced the external 'expr' and 'ls' commands by uses of
  the 'true' builtin, otherwise the tests take far too long to run
  with 16384 iterations. At least the alias leak was still behaving
  identically after replacing 'ls' by 'true'.
  • Loading branch information
McDutchie committed Sep 20, 2020
1 parent e661191 commit fe20311
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/cmd/ksh93/sh/lex.c
Original file line number Diff line number Diff line change
Expand Up @@ -2445,7 +2445,7 @@ static int alias_exceptf(Sfio_t *iop,int type,Sfdisc_t *handle)
if(dp!=handle)
sfdisc(iop,dp);
}
else if(type==SF_FINAL)
else if(type==SF_DPOP || type==SF_FINAL)
free((void*)ap);
goto done;
}
Expand Down
1 change: 1 addition & 0 deletions src/cmd/ksh93/sh/macro.c
Original file line number Diff line number Diff line change
Expand Up @@ -2110,6 +2110,7 @@ static void comsubst(Mac_t *mp,register Shnode_t* t, int type)
}
sfputc(stkp,c);
}
sfputc(stkp,' '); /* rhbz#982142: a=`some_alias` leaked memory, a=`some_alias ` did not! TODO: non-hack fix */
c = stktell(stkp);
str=stkfreeze(stkp,1);
/* disable verbose and don't save in history file */
Expand Down
1 change: 1 addition & 0 deletions src/cmd/ksh93/sh/path.c
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,7 @@ static void funload(Shell_t *shp,int fno, const char *name)
}
while((rp=dtnext(shp->fpathdict,rp)) && strcmp(pname,rp->fname)==0);
sh_close(fno);
free((void*)pname);
return;
}
sh_onstate(SH_NOLOG);
Expand Down
40 changes: 40 additions & 0 deletions src/cmd/ksh93/tests/leaks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -270,5 +270,45 @@ done
after=$(getmem)
err_exit_if_leak 'unalias'

# ======
# Red Hat bug rhbz#982142: command substitution leaks

# case1: Nested command substitutions
# (reportedly already fixed in 93u+, but let's keep the test)
before=$(getmem)
for ((i=0; i < N; i++))
do a=`true 1 + \`true 1 + 1\`` # was: a=`expr 1 + \`expr 1 + 1\``
done
after=$(getmem)
err_exit_if_leak 'nested command substitutions'

# case2: Command alias
alias ls='true -ltr' # was: alias ls='ls -ltr'
before=$(getmem)
for ((i=0; i < N; i++))
do eval 'a=`ls`'
done
after=$(getmem)
unalias ls
err_exit_if_leak 'alias in command substitution'

# case3: Function call via autoload
cat >$tmp/func1 <<\EOF
function func1
{
echo "func1 call";
}
EOF
FPATH=$tmp
autoload func1
before=$(getmem)
for ((i=0; i < N; i++))
do a=`func1`
done
after=$(getmem)
unset -f func1
unset -v FPATH
err_exit_if_leak 'function call via autoload in command substitution'

# ======
exit $((Errors<125?Errors:125))

3 comments on commit fe20311

@JohnoKing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found an email on the old mailing list with a different fix for the memory leak in macro.c: https://www.mail-archive.com/ast-developers@lists.research.att.com/msg01889.html
The patch in the email could replace the hack fix for a=some_alias. I tested it and all the regression tests pass when the email's patch is used instead.

@McDutchie
Copy link
Author

@McDutchie McDutchie commented on fe20311 Feb 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer. Unfortunately, when I replace the change in macro.c from this commit with the patch from that email, I do get a leak regression:

#### Regression-testing /usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh ####
test leaks begins at 2021-02-18+21:46:02
	leaks.sh[313]: alias in command substitution (leaked approx 5132 KiB after 16384 iterations)
test leaks failed at 2021-02-18+21:46:39 with exit code 1 [ 33 tests 1 error ]
Total errors: 1
CPU time       user:      system:
main:      0m00.008s    0m00.011s
tests:     0m14.798s    0m22.562s

@McDutchie
Copy link
Author

@McDutchie McDutchie commented on fe20311 Feb 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have done something wrong. I tried the fix from the mailing list again and the leak is gone.

I still don't like it, though. It's just as hackish as the Red Hat one.

And in fact it does now (somehow) cause one regression, of a test recently added:

test subshell begins at 2021-02-28+00:50:40
	subshell.sh[960]: backtick comsub with alias: expected word, got 'wo rd'
test subshell failed at 2021-02-28+00:50:53 with exit code 1 [ 119 tests 1 error ]

The real fix to figure out is how to actually throw a syntax error on something like echo `"ls foo`. It's bogus and the parser shouldn't accept it.

Please sign in to comment.