Re: bash passes changed termios to backgrounded process(es) groups?

[Chet Ramey]

On 8/28/24 4:12 PM, Steffen Nurpmeso wrote:
> Chet Ramey wrote in
>   <3ca901aa-5c5e-4be3-9a71-157d7101f892@case.edu>:
>   |On 8/27/24 7:46 PM, Steffen Nurpmeso wrote:
>   |> Hello.
>   |>
>   |> I got a bug report for my mailer which stated
>   |>
>   |>     $ ( echo blah | Mail root ) &
>   |>    [1] 2754649
>   |>     $ ^M^M^M^M^C^C
>   |>
>   |>    [1]+  Stopped                 ( echo blah | Mail root )
>   |>     $ fg
>   |>    ( echo blah | Mail root )
>   |>     $
>   |>
>   |> I turns out i answered him now
>   |>
>   |>    The thing is that if i apply the patch (this to [master])
>   |>
>   |>      diff --git a/src/mx/termios.c b/src/mx/termios.c
>   |>      index 733974ebce..08dd045226 100644
>   |>      --- a/src/mx/termios.c
>   |>      +++ b/src/mx/termios.c
>   |>      @@ -152,6 +152,8 @@ a_termios_norm_query(void){
>   |>                &a_termios_g.tiosg_normal->tiose_state) == 0);
>   |>          /* XXX always set ECHO and ICANON in our "normal" canonical \
>   |>          state */
>   |>          a_termios_g.tiosg_normal->tiose_state.c_lflag |= ECHO | ICANON;
>   |>      +   a_termios_g.tiosg_normal->tiose_state.c_iflag |= ICRNL;
>   |>      +
>   |>          /*NYD2_OU;*/
>   |>          return rv;
>   |>}
>   |>
>   |>    then everything is working as should in an otherwise unchanged MUA.
>   |>    It seems readline does this:
>   |>
>   |>      ./lib/sh/shtty.c:  ttp->c_iflag |= ICRNL;       /* make sure \
>   |>      we get CR->NL on input */
>   |>      ./lib/readline/rltty.c:  tiop->c_iflag &= ~(ICRNL | INLCR);
>   |>
>   |> ..and it seems that if bash starts a normal process then ICRNL is
>   |> set, but if it starts a (process)& or only process&, then not!
>   |> (I was about to send this to bug-readline first.)
>   |
>   |Under no circumstances should a background process attempt to fetch or
>   |modify terminal attributes. Why isn't your Mail process checking for that?
>
> How could it do so?
> (getpid()==tcgetpgrp() or what the function name is is the only
> idea i have, but note it is false for (EXE), too.  *Big problem*!)

It's not a big problem. You're in the background if your process group is
not equal to the terminal's process group. A simple test suffices:

tty = fileno (stderr);  /* pick your file descriptor */
pgrp = getpgid(0);
tpgrp = tcgetpgrp (tty);
running_in_background = (pgrp != tpgrp);

>   |Chances are excellent that it will fetch the terminal attributes as they've
>   |been set by readline when it goes to read the next command, then modify (?)
>   |them out from underneath readline.
>
> Yes, it is not right what readline does there.

No, readline does the right thing. It fetches and sets the terminal
attributes while it has control, and it relies on the caller to make
sure it's in the right process group (that is, it doesn't try to set
the process group itself). It tries to set the window size immediately --
before fetching the terminal attributes -- so it gets a SIGTTOU if it's
in the background, even though that can be defeated by `stty -tostop'.
It restores the terminal attributes before it returns a value to its
caller.

> And for me, two things.  For one we ensure we give to child
> processes, and to restore whenever we go, the original settings as
> we inherit them. 

If you're in the background, you have no idea what the foreground process
group has done to the terminal settings before you query them. You only
fetch and modify the terminal settings if you're in the foreground.

> We requery what means "original" whenever we get
> back from a suspension, because user etc may apply changes, and we
> should reflect (i have seen that, or even got a bug report).

Sure. But if you are restarted (and get your SIGCONT) due to the equivalent
of a `bg', you still have to check whether you're in the foreground.

> And second, if there isatty(3) somewhere, we do termios stuff;
> i agree this is bad, especially so since we look STDIN and STDOUT,
> not STDIN and STDERR, as POSIX says a shell should do (i think
> even dash that was notoriously "wrong" with that will have fixed
> this with the next release). 

You're not a shell, but looking at stderr is probably the right thing.

> "Interactive" we are only if both
> are isatty(3), and maybe i will change all that because we are no
> shell and never will be, to only be interactive and do termios
> stuff if all relevant FDs are terminals.

Also probably the right thing.

> (Anyhow, in the example we start a child process, and since STDERR
> is passed "as is", we try to restore termions settings (which,
> btw, have never been changed in the above snippet, but that
> aside), *if* i remember the context correctly.  Our termios code
> is a stack, and it is terribly complicated.)

Still the wrong thing to do if you're in the background. It's an
inherent race condition.

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    chet@case.edu    http://tiswww.cwru.edu/~chet/

OpenPGP_signature.asc
Description: OpenPGP digital signature