Correction: I should have said TSTP and not STOP. EINTR happens due to interruption by signal handler. STOP cannot be handled. I performed some experiments. I propose that we end the thread here [ discard everything ]. I will create a new one, and I will CC you there.
Thank you. On 8/4/20, Soumendra Ganguly <[email protected]> wrote: > 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. >>>> >>> >> >
