Re: [PATCH 2/2] posix: Add terminal control setting support for posix_spawn

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 27/06/2021 22:11, Godmar Back wrote:
> Thank you for cc'ing me. I am assuming you're asking for feedback on your patch.

Yes,  if you could review the API from the user perspective it would be really
helpful (and the patch review itself would be even more helpful).

> 
> A few notes with what I've noticed.
> 
> (1)
> +/* Returh the associated terminal FD in the attribute structure.  */
> 
> Should it be "Return?"

It should, I have fixed.

> 
> (2)
> +  int __tcpgrp;
> 
> This field holds a file descriptor, but the name '__tcpgrp' could be
> misinterpreted as a "pgrp" - as in "terminal control process group"
> If would suggest a different name perhaps, such as '__ctty_fd' or
> '__tcfd' or something like that to make it more clear that the field
> contains a file descriptor.

Indeed, I have changed to __ctty_fd.

> 
> (3)
> Is passing '0600' necessary if you open with O_RDONLY as in:
> 
> +  int fd = xopen (_PATH_TTY, O_RDONLY, 0600);

No really, we can use a different value for testing.

> 
> 
> (4)
> I didn't see any tests, and I'm not sure the code would work as it's written.
> Specifically, the child process needs to block (mask) SIGTTOU around
> the tcsetpgrp() call.
> This is because after it calls `setpgid` to place itself into a new
> process group, it will no longer be in the terminal's foreground
> process group (by definition, all process groups that are not the
> foreground process group are "background process groups").
> Consequently, attempts to call `tcsetpgrp()` will result in SIGTTOU,
> which must be blocked.
> 
> See tcgetpgrp(3):
> 
>    If tcsetpgrp() is called by a member of a background process group
> in its session, and the calling process is not
>    blocking or ignoring  SIGTTOU, a SIGTTOU signal is sent to all
> members of this background process group.
> 
> Try the following test program to see this:
> 
> #include <unistd.h>
> #include <fcntl.h>
> #include <signal.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> #include <sys/wait.h>
> 
> int
> main()
> {
>     pid_t pgid = 0;
>     int ttyfd = open("/dev/tty", O_RDONLY);
> 
>     // typical sequence if user types 'a | b' (without the pipes)
>     for (int i = 0; i < 2; i++) {
>         int child = fork();
>         if (child == 0) {
>             setpgid(0, pgid);
>             // you need this call
>             // signal(SIGTTOU, SIG_IGN);
>             int rc = tcsetpgrp(ttyfd, getpgrp());
>             if (rc != 0) {
>                 perror("tcsetpgrp");
>                 abort();
>             }
>             printf("child process %d group %d successfully has
> terminal ownership\n", getpid(), getpgrp());
>             exit(0);
>         } else {
>             setpgid(child, pgid);
>         }
> 
>         if (pgid == 0)
>             pgid = child;
> 
>         printf("spawned #%d\n", i+1);
>     }
> 
>     for (int i = 0; i < 2; i++) {
>         int status;
>         int child = waitpid(-1, &status, WUNTRACED);
>         if (WIFSTOPPED(status)) {
>             printf("child %d was stopped with %s\n", child,
> strsignal(WSTOPSIG(status)));
>         } else
>         if (WIFEXITED(status)) {
>             printf("child %d exited with %d\n", child, WEXITSTATUS(status));
>         }
>     }
> }

The second subtest on posix/tst-spawn5.c should check for it:

  {
    posix_spawnattr_t attr;
    TEST_COMPARE (posix_spawnattr_init (&attr), 0);
    TEST_COMPARE (posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP
						   | POSIX_SPAWN_SETPGROUP),
		  0);
    TEST_COMPARE (posix_spawnattr_setpgroup (&attr, 0), 0);

    run_subprogram (argc, argv, &attr, 0);
  }

But it contains an issue where the controlling terminal is not correctly
set.  Fixing it:

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

    run_subprogram (argc, argv, &attr, 0);
  }

It issues the following syscalls:

[pid 3367391] clone(child_stack=0x7f3bec54fff0, flags=CLONE_VM|CLONE_VFORK|SIGCHLD
[...]
[pid 3367392] rt_sigaction(SIGTTOU, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
[pid 3367392] rt_sigaction(SIGTTOU, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f3bec590560}, NULL, 8) = 0
[...]
[pid 3367392] setpgid(0, 0)             = 0
[pid 3367392] getpgrp()                 = 3367392
[pid 3367392] ioctl(4, TIOCSPGRP, [3367392]) = 0
[pid 3367392] rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0  
[pid 3367392] execve("/home/azanella/Projects/glibc/build/x86_64-linux-gnu-hp/posix/tst-spawn5", ["/home/azanella/Projects/glibc/bu"..., "--direct", "--restart", "setgrpr"], 0x7ffef7371ca8 /* 61 vars */ <unfinished ...>

So there is no SIGTTOU being generated.  I even tried to add a timeout on
the helper process to check if the problem is the signal being generated
asynchronously, but it is works as intended. 

> 
> (5) Since you have to block (or ignore) SIGTTOU around the call to
> tcsetpgrp, you would have to possibly restore the original signal
> state before exec'ing the child.
> This may interact with POSIX_SPAWN_SETSIGMASK and/or
> POSIX_SPAWN_SETSIGDEF if the caller to posix_spawn() includes SIGTTOU
> there. In either case, the correct semantics would have to be
> implemented.
> 

I need to understand better why I am not seeing this behavior in the
posix_spawn implementation before.