[PATCH] tcsetpgrp fails unexpectedly

[Tor Perkins <cygwin at noid dot net>]

I think I've found two problems in fhandler_termios::bg_check():

  * Cygwin's tcsetpgrp function will return EIO when the process
    group for the calling process has no leader.

  * This appears to be caused by a leaderless process group being
    interpreted as an orphaned process group.

Please find a plain text file attachment that includes a changelog
entry and a patch.

Please find a plain text file attachment that includes a small Perl
script that can be used as a test case.

Rationale:
----------

When the first process in a pipeline exits but a second process
keeps running, the process group for this pipeline will no longer
have a leader.  This is what happens when my test script is run like
so:

  cat </dev/null | /usr/bin/perl /tmp/test_bg_check.pl

The leader ('cat') has exited by the time the test script invokes
tcsetpgrp.  The following snippet in fhandler_termios::bg_check gets
triggered when this happens:

  /* If the process group is no more ... */
  int pgid_gone = !pid_exists (myself->pgid);

  if (pgid_gone)
    goto setEIO;

Here we see that fhandler_termios::bg_check will not tolerate a
leaderless process group.

The comment indicates that the test is looking for a process group
that is "no more", however, the process group still exists and has a
process in it (the calling process); it just happens to not have a
leader at this time.

I've not been able to find any documentation that says a leaderless
process group is problematic in some way.  Also, the script runs
fine in pipeline mode on both Linux and OpenBSD.

POSIX does talk about returning EIO for tcsetattr when:

  The process group of the writing process is orphaned, and the
  writing process is not ignoring or blocking SIGTTOU.

But SIGTTOU is ignored in my test script, so the above should not
apply (and I'm not even calling tcsetattr).  Parenthetically, SunOS
will return EIO for tcsetpgrp under the circumstance described
above, so I think EIO as a possible return value for tcsetpgrp is
not a problem per se...

Also, a leaderless process group is not the same thing as an
orphaned process group, the latter being defined by POSIX as:

  A process group in which the parent of every member is either
  itself a member of the group or is not a member of the group's
  session.

Out of curiosity, I researched how the EIO test is handled in both
Linux and OpenBSD.  The approaches are quite different.  In Linux,
we see code that loops through the relevant pids to see if the
process group is orphaned (edited for readability):

  vi /usr/src/linux-2.6.32/drivers/char/tty_io.c +/is_current_pgrp_orphaned
  vi /usr/src/linux-2.6.32/kernel/exit.c         +/will_become_orphaned_pgrp

    static int
    will_become_orphaned_pgrp (struct pid *pgrp, struct task_struct *ignored_task)
    {
      struct task_struct *p;
      do_each_pid_task(pgrp, PIDTYPE_PGID, p) {
        if (task_pgrp(p->real_parent) != pgrp &&
            task_session(p->real_parent) == task_session(p))
          return 0;
      } while_each_pid_task(pgrp, PIDTYPE_PGID, p);
      return 1;
    }

In OpenBSD we see that it maintains a count of "qualified" jobs per
process group.  A qualified job is one with a parent in a different
process group of the same session.  The EIO test then becomes a
matter of looking for a zero qualified jobs count (also edited):

  vi /usr/src/sys/kern/kern_proc.c +/fixjobc
  vi /usr/src/sys/kern/tty.c       +/isbackground

    while (isbackground(pr, tp) &&
        (p->p_flag & P_PPWAIT) == 0 &&
        (p->p_sigignore & sigmask(SIGTTOU)) == 0 &&
        (p->p_sigmask & sigmask(SIGTTOU)) == 0) {
      if (pr->ps_pgrp->pg_jobc == 0)                  // <-- here
        return (EIO);
      pgsignal(pr->ps_pgrp, SIGTTOU, 1);
    } // continue on - allow the ioctl

Neither approach cares if a process group has a leader or not...

My patch adds a new function to test for orphaned process groups.
It also modifies the decision tree to take advantage of the new
function.  I think it will cause fhandler_termios::bg_check to be
more correct and hopefully not introduce any regressions.  It is, at
least, a fix for my test case...

Thanks for your consideration and thanks for such a wonderful system!

- Tor

Attachment: cygwin_bg_eio.diff
Description: Text document

Attachment: test_bg_check.pl
Description: Text document