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