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.
>>
>
