Hello. On 8/4/20, Ingo Schwarze <[email protected]> wrote: > Hi, > > Soumendra Ganguly wrote on Tue, Aug 04, 2020 at 01:32:33AM -0500: > >> Program: script(1) >> >> Issue description: Currently, script(1) passes &tt and &win to openpty >> even when isatty(0) == 0. Also, the SIGWINCH handler does not check if >> stdin is a tty. >> >> The patch follows [ also attached ]. > > Don't do that. Just inline is enough, avoid attachments if you can. >
Henceforth, I will avoid attachments. > >> Signed-off-by: Soumendra Ganguly <[email protected]> >> --- >> --- src/usr.bin/script/script.c 2020-08-03 21:33:44.636187087 -0500 >> +++ script.c 2020-08-04 01:03:15.925066340 -0500 >> @@ -74,18 +74,18 @@ >> #include <util.h> >> #include <err.h> >> >> -FILE *fscript; >> -int master, slave; >> +static FILE *fscript; >> +static int master, slave; > > Please do not mix style changes into bugfix patches; > they unnecessarily make review harder. > Do you want me to revert these changes? >> volatile sig_atomic_t child; >> -pid_t subchild; >> -char *fname; >> +static pid_t subchild; >> +static char *fname; > > dto. > >> volatile sig_atomic_t dead; >> volatile sig_atomic_t sigdeadstatus; >> volatile sig_atomic_t flush; >> >> -struct termios tt; >> -int istty; >> +static int istty; >> +static struct termios tt; > > dto. > >> __dead void done(int); >> void dooutput(void); >> @@ -132,13 +132,18 @@ >> if ((fscript = fopen(fname, aflg ? "a" : "w")) == NULL) >> err(1, "%s", fname); >> >> - if (isatty(0)) { >> - if (tcgetattr(STDIN_FILENO, &tt) == 0 && >> - ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == 0) >> - istty = 1; >> + istty = isatty(STDIN_FILENO); >> + if (istty) { > > This does not look correct. The existing code only sets istty > if isatty(3), tcgetattr(3), and TIOCGWINSZ all succeed. > With your change, it is set even if tcgetattr(3) or TIOCGWINSZ > fail. > See next response. >> + if (tcgetattr(STDIN_FILENO, &tt) == -1) >> + err(1, "tcgetattr"); >> + if (ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == -1) >> + err(1, "ioctl"); > > This doesn't look correct either. The existing code simply runs > without istty if either of these fails. > Why would you want to error out instead? > I checked the OpenBSD manpages for tcgetattr and ioctl. This is interesting. In the existing code, let us consider tcgetattr == 0 && ioctl == 0. tcgeattr can fail with ENOTTY and EINTR; ioctl can fail with ENOTTY. I am not sure, but can ioctl fail with ENOTTY at all if tcgetattr has already failed with ENOTTY? The ioctl manpage does give two different possibilities for ENOTTY whereas tcgetattr gives only one. In my code, I believe tcgetattr and ioctl will not fail with ENOTTY since istty is true already. Therefore, the check for ioctl error is redundant. However, what if tcgetattr fails with EINTR? Should we still continue? In fact, just FYI, I just cross checked that while FreeBSD is unnecessarily checking for ioctl error, the Linux version actually does if(tcgetattr == -1) then bail but does not bother to check for ioctl == -1. This seems to be correct. [ https://github.com/karelzak/util-linux/blob/master/lib/pty-session.c ] >> + if (openpty(&master, &slave, NULL, &tt, &win) == -1) >> + err(1, "openpty"); >> + } else { >> + if (openpty(&master, &slave, NULL, NULL, NULL) == -1) >> + err(1, "openpty"); >> >> - if (openpty(&master, &slave, NULL, &tt, &win) == -1) >> - err(1, "openpty"); > > This is probably correct, there is no point in calling > tcsetattr(..., &tt) and ioctl(..., &winp) with tt and winp > full of zeros and without a usable terminal. > > Not sure it does actual harm though - is there a specific problem > you are trying to fix? > No. However, if you think that this need not be done, I would like to know. Honestly speaking, I have actually seen it being done on Linux and FreeBSD [ calling openpty separately if istty == 0 ]. If no harm is done by passing &tt and &win to openpty with stdin not a tty, then I will learn something new today (yay!) Also, I will check (later) if this behavior also works on Linux, FreeBSD, etc. >> (void)printf("Script started, output file is %s\n", fname); >> if (istty) { >> @@ -227,7 +232,7 @@ >> struct winsize win; >> pid_t pgrp; >> >> - if (ioctl(STDIN_FILENO, TIOCGWINSZ, &win) != -1) { >> + if (istty && ioctl(STDIN_FILENO, TIOCGWINSZ, &win) != -1) { >> ioctl(slave, TIOCSWINSZ, &win); >> if (ioctl(slave, TIOCGPGRP, &pgrp) != -1) >> killpg(pgrp, SIGWINCH); > > This looks potentially correct, too. Though strictly speaking, > it would require making the istty variable volatile sig_atomic_t. > > Then again, in this case, the signal handler does nothing, so why > even call it? If you want to change this, wouldn't it make more > sense to not install this signal handler in the first place > if istty is not set? > I realized the fact that it should be sig_atomic_t. In fact, I also realized that it is better to not register the handler at all if istty == 0. That is what my updated diff attachment was doing. Why should we unnecessarily register a handler if not necessary and make meaningless ioctl calls? I will send a new patch that only makes this one change for now. > Either way, this patch is clearly not ready for commit. > Can any developer with experience in terminal handling > comment on whether > > * passing empty structures to openpty() > * calling the ioctl()s in handlesigwinch() > > are really problematic if !istty, and worth fixing? > > Yours, > Ingo > Thank you.
