From 092b90da81f1ab1d45495052615fb4bf83a25d03 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Wed, 9 Sep 2020 20:02:20 +0200 Subject: [PATCH] Fix BUG_LOOPRET2 and related return/exit misbehaviour The 'exit' and 'return' commands without an argument failed to pass down the exit status of the last-run command when incorporated in a block with redirection, &&/|| list, 'case' statement, or 'while', 'until' or 'for' loop. src/cmd/ksh93/bltins/cflow.c: - Use $?, which is sh.savexit a.k.a. shp->savexit, as the default exit status value if there is no argument, instead of shp->oldexit. This fixes the default exit status behaviour to match POSIX and other shells. src/cmd/ksh93/include/defs.h, src/cmd/ksh93/include/shell.h: - Remove now-unused sh.oldexit (a.k.a. shp->oldexit) private struct member. It appeared to fulfill the same function as sh.savexit, but in a slightly broken way. - Move the savexit/$? declaration from the _SH_PRIVATE part of the struct definition to the public API part. Since $? uses this, it's clearly a publicly exposed value already, and this is generally the one to use. (If anything, it's exitval that should have been private.) This declares savexit right next to exitval, rewriting the comments to clarify the difference between them. src/cmd/ksh93/sh/fault.c, src/cmd/ksh93/sh/subshell.c, src/cmd/ksh93/sh/xec.c: - Remove assignments to shp->oldexit. src/cmd/ksh93/tests/basic.sh: - Add thorough regression tests for the default exit status behaviour of 'return' and 'exit' in various lexical contexts. - Verify that 'for' and 'case' without any command, as well as a lone redirection, still correctly reset the exit status to 0. Fixes: #117 --- NEWS | 12 +++++ TODO | 6 --- src/cmd/ksh93/bltins/cflow.c | 2 +- src/cmd/ksh93/include/defs.h | 2 - src/cmd/ksh93/include/shell.h | 3 +- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/fault.c | 1 - src/cmd/ksh93/sh/subshell.c | 1 - src/cmd/ksh93/sh/xec.c | 2 - src/cmd/ksh93/tests/basic.sh | 82 +++++++++++++++++++++++++++++++++ 10 files changed, 98 insertions(+), 15 deletions(-) diff --git a/NEWS b/NEWS index 51dd10539..8ab758819 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,18 @@ For full details, see the git log at: https://github.com/ksh93/ksh Any uppercase BUG_* names are modernish shell bug IDs. +2020-09-09: + +- Fixed BUG_LOOPRET2 and related bugs. The 'exit' and 'return' commands without + an argument now correctly default to passing down the exit status of the + last-run command. Tests like the following, in which the last-run command is + 'false', now correctly output 1 instead of 0: + fn() { return || true; }; false; fn; echo "$?" + fn() { while return; do true; done; }; false; fn; echo "$?" + fn() { for i in 1; do return; done; }; false; fn; echo "$?" + fn() { case 1 in 1) return ;; esac; }; false; fn; echo "$?" + fn() { { return; } 2>&1; }; false; fn; echo "$?" + 2020-09-05: - Fixed erroneous syntax errors in parameter expansions such as ${var:-wor)d} diff --git a/TODO b/TODO index 8ffc2c499..df46db562 100644 --- a/TODO +++ b/TODO @@ -49,9 +49,3 @@ https://github.com/modernish/modernish/tree/0.16/lib/modernish/cap/ parameter expansion inserts any IFS separator characters, those characters are erroneously interpreted as wildcards when quoted "$*" is used as the glob pattern. - -- BUG_LOOPRET2: If a 'return' command is given without a status argument - within the set of conditional commands in a 'while' or 'until' loop (i.e., - between 'while'/'until' and 'do'), the exit status passed down from the - previous command is ignored and the function returns with status 0 - instead. diff --git a/src/cmd/ksh93/bltins/cflow.c b/src/cmd/ksh93/bltins/cflow.c index 9396de789..34a3e0f6e 100644 --- a/src/cmd/ksh93/bltins/cflow.c +++ b/src/cmd/ksh93/bltins/cflow.c @@ -64,7 +64,7 @@ done: errormsg(SH_DICT,ERROR_usage(2),"%s",optusage((char*)0)); pp->mode = (**argv=='e'?SH_JMPEXIT:SH_JMPFUN); argv += opt_info.index; - n = (((arg= *argv)?(int)strtol(arg, (char**)0, 10):shp->oldexit)); + n = (arg = *argv) ? (int)strtol(arg, (char**)0, 10) : shp->savexit; if(n<0 || n==256 || n > SH_EXITMASK+shp->gd->sigmax+1) n &= ((unsigned int)n)&SH_EXITMASK; /* return outside of function, dotscript and profile is exit */ diff --git a/src/cmd/ksh93/include/defs.h b/src/cmd/ksh93/include/defs.h index df5cdfbf6..59a4b28ca 100644 --- a/src/cmd/ksh93/include/defs.h +++ b/src/cmd/ksh93/include/defs.h @@ -155,7 +155,6 @@ struct shared Sfio_t *heredocs; /* current here-doc temp file */ \ Sfio_t *funlog; /* for logging function definitions */ \ int **fdptrs; /* pointer to file numbers */ \ - int savexit; \ char *lastarg; \ char *lastpath; /* last alsolute path found */ \ int path_err; /* last error on path search */ \ @@ -181,7 +180,6 @@ struct shared char *prefix; /* prefix for compound assignment */ \ sigjmp_buf *jmplist; /* longjmp return stack */ \ char *fifo; /* fifo name for process sub */ \ - int oldexit; \ pid_t bckpid; /* background process id */ \ pid_t cpid; \ pid_t spid; /* subshell process id */ \ diff --git a/src/cmd/ksh93/include/shell.h b/src/cmd/ksh93/include/shell.h index 2a3c5acdc..77d908dd0 100644 --- a/src/cmd/ksh93/include/shell.h +++ b/src/cmd/ksh93/include/shell.h @@ -140,7 +140,8 @@ struct Shell_s Dt_t *bltin_tree; /* for builtin commands */ Shscope_t *topscope; /* pointer to top-level scope */ int inlineno; /* line number of current input file */ - int exitval; /* most recent exit value */ + int exitval; /* exit status of the command currently being run */ + int savexit; /* $? == exit status of the last command executed */ unsigned char trapnote; /* set when trap/signal is pending */ char shcomp; /* set when running shcomp */ unsigned int subshell; /* set for virtual subshell */ diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index e06fe1525..f24a1351f 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -17,4 +17,4 @@ * David Korn * * * ***********************************************************************/ -#define SH_RELEASE "93u+m 2020-09-05" +#define SH_RELEASE "93u+m 2020-09-09" diff --git a/src/cmd/ksh93/sh/fault.c b/src/cmd/ksh93/sh/fault.c index 5d138a23b..6af0b003a 100644 --- a/src/cmd/ksh93/sh/fault.c +++ b/src/cmd/ksh93/sh/fault.c @@ -614,7 +614,6 @@ void sh_done(void *ptr, register int sig) if(t=shp->st.trapcom[0]) { shp->st.trapcom[0]=0; /*should free but not long */ - shp->oldexit = savxit; sh_trap(t,0); savxit = shp->exitval; } diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index 504afb5ca..b74b00430 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -610,7 +610,6 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) /* trap on EXIT not handled by child */ char *trap=shp->st.trapcom[0]; shp->st.trapcom[0] = 0; /* prevent recursion */ - shp->oldexit = shp->exitval; sh_trap(trap,0); free(trap); } diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 6c620badd..122a1db8e 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -972,8 +972,6 @@ int sh_exec(register const Shnode_t *t, int flags) if(was_monitor&flags) sh_onstate(SH_MONITOR); type = t->tre.tretyp; - if(!shp->intrap) - shp->oldexit=shp->exitval; shp->exitval=0; shp->lastsig = 0; shp->lastpath = 0; diff --git a/src/cmd/ksh93/tests/basic.sh b/src/cmd/ksh93/tests/basic.sh index 455efcfe2..71693b0c1 100755 --- a/src/cmd/ksh93/tests/basic.sh +++ b/src/cmd/ksh93/tests/basic.sh @@ -428,6 +428,88 @@ if env x-a=y >/dev/null 2>&1 then [[ $(env 'x-a=y' $SHELL -c 'env | grep x-a') == *x-a=y* ]] || err_exit 'invalid environment variables not preserved' fi +foo() { return; } +false +foo && err_exit "'return' within function does not preserve exit status" +false +foo & wait "$!" && err_exit "'return' within function does not preserve exit status (bg job)" + +foo() { false; return || :; } +foo && err_exit "'return ||' does not preserve exit status" + +foo() { false; return && :; } +foo && err_exit "'return &&' does not preserve exit status" + +foo() { false; while return; do true; done; } +foo && err_exit "'while return' does not preserve exit status" + +foo() { false; while return; do true; done 2>&1; } +foo && err_exit "'while return' with redirection does not preserve exit status" + +foo() { false; until return; do true; done; } +foo && err_exit "'until return' does not preserve exit status" + +foo() { false; until return; do true; done 2>&1; } +foo && err_exit "'until return' with redirection does not preserve exit status" + +foo() { false; for i in 1; do return; done; } +foo && err_exit "'return' within 'for' does not preserve exit status" + +foo() { false; for i in 1; do return; done 2>&1; } +foo && err_exit "'return' within 'for' with redirection does not preserve exit status" + +foo() { false; { return; } 2>&1; } +foo && err_exit "'return' within { block; } with redirection does not preserve exit status" + +foo() ( exit ) +false +foo && err_exit "'exit' within function does not preserve exit status" +false +foo & wait "$!" && err_exit "'exit' within function does not preserve exit status (bg job)" + +foo() ( false; exit || : ) +foo && err_exit "'exit ||' does not preserve exit status" + +foo() ( false; exit && : ) +foo && err_exit "'exit &&' does not preserve exit status" + +foo() ( false; while exit; do true; done ) +foo && err_exit "'while exit' does not preserve exit status" + +foo() ( false; while exit; do true; done 2>&1 ) +foo && err_exit "'while exit' with redirection does not preserve exit status" + +foo() ( false; until exit; do true; done ) +foo && err_exit "'until exit' does not preserve exit status" + +foo() ( false; until exit; do true; done 2>&1 ) +foo && err_exit "'until exit' with redirection does not preserve exit status" + +foo() ( false; for i in 1; do exit; done ) +foo && err_exit "'exit' within 'for' does not preserve exit status" + +foo() ( false; for i in 1; do exit; done 2>&1 ) +foo && err_exit "'exit' within 'for' with redirection does not preserve exit status" + +foo() ( false; { exit; } 2>&1 ) +foo && err_exit "'exit' within { block; } with redirection does not preserve exit status" + +set -- +false +for i in "$@"; do :; done || err_exit 'empty loop does not reset exit status ("$@")' +false +for i do :; done || err_exit 'empty loop does not reset exit status ("$@" shorthand)' +false +for i in; do :; done || err_exit "empty loop does not reset exit status (empty 'in' list)" + +false +case 1 in 1) ;; esac || err_exit "empty case does not reset exit status" +false +case 1 in 2) echo whoa ;; esac || err_exit "non-matching case does not reset exit status" + +false +2>&1 || err_exit "lone redirection does not reset exit status" + float s=SECONDS for i in .1 .2 do print $i