[Bug libc/28853] New: Problems with terminal control

[Bug libc/28853] New: Problems with terminal control

[danglin at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

            Bug ID: 28853
           Summary: Problems with terminal control
           Product: glibc
           Version: 2.35
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: danglin at gcc dot gnu.org
                CC: adhemerval.zanella at linaro dot org, deller at gmx dot de,
                    drepper.fsp at gmail dot com
  Target Milestone: ---
              Host: hppa*-*-linux*
            Target: hppa*-*-linux*
             Build: hppa*-*-linux*

This commit causes issues with job control on hppa:

commit 342cc934a3bf74ac618e2318d738f22ac93257ba (HEAD)
Author:     Adhemerval Zanella <adhemerval.zanella@linaro.org>
AuthorDate: Mon Jun 14 14:41:31 2021 -0300
Commit:     Adhemerval Zanella <adhemerval.zanella@linaro.org>
CommitDate: Tue Jan 25 14:07:53 2022 -0300

    posix: Add terminal control setting support for posix_spawn

    Currently there is no proper way to set the controlling terminal through
    posix_spawn in race free manner [1].  This forces shell implementations
    to keep using fork+exec when launching background process groups,
    even when using posix_spawn yields better performance.

    This patch adds a new GNU extension so the creating process can
    configure the created process terminal group.  This is done with a new
    flag, POSIX_SPAWN_TCSETPGROUP, along with two new attribute functions:
    posix_spawnattr_tcsetpgrp_np, and posix_spawnattr_tcgetpgrp_np.
    The function sets a new attribute, spawn-tcgroupfd, that references to
    the controlling terminal.

    The controlling terminal is set after the spawn-pgroup attribute, and
    uses the spawn-tcgroupfd along with current creating process group
    (so it is composable with POSIX_SPAWN_SETPGROUP).

    To create a process and set the controlling terminal, one can use the
    following sequence:

        posix_spawnattr_t attr;
        posix_spawnattr_init (&attr);
        posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP);
        posix_spawnattr_tcsetpgrp_np (&attr, tcfd);

    If the idea is also to create a new process groups:

        posix_spawnattr_t attr;
        posix_spawnattr_init (&attr);
        posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP
                                         | POSIX_SPAWN_SETPGROUP);
        posix_spawnattr_tcsetpgrp_np (&attr, tcfd);
        posix_spawnattr_setpgroup (&attr, 0);

    The controlling terminal file descriptor is ignored if the new flag is
    not set.

    This interface is slight different than the one provided by QNX [2],
    which only provides the POSIX_SPAWN_TCSETPGROUP flag.  The QNX
    documentation does not specify how the controlling terminal is obtained
    nor how it iteracts with POSIX_SPAWN_SETPGROUP.  Since a glibc
    implementation is library based, it is more straightforward and avoid
    requires additional file descriptor operations to request the caller
    to setup the controlling terminal file descriptor (and it also allows
    a bit less error handling by posix_spawn).

    Checked on x86_64-linux-gnu and i686-linux-gnu.

    [1] https://github.com/ksh93/ksh/issues/79
    [2]
https://www.qnx.com/developers/docs/7.0.0/index.html#com.qnx.doc.neutrino.lib_ref/topic/p/posix_spawn.html

    Reviewed-by: Carlos O'Donell <carlos@redhat.com>
    Tested-by: Carlos O'Donell <carlos@redhat.com>

When I do a glibc build and check, I use a script as follows:

nohup ../build > build.log 2>&1 &

I usually monitor progress with less:

less build.log

At some point in the check, less stops and goes into the background.  I
think this occurs when tst-spawn6 runs.  It appears that the test affects
processes beyond those spawned in test.

This occurs with all kernel versions that I have tried.

With 5.16.4, I also see general problems with job control after running
the glibc testsuite.  Here is output from ssh login:

Using username "dave".
Authenticating with public key "imported-openssh-key"
Linux mx3210 5.16.4+ #1 SMP Sat Jan 29 18:56:52 UTC 2022 parisc64

