.sh.tilde discipline is too brittle #346
Comments
Defining a .sh.tilde.get or .sh.tilde.set discipline function to extend tilde expansion works well as long as the discipline function doesn't get interrupted (e.g. with Crtl+C) or produce an error message. Either of those will cause the shell to become unstable and crash. This feature is now removed from the 1.0 branch as it is not ready for prime time. It can return to a release branch if/when we manage to fix it on the master branch. Related: #346
|
Perhaps --- a/src/cmd/ksh93/sh/macro.c
+++ b/src/cmd/ksh93/sh/macro.c
@@ -2638,6 +2638,7 @@ static void tilde_expand2(Shell_t *shp, register int offset)
{
stakfreeze(1); /* terminate current stack object to avoid data corruption */
block++;
+ setjmp(*shp->jmplist);
nv_putval(SH_TILDENOD, &stakp[offset], 0);
cp = nv_getval(SH_TILDENOD);
block--;# Results after applying the patch above
$ arch/*/bin/ksh
$ echo ~
arch/linux.i386-64/bin/ksh[44]: trap: -Q: unknown option
Usage: trap [-p] [action condition ...]
Help: trap [ --help | --man ] 2>&1
/home/johno
ksh $ echo ~ # Press tab
arch/linux.i386-64/bin/ksh[44]: trap: -Q: unknown option
Usage: trap [-p] [action condition ...]
Help: trap [ --help | --man ] 2>&1
/home/johno
/home/johno
$ echo ~ksh
arch/linux.i386-64/bin/ksh[44]: trap: -Q: unknown option
Usage: trap [-p] [action condition ...]
Help: trap [ --help | --man ] 2>&1
~kshEdit: See also #347 (comment), which demonstrates this bug likely applies to all discipline functions. |
|
As expected, the discipline function for $ PS1.get() { trap -Q; }
/usr/bin/ksh: trap: -Q: unknown option
Usage: trap [-p] [action condition ...]
Help: trap [ --help | --man ] 2>&1
/usr/bin/ksh: trap: -Q: unknown option
Usage: trap [-p] [action condition ...]
Help: trap [ --help | --man ] 2>&1
# FreezesAlso as expected, |
|
After yet more experimentation I've come up with this patch (edit: patch updated to avoid using --- a/src/cmd/ksh93/sh/nvdisc.c
+++ b/src/cmd/ksh93/sh/nvdisc.c
@@ -283,11 +283,16 @@ static void assign(Namval_t *np,const char* val,int flags,Namfun_t *handle)
nq = vp->disc[type=UNASSIGN];
if(nq && !isblocked(bp,type))
{
- int bflag=0, savexit=sh.savexit;
+ int bflag=0, savexit=sh.savexit, jmpval=0;
+ struct checkpt buff;
block(bp,type);
if (type==APPEND && (bflag= !isblocked(bp,LOOKUPS)))
block(bp,LOOKUPS);
- sh_fun(nq,np,(char**)0);
+ sh_pushcontext(&sh,&buff,1);
+ jmpval = sigsetjmp(buff.buff,0);
+ if(!jmpval)
+ sh_fun(nq,np,(char**)0);
+ sh_popcontext(&sh,&buff);
unblock(bp,type);
if(bflag)
unblock(bp,LOOKUPS);
@@ -376,7 +381,8 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
union Value *up = np->nvalue.up;
if(nq && !isblocked(bp,type))
{
- int savexit = sh.savexit;
+ int savexit = sh.savexit, jmpval = 0;
+ struct checkpt buff;
node = *SH_VALNOD;
if(!nv_isnull(SH_VALNOD))
{
@@ -389,7 +395,11 @@ static char* lookup(Namval_t *np, int type, Sfdouble_t *dp,Namfun_t *handle)
nv_setsize(SH_VALNOD,10);
}
block(bp,type);
- sh_fun(nq,np,(char**)0);
+ sh_pushcontext(&sh,&buff,1);
+ jmpval = sigsetjmp(buff.buff,0);
+ if(!jmpval)
+ sh_fun(nq,np,(char**)0);
+ sh_popcontext(&sh,&buff);
unblock(bp,type);
if(!vp->disc[type])
chktfree(np,vp);It makes ksh do a longjmp back to the correct context after a discipline function exits with an error. The |
|
Does this fix the crash in --- a/src/cmd/ksh93/sh/parse.c
+++ b/src/cmd/ksh93/sh/parse.c
@@ -284,6 +284,7 @@ static void dcl_dehacktivate(void)
}
static noreturn void dcl_exit(int e)
{
+ dcl_recursion = 1;
dcl_dehacktivate();
(*error_info.exit)(e);
UNREACHABLE(); |
|
BTW, side note: let's not bother with introducing more pointless |
Nope, it still crashes.
I've updated the patch to avoid using |
|
I think I've made sense of why that crash occurs. The PS2 discipline is run while trying to parse a command. So it's another context where arbitrary code might be executed at parse time, which messes up that hack. Aaaargh. Apparently at AT&T they'd never heard about the idea of having a clean separation between distinct subsystems! Anyway, the backtrace confirms that hypothesis, as it includes edit: But that'll have to be for later, I simply don't have time for that right now. I've reverted that commit until it can be fixed. |
This patch fixes the crashes experienced when a discipline function exited because of a signal or an error from a special builtin. The crashes were caused by ksh entering an inconsistent state after performing a longjmp away from the assign() and lookup() functions in nvdisc.c. Fixing the crash requires entering a new context, then setting a nonlocal goto with sigsetjmp(3). Any longjmps that happen while running the discipline function will go back to assign/lookup, allowing ksh to do a proper cleanup afterwards. Resolves: ksh93#346
…356) This patch fixes the crashes experienced when a discipline function exited because of a signal or an error from a special builtin. The crashes were caused by ksh entering an inconsistent state after performing a longjmp away from the assign() and lookup() functions in nvdisc.c. Fixing the crash requires entering a new context, then setting a nonlocal goto with sigsetjmp(3). Any longjmps that happen while running the discipline function will go back to assign/lookup, allowing ksh to do a proper cleanup afterwards. Resolves: #346
…356) This patch fixes the crashes experienced when a discipline function exited because of a signal or an error from a special builtin. The crashes were caused by ksh entering an inconsistent state after performing a longjmp away from the assign() and lookup() functions in nvdisc.c. Fixing the crash requires entering a new context, then setting a nonlocal goto with sigsetjmp(3). Any longjmps that happen while running the discipline function will go back to assign/lookup, allowing ksh to do a proper cleanup afterwards. Resolves: #346
Defining a .sh.tilde.get or .sh.tilde.set discipline function to extend tilde expansion works well as long as the discipline function doesn't get interrupted (e.g. with Crtl+C) or produce an error message. Either of those will cause the shell to become unstable and crash.
To reproduce: To trigger an interrupt, adding 'sleep 1' to the discipline function makes it easy to press Ctrl+C. To trigger an error, give an invalid option to a special built-in, e.g.
trap -Q.I have experimented with two different techniques to fix this, with no success at all so far.
sh_pushcontextis getting me nowhere and running the command viash_trapdoesn't seem to help either.A workaround for the interruption crash is to make it uninterruptible with
sigblock(SIGINT)intilde_expand2()before running the discipline andsigrelease(SIGINT)after. But that does nothing to fix the crash on error.This feature should probably be removed from the 1.0 branch as it is not ready for prime time. It can return to a release branch if/when we manage to fix it on the master branch.
The text was updated successfully, but these errors were encountered: