| Message ID | 20210106044512.GA28191@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | jobs: Block signals during tcsetpgrp | expand |
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@sdaoden.eu> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > 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. Cheers, Harald van Dijk
On Wed, Jan 06, 2021 at 09:16:58PM +0000, Harald van Dijk 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@sdaoden.eu> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > 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. Comparing this error situation to the normal case, I think the right solution is to close any stray pipe ends we have, wait for the remaining child processes and only then report the error (throwing an exception as normal). The child processes will probably terminate soon because of a broken pipe, but even if they stop, they will not change the tty foreground process group any more. The code in jobs.c will then reset it. The same error handling applies to the situation where pipe() fails. This is a bit easier to test reliably, using ulimit -n. Adding synchronization with the child processes slows down the normal case for a rare error case, and the synchronization objects such as pipe, eventfd, SysV semaphore or MAP_SHARED mapping might cause unexpected issues in strange use cases. A related bug: if fork fails for a command substitution, the pipe created for reading the command output remains open (two descriptors). This one is also in dash as well as FreeBSD sh. Throwing exceptions from forkshell() may not be the best idea.
On Wed, Jan 6, 2021 at 10:17 PM Harald van Dijk <harald@gigawatt.nl> 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@sdaoden.eu> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > > > 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.
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