From d8eba9d11286f8d2167ab6b3077326c8388f579f Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Fri, 12 Jun 2020 06:57:57 +0200 Subject: [PATCH] Remove 'login' and 'newgrp' builtins: not sane default behaviour This commit removes the undocumented 'login' and 'newgrp' builtin commands. They already stopped blocking shell functions by that name by changing from special to regular builtins in 04b91718 (a change I forgot to mention in that commit message), but there is another obnoxious aspect to these: being glorified hooks into 'exec', they replaced your shell session with the external commands by the same name. This makes argument and error checking impossible, so if you made so much as a typo, you would be immediately logged out. Even if that behaviour is wanted by a few, having it as the default is user-hostile enough to be called a bug. It also violates the POSIX definition of the 'newgrp' utility which explicitly says that it "shall create a new shell execution environment", not replace the existing one. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/newgrp.html Users who do want this behaviour can easily restore it by setting: alias login='exec login' alias newgrp='exec newgrp' src/cmd/ksh93/bltins/misc.c: - As there is no more 'login' builtin, combine b_exec() and B_login() functions, which allows eliminating a few variables. Note that most of 'exec' was actually implemented in B_login()! src/cmd/ksh93/data/builtins.c: - Remove "login" and "newgrp" table entries. src/cmd/ksh93/include/builtins.h: - Remove SYSLOGIN parser ID. As this was the first, all the others needed renumbering. src/cmd/ksh93/sh/xec.c: - Remove SYSLOGIN parser check that made 'login' and 'newgrp' act like 'exec' and replace the shell. --- NEWS | 8 ++++ src/cmd/ksh93/bltins/misc.c | 68 ++++++++++++++------------------ src/cmd/ksh93/data/builtins.c | 4 -- src/cmd/ksh93/include/builtins.h | 33 ++++++++-------- src/cmd/ksh93/sh/xec.c | 6 +-- 5 files changed, 56 insertions(+), 63 deletions(-) diff --git a/NEWS b/NEWS index ea6fb0fee..62e5df85b 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,14 @@ Any uppercase BUG_* names are modernish shell bug IDs. * users no longer accidentally get logged out of their shells if they type something intuitive but wrong, like 'redirect ls >file'. +- The undocumented 'login' and 'newgrp' builtin commands have been removed. + These replaced your shell session with the external commands by the same + name, as in 'exec'. If an error occurred (e.g. due to a typo), you would + end up immediately logged out. + If you do want this behaviour, you can restore it by setting: + alias login='exec login' + alias newgrp='exec newgrp' + 2020-06-10: - The 'hash' utility is now a regular builtin instead of an alias to diff --git a/src/cmd/ksh93/bltins/misc.c b/src/cmd/ksh93/bltins/misc.c index 88b36cfb8..6bdbda33b 100644 --- a/src/cmd/ksh93/bltins/misc.c +++ b/src/cmd/ksh93/bltins/misc.c @@ -60,6 +60,15 @@ struct login char *arg0; }; +/* + * Handler function for nv_scan() that unsets a variable's export attribute + */ +static void noexport(register Namval_t* np, void *data) +{ + NOT_USED(data); + nv_offattr(np,NV_EXPORT); +} + /* * 'exec' special builtin and 'redirect' builtin */ @@ -67,6 +76,9 @@ int b_exec(int argc,char *argv[], Shbltin_t *context) { struct login logdata; register int n; + struct checkpt *pp; + const char *pname; + logdata.clear = 0; logdata.arg0 = 0; logdata.sh = context->shp; @@ -92,74 +104,54 @@ int b_exec(int argc,char *argv[], Shbltin_t *context) if(*argv[0]=='r' && argv[opt_info.index]) /* 'redirect' supports no args */ errormsg(SH_DICT,ERROR_exit(2),e_badsyntax); argv += opt_info.index; - if(*argv) - B_login(0,argv,(Shbltin_t*)&logdata); - return(0); -} + if(!*argv) + return(0); -static void noexport(register Namval_t* np, void *data) -{ - NOT_USED(data); - nv_offattr(np,NV_EXPORT); -} - -int B_login(int argc,char *argv[],Shbltin_t *context) -{ - struct checkpt *pp; - register struct login *logp=0; - register Shell_t *shp; - const char *pname; - if(argc) - shp = context->shp; - else - { - logp = (struct login*)context; - shp = logp->sh; - } - pp = (struct checkpt*)shp->jmplist; + /* from here on, it's 'exec' with args, so we're replacing the shell */ if(sh_isoption(SH_RESTRICTED)) errormsg(SH_DICT,ERROR_exit(1),e_restricted,argv[0]); else { - register struct argnod *arg=shp->envlist; + register struct argnod *arg=logdata.sh->envlist; register Namval_t* np; register char *cp; - if(shp->subshell && !shp->subshare) + if(logdata.sh->subshell && !logdata.sh->subshare) sh_subfork(); - if(logp && logp->clear) + if(logdata.clear) { #ifdef _ENV_H - env_close(shp->env); - shp->env = env_open((char**)0,3); + env_close(logdata.sh->env); + logdata.sh->env = env_open((char**)0,3); #else - nv_scan(shp->var_tree,noexport,0,NV_EXPORT,NV_EXPORT); + nv_scan(logdata.sh->var_tree,noexport,0,NV_EXPORT,NV_EXPORT); #endif } while(arg) { if((cp=strchr(arg->argval,'=')) && - (*cp=0,np=nv_search(arg->argval,shp->var_tree,0))) + (*cp=0,np=nv_search(arg->argval,logdata.sh->var_tree,0))) { nv_onattr(np,NV_EXPORT); - sh_envput(shp->env,np); + sh_envput(logdata.sh->env,np); } if(cp) *cp = '='; arg=arg->argnxt.ap; } pname = argv[0]; - if(logp && logp->arg0) - argv[0] = logp->arg0; + if(logdata.arg0) + argv[0] = logdata.arg0; #ifdef JOBS - if(job_close(shp) < 0) + if(job_close(logdata.sh) < 0) return(1); #endif /* JOBS */ /* force bad exec to terminate shell */ + pp = (struct checkpt*)logdata.sh->jmplist; pp->mode = SH_JMPEXIT; sh_sigreset(2); - sh_freeup(shp); - path_exec(shp,pname,argv,NIL(struct argnod*)); - sh_done(shp,0); + sh_freeup(logdata.sh); + path_exec(logdata.sh,pname,argv,NIL(struct argnod*)); + sh_done(logdata.sh,0); } return(1); } diff --git a/src/cmd/ksh93/data/builtins.c b/src/cmd/ksh93/data/builtins.c index 2b85a17ac..70e7698d5 100644 --- a/src/cmd/ksh93/data/builtins.c +++ b/src/cmd/ksh93/data/builtins.c @@ -58,7 +58,6 @@ */ const struct shtable3 shtab_builtins[] = { - "login", NV_BLTIN|BLT_ENV, Bltin(login), "exec", NV_BLTIN|BLT_ENV|BLT_SPC, bltin(exec), "redirect", NV_BLTIN|BLT_ENV, bltin(exec), "set", NV_BLTIN|BLT_ENV|BLT_SPC, bltin(set), @@ -81,9 +80,6 @@ const struct shtable3 shtab_builtins[] = /* * Builtins without offset macros in include/builtins.h follow. */ -#if _bin_newgrp || _usr_bin_newgrp - "newgrp", NV_BLTIN|BLT_ENV, Bltin(login), -#endif /* _bin_newgrp || _usr_bin_newgrp */ "alias", NV_BLTIN|BLT_ENV, bltin(alias), "hash", NV_BLTIN|BLT_ENV, bltin(alias), "enum", NV_BLTIN|BLT_ENV|BLT_DCL, bltin(enum), diff --git a/src/cmd/ksh93/include/builtins.h b/src/cmd/ksh93/include/builtins.h index f0d654852..b17e644aa 100644 --- a/src/cmd/ksh93/include/builtins.h +++ b/src/cmd/ksh93/include/builtins.h @@ -32,25 +32,24 @@ * IMPORTANT: The offsets on these macros must be synchronous * with the order of shtab_builtins[] in data/builtins.c! */ -#define SYSLOGIN (shgd->bltin_cmds) /* login */ -#define SYSEXEC (shgd->bltin_cmds+1) /* exec */ -#define SYSREDIR (shgd->bltin_cmds+2) /* redirect */ -#define SYSSET (shgd->bltin_cmds+3) /* set */ +#define SYSEXEC (shgd->bltin_cmds) /* exec */ +#define SYSREDIR (shgd->bltin_cmds+1) /* redirect */ +#define SYSSET (shgd->bltin_cmds+2) /* set */ /* : */ -#define SYSTRUE (shgd->bltin_cmds+5) /* true */ -#define SYSCOMMAND (shgd->bltin_cmds+6) /* command */ -#define SYSCD (shgd->bltin_cmds+7) /* cd */ -#define SYSBREAK (shgd->bltin_cmds+8) /* break */ -#define SYSCONT (shgd->bltin_cmds+9) /* continue */ -#define SYSTYPESET (shgd->bltin_cmds+10) /* typeset */ -#define SYSTEST (shgd->bltin_cmds+11) /* test */ -#define SYSBRACKET (shgd->bltin_cmds+12) /* [ */ -#define SYSLET (shgd->bltin_cmds+13) /* let */ -#define SYSEXPORT (shgd->bltin_cmds+14) /* export */ -#define SYSDOT (shgd->bltin_cmds+15) /* . */ -#define SYSRETURN (shgd->bltin_cmds+16) /* return */ +#define SYSTRUE (shgd->bltin_cmds+4) /* true */ +#define SYSCOMMAND (shgd->bltin_cmds+5) /* command */ +#define SYSCD (shgd->bltin_cmds+6) /* cd */ +#define SYSBREAK (shgd->bltin_cmds+7) /* break */ +#define SYSCONT (shgd->bltin_cmds+8) /* continue */ +#define SYSTYPESET (shgd->bltin_cmds+9) /* typeset */ +#define SYSTEST (shgd->bltin_cmds+10) /* test */ +#define SYSBRACKET (shgd->bltin_cmds+11) /* [ */ +#define SYSLET (shgd->bltin_cmds+12) /* let */ +#define SYSEXPORT (shgd->bltin_cmds+13) /* export */ +#define SYSDOT (shgd->bltin_cmds+14) /* . */ +#define SYSRETURN (shgd->bltin_cmds+15) /* return */ #if SHOPT_BASH -# define SYSLOCAL (shgd->bltin_cmds+17) /* local */ +# define SYSLOCAL (shgd->bltin_cmds+16) /* local */ #else # define SYSLOCAL 0 #endif diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 215f505ce..7e9f758cb 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -1332,10 +1332,8 @@ int sh_exec(register const Shnode_t *t, int flags) if(io) { struct openlist *item; - if(np==SYSLOGIN) - type=1; - else if(np==SYSEXEC || np==SYSREDIR) /* 'exec' or 'redirect' */ - type=1+!com[1]; /* redirections persist if no args */ + if(np==SYSEXEC || np==SYSREDIR) /* 'exec' or 'redirect' */ + type=1+!com[1]; /* redirections persist if no args */ else type = (execflg && !shp->subshell && !shp->st.trapcom[0]); shp->redir0 = 1;