Ingo,
Your questions were very insightful. I ran several tests, and I
have understood that a number of things that I said earlier in this
thread are incorrect. If/when you have time, can you please review
these? That would be tremendously helpful.
1. tcgetattr itself is a call to ioctl [
src/lib/libc/termios/tcgetattr.c and ttioctl in
/src/blob/master/sys/kern/tty.c ].
2. Contrary to what I had claimed earlier, ioctl [ and hence tcgetattr
] do not ever fail with errno set to EINTR.
3. The two possible errno values set by tcgetattr upon failure are
EBADF and ENOTTY.
4. tcgetattr(STDIN_FILENO,...) cannot fail with EBADF in normal
circumstances. I was only able to force and EBADF by closing stdin
using gdb [ p close(0) ].
5. That only leaves us with ENOTTY, which can happen, if for example,
the program is exec-d by a daemon or if its stdin is a regular file.
Thank you.
Soumendra
On 8/4/20, Soumendra Ganguly <[email protected]> wrote:
> 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.
>>>>>
>>>>
>>>
>>
>
