Re: supporting terminal ownership assignment (tcsetpgrp()) in posix_spawn

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

On 07/06/2021 20:57, Godmar Back wrote:
> On Mon, Jun 7, 2021 at 5:36 PM Adhemerval Zanella <
> adhemerval.zanella@linaro.org> wrote:
> 
>>
>> Yeah I forgot the inherent race condition on this scheme.  I think it
>> should be possible to add such interface as a posix_spawn file action:
>>
>>   int posix_spawn_file_actions_tcsetpgrp_np (int fd)
>>
>> Not sure if it make sense to support a pid different than the created
>> helper process (which will eventually call execve) since it is something
>> the parent can do it itself.
>>
>>
> The Blackberry implementation [1] has a single flag, with no arguments.  It
> doesn't pass a file descriptor. It appears undocumented, but I would guess
> that this means that the child's process group becomes the foreground
> process group of the controlling terminal, which in a kernel implementation
> may be directly accessible. In a library implementation, it would require
> obtaining a file descriptor, perhaps via `ctermid`, which would introduce
> another error situation that would need to be handled.
> 
> I think either approach (flag or file action) would be fine; in the
> envisioned use case, it's likely that the caller already has an open file
> descriptor available that refers to the intended terminal.
> 
> Incidentally, I also sent an email to the mailing list of the Austin group,
> though haven't received any replies.
> 
>  - Godmar
> 
> [1]
> https://developer.blackberry.com/native/reference/core/com.qnx.doc.neutrino.lib_ref/topic/p/posix_spawnattr_setxflags.html
> 

It seems that another shell implementation have stumbled over the same 
issue [1].  Setting the terminal ownership does seem to be a good API 
extension because some developers are leaning of using vfork(), which 
is another really bad API (I have improved glibc  posix_spawn() exactly 
to give developers a better options than using vfork+exec).

I think the only issue is which would be better API to provide such extension.
The QNX API you referred in indeed is really scarce regarding documentation [2],
it only states that:

| If you want to:                   	Do the following:
| Start a new terminal group            Set POSIX_SPAWN_TCSETPGROUP

It does not really specify which process group will be used to tcsetpgrp call,
neither how it interacts with POSIX_SPAWN_SETPGROUP or POSIX_SPAWN_SETSID 
(which order the flags commands are execute and if they interfere with each
other).

So one options might be to add something as the one I suggested before,
but as generic extension instead of a file extension (which does not make
sense in fact):

  int posix_spawnattr_tcsetpgrp_np (posix_spawnattr_t *__attr, int fd, pid_t pgrp);

  Similar to tcgetpgrp, it make the created process group with process group
  instructed by the PGRP argument the foreground process group on the terminal
  associated to FD.  If PGRP is 0, the current group obtained with getpgrp()
  will be used, otherwise PGRP will be used.

  This is done after just after setting the process group ID
  (POSIX_SPAWN_SETPGROUP) and right before setting the effective user and
  group id (POSIX_SPAWN_RESETIDS).

So if the called want to create a new session id, it can issue:

  posix_spawnattr_t attr;
  posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSID);
  posix_spawnattr_tcsetpgrp_np (&attr, fd, 0);

And the created process will issue the following in the order:

  setsid ();
  tcsetpgrp (fd, getpgid (0));


If the caller already has group it want to use, it can issue instead:

  posix_spawnattr_t attr;
  posix_spawnattr_setpgroup (&attr, groupid);
  posix_spawnattr_tcsetpgrp_np (&attr, fd, groupid);

Which in turn will make the created process to issue:

  setpgid (0, groupid);
  tcsetpgrp (fd, groupid);

What do you think? Does it help your shell implementation and avoid the
potential race condition?

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