Re: [PATCH] jobs: Block signals during tcsetpgrp

[Denys Vlasenko <vda.linux@xxxxxxxxxxxxxx>]

On Wed, Jan 6, 2021 at 10:17 PM Harald van Dijk <harald@xxxxxxxxxxx> wrote:
> On 06/01/2021 04:45, Herbert Xu wrote:
> > This patch implements the blocking of SIGTTOU (and everything else)
> > while we call tcsetpgrp.
> >
> > Reported-by: Steffen Nurpmeso <steffen@xxxxxxxxxx>
> > Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> >
> > diff --git a/src/jobs.c b/src/jobs.c
> > index 516786f..809f37c 100644
> > --- a/src/jobs.c
> > +++ b/src/jobs.c
> > @@ -1512,7 +1512,13 @@ showpipe(struct job *jp, struct output *out)
> >   STATIC void
> >   xtcsetpgrp(int fd, pid_t pgrp)
> >   {
> > -     if (tcsetpgrp(fd, pgrp))
> > +     int err;
> > +
> > +     sigblockall(NULL);
> > +     err = tcsetpgrp(fd, pgrp);
> > +     sigclearmask();
> > +
> > +     if (err)
> >               sh_error("Cannot set tty process group (%s)", strerror(errno));
> >   }
> >   #endif
>
> While this is a step in the right direction, Jilles has already replied
> with an explanation of why this is not enough: if the terminal is in
> TOSTOP mode, it's not just tcsetpgrp() that needs to be handled, it's
> any write as well that may occur while the shell is not in the
> foreground process group. While it may be working according to design
> for messages written when the shell is not supposed to be in the
> foreground process group, it is another story when the shell is both
> responsible for taking itself out of the foreground process group and
> for writing a message. This is made worse by the fact that there is no
> synchronisation with child processes on errors, so even forcibly
> restoring the foreground process group may not be enough: unfortunate
> scheduling may result in a child process immediately setting the
> foreground process group to the child process after the parent process
> attempted to restore it to itself. I have not yet seen a good solution
> for this.

How about not simply allowing traps on SIGTTOU, like other shells do?

It's not like there is real-world need to allow usage of this signal
- it's not customarily used by userspace for inter-process signaling,
unlike e.g. SIGTERM or SIGUSR1/2.