The programs included with the Debian GNU/Linux system are free software;
the exact distribution terms for each program are described in the
individual files in /usr/share/doc/*/copyright.

Debian GNU/Linux comes with ABSOLUTELY NO WARRANTY, to the extent
permitted by applicable law.
You have no mail.
Last login: Sat Jan 29 20:27:13 2022 from 192.168.2.49
-bash: cannot set terminal process group (-1): Bad file descriptor
-bash: no job control in this shell

This doesn't seem to occur if glibc testsuite hasn't been run.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] Problems with terminal control

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

--- Comment #1 from Andreas Schwab <schwab@linux-m68k.org> ---
Since hppa is big endian, this could be an endian bug clobbering the terminal
process group.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] Problems with terminal control

[danglin at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

--- Comment #2 from John David Anglin <danglin at gcc dot gnu.org> ---
I suppose this is the problematic part of the change:

  /* Set the controlling terminal.  */
  if ((attr->__flags & POSIX_SPAWN_TCSETPGROUP) != 0)
    {
      /* Check if it is possible to avoid an extra syscall.  */
      pid_t pgrp = (attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
                    && attr->__pgrp != 0
                   ? attr->__pgrp : __getpgid (0);
      if (__tcsetpgrp (attr->__ctty_fd, pgrp) != 0)
        goto fail;
    }

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] Problems with terminal control

[danglin at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

--- Comment #3 from John David Anglin <danglin at gcc dot gnu.org> ---
       The  function tcsetpgrp() makes the process group with process group ID
       pgrp the foreground process group on the  terminal  associated  to  fd,
       which  must  be  the  controlling  terminal of the calling process, and
       still be associated  with  its  session.   Moreover,  pgrp  must  be  a
       (nonempty)  process  group belonging to the same session as the calling
       process.

       If tcsetpgrp() is called by a member of a background process  group  in
       its  session, and the calling process is not blocking or ignoring SIGT-
       TOU, a SIGTTOU signal is sent to all members of this background process
       group.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] Problems with terminal control

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com

--- Comment #4 from Carlos O'Donell <carlos at redhat dot com> ---
Dave,

Thanks for the bug report.

I can confirm that posix/tst-spawn6 breaks out of the support/* isolation and
in doing so impacts the shell / process group / session on the terminal.

e.g.
[2]+  Stopped                 less ~/build/glibc-review/check.log

(In reply to John David Anglin from comment #2)
> I suppose this is the problematic part of the change:
> 
>   /* Set the controlling terminal.  */
>   if ((attr->__flags & POSIX_SPAWN_TCSETPGROUP) != 0)
>     {
>       /* Check if it is possible to avoid an extra syscall.  */
>       pid_t pgrp = (attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
>                     && attr->__pgrp != 0
>                    ? attr->__pgrp : __getpgid (0);
>       if (__tcsetpgrp (attr->__ctty_fd, pgrp) != 0)
>         goto fail;
>     }

This is not problematic per-se, since this is what you want to do for shell
control, but if the test isolation code doesn't stop this affecting your
current shell then that's a problem.

The posix/tst-spawn6 test is the *only* test in which we exercise process group
control testing, and so it may break the testing isolation we expect.

(In reply to John David Anglin from comment #3)
> The  function tcsetpgrp() makes the process group with process group ID
> pgrp the foreground process group on the  terminal  associated  to fd,
> which  must  be  the  controlling  terminal of the calling process, and
> still be associated  with  its  session.   Moreover,  pgrp  must  be a
> (nonempty)  process  group belonging to the same session as the calling
> process.
> If tcsetpgrp() is called by a member of a background process  group in
> its  session, and the calling process is not blocking or ignoring SIGT-
> TOU, a SIGTTOU signal is sent to all members of this background process
> group.

Yes, for the current /dev/tty (in the test), we make the posix_spawn'd
process the foreground process group for the session to verify the test works
correctly.

I can't easily containerize the test because we don't setup an alternative
TTY/session/pgrp in the container e.g. tests-container += tst-spawn6.

make[2]: Leaving directory '/home/carlos/src/glibc-review/posix'
FAIL: posix/tst-spawn6
original exit status 1
error: tst-spawn6.c:138: open64 ("/dev/tty", 0x0, 0600): No such file or
directory
error: 1 test failures
make[1]: Leaving directory '/home/carlos/src/glibc-review'

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Problems with terminal      |tst-spawn6 changes current
                   |control                     |foreground process group
                   |                            |(breaks test isolation)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

--- Comment #5 from Carlos O'Donell <carlos at redhat dot com> ---
The solution here is that the test needs it's own session with a new process
group, and new terminal. So posix/tst-spawn6 needs some additional work to
avoid affecting build systems.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

--- Comment #6 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Carlos O'Donell from comment #5)
> The solution here is that the test needs it's own session with a new process
> group, and new terminal. So posix/tst-spawn6 needs some additional work to
> avoid affecting build systems.

And we have a fix posted for this:
https://sourceware.org/pipermail/libc-alpha/2022-February/136033.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

--- Comment #7 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Fixed on 2.36.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
As per last comment.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |2.36

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/28853] tst-spawn6 changes current foreground process group (breaks test isolation)

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28853

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hjl.tools at gmail dot com

--- Comment #9 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
*** Bug 28836 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.