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. > 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. > 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. > + 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? > + 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? > (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? 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
