Actually... I will perform a test now to see if this is correct... but I think tcgetattr and ioctl can still fail [ in any case ] with ENOTTY if we detach the session from the terminal right before the tcgetatttr/ioctl call. In that case, should we still continue with !isatty or should we call err [ my patch does the latter ]? Anyway, this justifies checking for ioctl error [ note that the sigwinch handler is also checking for ioctl error ].
On 8/4/20, Soumendra Ganguly <[email protected]> wrote: > Actually, I realized that even in the current version of the code, > tcgetattr and ioctl cannot fail with ENOTTY because they are wrapped > by the isatty(0); therefore [ correct me if I am wrong ], the only way > > (tcgetattr(STDIN_FILENO, &tt) == 0 && > ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == 0) > > is false is if tcgetattr fails with EINTR [ ioctl should not fail at all ]. > > Summing up, the only difference in that bit of the original code and > my patch is that my patch errs out on tcgetattr fail with EINTR while > the current version keeps running with !istty. Not sure why someone > would want to do that [ maybe if the EINTR was due to a STOP signal > and one would want to keep script running with !istty after a CONT ]. > All of this seems strange to me anyway. I would want my program to err > on tcgetattr EINTR. It would help if I got someone else's opinion. > Thank you. > > > On 8/4/20, Soumendra Ganguly <[email protected]> wrote: >> Signed-off-by: Soumendra Ganguly <[email protected]> >> --- >> --- src/usr.bin/script/script.c 2020-08-04 02:41:13.226129480 -0500 >> +++ script.c 2020-08-04 14:13:00.240012281 -0500 >> @@ -132,7 +132,7 @@ >> if ((fscript = fopen(fname, aflg ? "a" : "w")) == NULL) >> err(1, "%s", fname); >> >> - if (isatty(0)) { >> + if (isatty(STDIN_FILENO)) { >> if (tcgetattr(STDIN_FILENO, &tt) == 0 && >> ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == 0) >> istty = 1; >> @@ -147,13 +147,13 @@ >> cfmakeraw(&rtt); >> rtt.c_lflag &= ~ECHO; >> (void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt); >> - } >> >> - bzero(&sa, sizeof sa); >> - sigemptyset(&sa.sa_mask); >> - sa.sa_handler = handlesigwinch; >> - sa.sa_flags = SA_RESTART; >> - (void)sigaction(SIGWINCH, &sa, NULL); >> + bzero(&sa, sizeof sa); >> + sigemptyset(&sa.sa_mask); >> + sa.sa_handler = handlesigwinch; >> + sa.sa_flags = SA_RESTART; >> + (void)sigaction(SIGWINCH, &sa, NULL); >> + } >> >> child = fork(); >> if (child == -1) { >> >> >> On 8/4/20, Soumendra Ganguly <[email protected]> wrote: >>> 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. >>> >> >
