From 09a8a279f24659dde8dce607e52cddb810b97a5c Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sun, 7 Nov 2021 10:14:54 +0000 Subject: [PATCH] Fix bug on closed stdout; improve BUG_PUTIOERR fix (re: 93e15a30) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stéphane Chazelas reported: > As noted in this austin-group-l discussion[*] (relevant to this > issue): > > $ ksh93u+m -c 'pwd; echo "$?" >&2; echo test; echo "$?" >&2' >&- > 0 > 1 > /home/chazelas > > when stdout is closed, pwd does claim it succeeds (by returning a > 0 exit status), while echo doesn't (not really relevant to the > problem here, only to show it doesn't affect all builtins), and > the output that pwd failed to write earlier ends up being written > on stderr here instead of stdout upon exit (presumably) because > of that >&2 redirection. > > strace shows ksh93 attempting write(1, "/home/chazelas\n", 15) 6 > times (1, the last one, successful). > > It gets even weirder when redirecting to a file: > > $ ksh93u+m -c 'pwd; echo "$?" >&2; echo test; echo "$?" > file' >&- > 0 > $ cat file > 1 > 1 > ome/chazelas In my testing, the problem does not occur when closing stdout at the start of the -c script itself (using redirect >&- or exec >&-); it only occurs if stdout was closed before initialising the shell. That made me suspect that the problem had to do with an inconsistent file descriptor state in the shell. ksh uses internal sh_open() and sh_close() functions, among others, to maintain that state. src/cmd/ksh93/sh/main.c: sh_main(): - If the shell is initialised with stdin, stdout or stderr closed, then make the shell's file descriptor state tables reflect that fact by calling sh_close() for the closed file descriptors. This commit also improves the BUG_PUTIOERR fix from 93e15a30. Error checking after sfsync() is not sufficient. For instance, on FreeBSD, the following did not produce a non-zero exit status: ksh -c 'echo hi' >/dev/full even though this did: ksh -c 'echo hi >/dev/full' Reliable error checking requires not only checking the result of every SFIO command that writes output, but also synching the buffer at the end of the operation and checking the result of that. src/cmd/ksh93/bltins/print.c: - Make exitval variable global to allow functions called by b_print() to set a nonzero exit status. - Check the result of all SFIO output commands that write output. - b_print(): Always sfsync() at the end, except if the s (history) flag was given. This allows getting rid of the sfsync() call that required the workaround introduced in 846ad932. [*] https://www.mail-archive.com/austin-group-l@opengroup.org/msg08056.html Resolves: https://github.com/ksh93/ksh/issues/314 --- NEWS | 9 ++++++++ src/cmd/ksh93/bltins/print.c | 40 +++++++++++++++++---------------- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/main.c | 4 ++++ src/cmd/ksh93/tests/io.sh | 21 +++++++++++++++++ 5 files changed, 56 insertions(+), 20 deletions(-) diff --git a/NEWS b/NEWS index 37e6c276a..6d7e57740 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,15 @@ For full details, see the git log at: https://github.com/ksh93/ksh Any uppercase BUG_* names are modernish shell bug IDs. +2021-11-07: + +- Fixed a bug that could corrupt output if standard output is closed upon + initializing the shell. + +- Improved BUG_PUTIOERR fix (2020-05-14) with more error checking. On + systems with the "disk full" error testing device /dev/full, an + echo/print/printf to /dev/full now always yields a non-zero exit status. + 2021-09-13: - Fixed a bug introduced in 93u+ 2012-02-07 that caused the 'printf' builtin diff --git a/src/cmd/ksh93/bltins/print.c b/src/cmd/ksh93/bltins/print.c index b65b4493e..2d0356706 100644 --- a/src/cmd/ksh93/bltins/print.c +++ b/src/cmd/ksh93/bltins/print.c @@ -100,6 +100,7 @@ struct print }; static char* nullarg[] = { 0, 0 }; +static int exitval; #if !SHOPT_ECHOPRINT int B_echo(int argc, char *argv[],Shbltin_t *context) @@ -172,12 +173,13 @@ static int infof(Opt_t* op, Sfio_t* sp, const char* s, Optdisc_t* dp) int b_print(int argc, char *argv[], Shbltin_t *context) { register Sfio_t *outfile; - register int exitval=0,n, fd = 1; + register int n, fd = 1; register Shell_t *shp = context->shp; const char *options, *msg = e_file+4; char *format = 0; int sflag = 0, nflag=0, rflag=0, vflag=0; Optdisc_t disc; + exitval = 0; disc.version = OPT_VERSION; disc.infof = infof; opt_info.disc = &disc; @@ -335,15 +337,7 @@ skip2: sfprintf(outfile,"%!",&pdata); } while(*pdata.nextarg && pdata.nextarg!=argv); if(pdata.nextarg == nullarg && pdata.argsize>0) - sfwrite(outfile,stakptr(staktell()),pdata.argsize); - /* - * -f flag skips adding newline at the end of output, which causes issues - * with syncing history if -s and -f are used together. So don't sync now - * if sflag was given. History is synced later with hist_flush() function. - * https://github.com/att/ast/issues/425 - */ - if(!sflag && sffileno(outfile)!=sffileno(sfstderr)) - if (sfsync(outfile) < 0) + if(sfwrite(outfile,stakptr(staktell()),pdata.argsize) < 0) exitval = 1; sfpool(sfstderr,pool,SF_WRITE); if (pdata.err) @@ -353,9 +347,11 @@ skip2: { while(*argv) { - fmtbase64(outfile,*argv++,vflag=='C'); + if(fmtbase64(outfile,*argv++,vflag=='C') < 0) + exitval = 1; if(!nflag) - sfputc(outfile,'\n'); + if(sfputc(outfile,'\n') < 0) + exitval = 1; } } else @@ -367,16 +363,18 @@ skip2: exitval = 1; } else if(sh_echolist(shp,outfile,rflag,argv) && !nflag) - sfputc(outfile,'\n'); + if(sfputc(outfile,'\n') < 0) + exitval = 1; } if(sflag) { hist_flush(shp->gd->hist_ptr); sh_offstate(SH_HISTORY); } - else if(n&SF_SHARE) + else { - sfset(outfile,SF_SHARE|SF_PUBLIC,1); + if(n&SF_SHARE) + sfset(outfile,SF_SHARE|SF_PUBLIC,1); if (sfsync(outfile) < 0) exitval = 1; } @@ -401,12 +399,15 @@ int sh_echolist(Shell_t *shp,Sfio_t *outfile, int raw, char *argv[]) if(!raw && (n=fmtvecho(cp,&pdata))>=0) { if(n) - sfwrite(outfile,stakptr(staktell()),n); + if(sfwrite(outfile,stakptr(staktell()),n) < 0) + exitval = 1; } else - sfputr(outfile,cp,-1); + if(sfputr(outfile,cp,-1) < 0) + exitval = 1; if(*argv) - sfputc(outfile,' '); + if(sfputc(outfile,' ') < 0) + exitval = 1; sh_sigcheck(shp); } return(!pdata.cescape); @@ -633,7 +634,8 @@ static ssize_t fmtbase64(Sfio_t *iop, char *string, int alt) else if(nv_isarray(np) && (ap=nv_arrayptr(np)) && array_elem(ap) && (ap->nelem&(ARRAY_UNDEF|ARRAY_SCAN))) { nv_outnode(np,iop,(alt?-1:0),0); - sfputc(iop,')'); + if(sfputc(iop,')') < 0) + exitval = 1; return(sftell(iop)); } else diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index d98ce4a3f..f211bc6d9 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -21,7 +21,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-beta.2" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2021-09-13" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2021-11-07" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c index dfa5fc257..342d3565e 100644 --- a/src/cmd/ksh93/sh/main.c +++ b/src/cmd/ksh93/sh/main.c @@ -344,6 +344,10 @@ int sh_main(int ac, char *av[], Shinit_f userinit) sh_accbegin(error_info.id); #endif /* SHOPT_ACCT */ } + /* If the shell is init'ed with std{in,out,err} closed, make the shell's FD state reflect that. */ + for(i=0; i<=2; i++) + if(fcntl(i,F_GETFD,NiL)==-1 && errno==EBADF) /* closed at OS level? */ + sh_close(i); /* update shell FD state */ } else { diff --git a/src/cmd/ksh93/tests/io.sh b/src/cmd/ksh93/tests/io.sh index 275214591..adda21c39 100755 --- a/src/cmd/ksh93/tests/io.sh +++ b/src/cmd/ksh93/tests/io.sh @@ -878,5 +878,26 @@ got=$(< dir1/dir2/x) [[ $got == "$exp" ]] || err_exit "symlink in conditional redirect wrong" \ "(expected $(printf %q "$exp"), got $(printf %q "$got"))" +# ====== +# ksh misbehaved when stdout is closed +# https://github.com/ksh93/ksh/issues/314 +"$SHELL" -c 'pwd; echo "$?" >&2; echo test; echo "$?" > file' >&- 2>stderr +exp='1' +[[ $(&- && err_exit "'$cmd' does not detect closed stdout (simple redirection)" + "$SHELL" -c "$cmd hi" >&- && err_exit "'$cmd' does not detect closed stdout (inherited FD)" +done +if [[ -c /dev/full ]] +then for cmd in echo print printf + do "$cmd" hi >/dev/full && err_exit "'$cmd' does not detect disk full (simple redirection)" + "$SHELL" -c "$cmd hi" >/dev/full && err_exit "'$cmd' does not detect disk full (inherited FD)" + done +fi + # ====== exit $((Errors<125?Errors:125))