supporting terminal ownership assignment (tcsetpgrp()) in posix_spawn

supporting terminal ownership assignment (tcsetpgrp()) in posix_spawn

[Godmar Back]

I've recently been playing with posix_spawn() and noticed that it lacks
support for assigning terminal ownership (as would be done via
tcsetpgrp()). As a consequence, it cannot be used for job control shells
when starting jobs in the foreground [1].

IBM's implementation of spawn [2] for zOS supports SPAWN_SETTCPGRP for this
reason.

Even though the POSIX spec states "Future Directions: None" I came across
this Austin group issue [3] which proposes to add a way to set the child
process's session id, and which appears to have been accepted in 2016. This
is an example of evolution of posix_spawn.

Is anyone aware of efforts to add something similar to POSIX, and more
specifically, to Linux?

If the current implementation of posix_spawn is library based (I'm guessing
it is) then there would be nothing to prevent Linux from adding additional
flags, just like QNX or Blackberry have already done ([3]).

 - Godmar

[1]
https://github.com/fish-shell/fish-shell/commit/5e371e8fe7cbdecd87678d50d06c12851213776e
[2]
https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-spawn-spawnp-spawn-new-process
[3] https://www.austingroupbugs.net/view.php?id=1044

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

[Adhemerval Zanella]

On 03/06/2021 10:58, Godmar Back via Libc-help wrote:
> I've recently been playing with posix_spawn() and noticed that it lacks
> support for assigning terminal ownership (as would be done via
> tcsetpgrp()). As a consequence, it cannot be used for job control shells
> when starting jobs in the foreground [1].
> 
> IBM's implementation of spawn [2] for zOS supports SPAWN_SETTCPGRP for this
> reason.
> 
> Even though the POSIX spec states "Future Directions: None" I came across
> this Austin group issue [3] which proposes to add a way to set the child
> process's session id, and which appears to have been accepted in 2016. This
> is an example of evolution of posix_spawn.
> 
> Is anyone aware of efforts to add something similar to POSIX, and more
> specifically, to Linux?
> 
> If the current implementation of posix_spawn is library based (I'm guessing
> it is) then there would be nothing to prevent Linux from adding additional
> flags, just like QNX or Blackberry have already done ([3]).

Other OS might provide a syscall for posix_spawn (I know MacOSX does and
I think maybe Solaris as well), but for Linux is done on top of clone
syscall.

And glibc alread supports POSIX_SPAWN_SETSID since 2.26, so why can't you
use it along with with tcsetpgrp() called from the parent? 

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

[Godmar Back]

On Mon, Jun 7, 2021 at 5:04 PM Adhemerval Zanella <
adhemerval.zanella@linaro.org> wrote:

>
>
> On 03/06/2021 10:58, Godmar Back via Libc-help wrote:
> > I've recently been playing with posix_spawn() and noticed that it lacks
> > support for assigning terminal ownership (as would be done via
> > tcsetpgrp()). As a consequence, it cannot be used for job control shells
> > when starting jobs in the foreground [1].
> >
> > IBM's implementation of spawn [2] for zOS supports SPAWN_SETTCPGRP for
> this
> > reason.
> >
> > Even though the POSIX spec states "Future Directions: None" I came across
> > this Austin group issue [3] which proposes to add a way to set the child
> > process's session id, and which appears to have been accepted in 2016.
> This
> > is an example of evolution of posix_spawn.
> >
> > Is anyone aware of efforts to add something similar to POSIX, and more
> > specifically, to Linux?
> >
> > If the current implementation of posix_spawn is library based (I'm
> guessing
> > it is) then there would be nothing to prevent Linux from adding
> additional
> > flags, just like QNX or Blackberry have already done ([3]).
>
> Other OS might provide a syscall for posix_spawn (I know MacOSX does and
> I think maybe Solaris as well), but for Linux is done on top of clone
> syscall.
>
> And glibc alread supports POSIX_SPAWN_SETSID since 2.26, so why can't you
> use it along with with tcsetpgrp() called from the parent?


SETSID sets a new session, which is a different concept than the foreground
process group of a
controlling terminal.

tcsetpgrp() informs the OS which process group shall be the foreground
process group of the
terminal to which it is applied.  However, this can't be called by the
parent since the parent
doesn't know the process group id of the process that will be spawned by
the call to posix_spawn().

This process id (and the corresponding process group id) is known to the
parent only after the
call to posix_spawn() was made (and has returned). By that point, it's too
late to set the controlling
terminal's foreground process group. Specifically, by the time posix_spawn
returns, the child process
may have already acted under the assumption that it has ownership of the
terminal. Since the
child process is in a separate process group (because POSIX_SPAWN_SETPGROUP
was given to
posix_spawn()), this may lead to the suspension of the child process via
SIGTTIN/SIGTTOU.

Thus, calling tcsetpgrp() from the parent after the fact cannot be done in
a race-free fashion, one
needs to ensure that it's done after placing the child in a new process
group but before exec'ing the
child's executable.

In Linux's library-based implementation of posix_spawn(), support
for POSIX_SPAWN_TCSETPGROUP
could be easily added - my question is why hasn't it?

 - Godmar

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

[Adhemerval Zanella]

On 07/06/2021 18:23, Godmar Back wrote:
> On Mon, Jun 7, 2021 at 5:04 PM Adhemerval Zanella <
> adhemerval.zanella@linaro.org> wrote:
> 
>>
>>
>> On 03/06/2021 10:58, Godmar Back via Libc-help wrote:
>>> I've recently been playing with posix_spawn() and noticed that it lacks
>>> support for assigning terminal ownership (as would be done via
>>> tcsetpgrp()). As a consequence, it cannot be used for job control shells
>>> when starting jobs in the foreground [1].
>>>
>>> IBM's implementation of spawn [2] for zOS supports SPAWN_SETTCPGRP for
>> this
>>> reason.
>>>
>>> Even though the POSIX spec states "Future Directions: None" I came across
>>> this Austin group issue [3] which proposes to add a way to set the child
>>> process's session id, and which appears to have been accepted in 2016.
>> This
>>> is an example of evolution of posix_spawn.
>>>
>>> Is anyone aware of efforts to add something similar to POSIX, and more
>>> specifically, to Linux?
>>>
>>> If the current implementation of posix_spawn is library based (I'm
>> guessing
>>> it is) then there would be nothing to prevent Linux from adding
>> additional
>>> flags, just like QNX or Blackberry have already done ([3]).
>>
>> Other OS might provide a syscall for posix_spawn (I know MacOSX does and
>> I think maybe Solaris as well), but for Linux is done on top of clone
>> syscall.
>>
>> And glibc alread supports POSIX_SPAWN_SETSID since 2.26, so why can't you
>> use it along with with tcsetpgrp() called from the parent?
> 
> 
> SETSID sets a new session, which is a different concept than the foreground
> process group of a
> controlling terminal.
> 
> tcsetpgrp() informs the OS which process group shall be the foreground
> process group of the
> terminal to which it is applied.  However, this can't be called by the
> parent since the parent
> doesn't know the process group id of the process that will be spawned by
> the call to posix_spawn().
> 
> This process id (and the corresponding process group id) is known to the
> parent only after the
> call to posix_spawn() was made (and has returned). By that point, it's too
> late to set the controlling
> terminal's foreground process group. Specifically, by the time posix_spawn
> returns, the child process
> may have already acted under the assumption that it has ownership of the
> terminal. Since the
> child process is in a separate process group (because POSIX_SPAWN_SETPGROUP
> was given to
> posix_spawn()), this may lead to the suspension of the child process via
> SIGTTIN/SIGTTOU.
> 
> Thus, calling tcsetpgrp() from the parent after the fact cannot be done in
> a race-free fashion, one
> needs to ensure that it's done after placing the child in a new process
> group but before exec'ing the
> child's executable.

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.

> 
> In Linux's library-based implementation of posix_spawn(), support
> for POSIX_SPAWN_TCSETPGROUP
> could be easily added - my question is why hasn't it?

I would say because POSIX does not define it in the standard and no one
has asked it before.

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

[Godmar Back]

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

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

[Adhemerval Zanella]

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

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

[Godmar Back]

On Tue, Jun 8, 2021 at 9:50 AM Adhemerval Zanella <
adhemerval.zanella@linaro.org> wrote:

>
> 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));
>
>
After setsid(), the caller is a new session leader and a new process group
leader, but it doesn't have a controlling terminal.
(setsid(2) says: "The calling process will be the only process in the new
process group and in the new session. Initially, the new session has no
controlling terminal.")

The man page of tcsetpgrp however states that the fd passed: "must be the
controlling terminal of the calling process."
So if you implemented it like that in a library, it should fail based on
the description in the man pages.

I'm not sure if POSIX_SPAWN_SETSID needs to be able to be combined with
setting the terminal's foreground process group. Those are different use
cases. In the case of setsid, the spawned process must be prepared to
become a new session leader (open a new controlling terminal, etc.) so it
would not generally assume to already be a member of the terminal's fg
process group (since it doesn't have a controlling terminal yet).


>
> 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);
>

For this use case, as long as it supports groupid == 0, this should work as
it is what shells currently do.

 - Godmar

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

[Adhemerval Zanella]

On 08/06/2021 11:37, Godmar Back wrote:
> On Tue, Jun 8, 2021 at 9:50 AM Adhemerval Zanella <
> adhemerval.zanella@linaro.org> wrote:
> 
>>
>> 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));
>>
>>
> After setsid(), the caller is a new session leader and a new process group
> leader, but it doesn't have a controlling terminal.
> (setsid(2) says: "The calling process will be the only process in the new
> process group and in the new session. Initially, the new session has no
> controlling terminal.")
> 
> The man page of tcsetpgrp however states that the fd passed: "must be the
> controlling terminal of the calling process."
> So if you implemented it like that in a library, it should fail based on
> the description in the man pages.
> 
> I'm not sure if POSIX_SPAWN_SETSID needs to be able to be combined with
> setting the terminal's foreground process group. Those are different use
> cases. In the case of setsid, the spawned process must be prepared to
> become a new session leader (open a new controlling terminal, etc.) so it
> would not generally assume to already be a member of the terminal's fg
> process group (since it doesn't have a controlling terminal yet).

Indeed this won't accomplish anything in fact, one will need to issue
a ioctl with TIOCSCTTY to recover the terminal (as login_tty does). I
agree with you that the usual usercase for POSIX_SPAWN_SETSID is to 
have the created process to setup itself the required terminal.

There is hacks to reparent a running program to a new terminal by using
ptrace to inject syscalls on the target process, but I think it is
really out of scope from the libc.

> 
> 
>>
>> 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);
>>
> 
> For this use case, as long as it supports groupid == 0, this should work as
> it is what shells currently do.

So the my question is whether providing the groupid as an argument is
really required (I would say yes so it can be combined with
posix_spawnattr_setpgroup).

Another question is when to issue the tcsetpgrp related to
POSIX_SPAWN_SETSID. I would say tcsetpgrp should be issued *before*
setsid, so tcsetpgrp can return early if it fails. Otherwise tcsetpgrp
will always fail if POSIX_SPAWN_SETSID is set (it would be a caller
error, but I think from API viewpoint it should be better if we could 
minimize the possible error scenarios).

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

[Godmar Back]

On Tue, Jun 8, 2021 at 12:42 PM Adhemerval Zanella <
adhemerval.zanella@linaro.org> wrote:

> >
> >
> >>
> >> 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);
> >>
> >
> > For this use case, as long as it supports groupid == 0, this should work
> as
> > it is what shells currently do.
>
> So the my question is whether providing the groupid as an argument is
> really required (I would say yes so it can be combined with
> posix_spawnattr_setpgroup).
>

The Blackberry API does not pass an argument (as a point of reference).
In practice, when spawning a pipeline like a | b | c ... we pass group id 0
to the
first child, and then the pid if of the first child as the pgrp id to the
second and
third. This means that if the groupid is omitted, it needs to refer to the
group id that was given to POSIX_SPAWN_SETPGROUP, and thus
POSIX_SPAWN_SETPGROUP is a prerequisite for it.


>
> Another question is when to issue the tcsetpgrp related to
> POSIX_SPAWN_SETSID. I would say tcsetpgrp should be issued *before*
> setsid, so tcsetpgrp can return early if it fails. Otherwise tcsetpgrp
> will always fail if POSIX_SPAWN_SETSID is set (it would be a caller
> error, but I think from API viewpoint it should be better if we could
> minimize the possible error scenarios).
>

I haven't checked how the current implementation works. Do you use a pipe
or something
for the child to report back if something went wrong, or does the parent
somehow check if the
asked-for actions in the child will likely succeed?  If the former, doing
setsid first, and then,
if given, doing the tcsetpgrp call will fail as you say and this failure
would be reported back
to the caller. Then they know they've misused the API.

If you do tcsetpgrp first and then setsid, the tcsetpgrp will succeed, but
will be ineffective, and
the caller won't ever know.

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

[Florian Weimer]

* Godmar Back via Libc-help:

> I haven't checked how the current implementation works. Do you use a
> pipe or something for the child to report back if something went
> wrong, or does the parent somehow check if the asked-for actions in
> the child will likely succeed?  If the former, doing setsid first, and
> then, if given, doing the tcsetpgrp call will fail as you say and this
> failure would be reported back to the caller. Then they know they've
> misused the API.

It's the first.  We use shared memory (the side effect of vfork) to
communicate errno to the original process.

Thanks,
Florian

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

[Adhemerval Zanella]

On 08/06/2021 19:11, Godmar Back wrote:
> On Tue, Jun 8, 2021 at 12:42 PM Adhemerval Zanella <
> adhemerval.zanella@linaro.org> wrote:
> 
>>>
>>>
>>>>
>>>> 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);
>>>>
>>>
>>> For this use case, as long as it supports groupid == 0, this should work
>> as
>>> it is what shells currently do.
>>
>> So the my question is whether providing the groupid as an argument is
>> really required (I would say yes so it can be combined with
>> posix_spawnattr_setpgroup).
>>
> 
> The Blackberry API does not pass an argument (as a point of reference).
> In practice, when spawning a pipeline like a | b | c ... we pass group id 0
> to the
> first child, and then the pid if of the first child as the pgrp id to the
> second and
> third. This means that if the groupid is omitted, it needs to refer to the
> group id that was given to POSIX_SPAWN_SETPGROUP, and thus
> POSIX_SPAWN_SETPGROUP is a prerequisite for it.
> 

The libc interface should be as generic possible to cover most users cases,
I really don't want to add a posix_spawnattr_tcsetpgrp_np that uses the 
groupid implicit from POSIX_SPAWN_SETPGROUP to someone ask for a
posix_spawnattr_tcsetpgrp_np_ex so one can set the groupid.

Also, requiring POSIX_SPAWN_SETPGROUP for posix_spawnattr_tcsetpgrp_np is
not really a good API, it adds subtle semantics (should it use a default
value set by posix_spawnattr_init or should we fail with EINVAL), and adds
complexity in the error path (we will need to either pre validate the
posix_spawnattr_t input before start the process creation or handle a
possible invalid combination on the helper process itself).

That's why I am more inclined to follow the tcsetpgrp on the posix_spawn
extension and let the caller set the required groups.

> 
>>
>> Another question is when to issue the tcsetpgrp related to
>> POSIX_SPAWN_SETSID. I would say tcsetpgrp should be issued *before*
>> setsid, so tcsetpgrp can return early if it fails. Otherwise tcsetpgrp
>> will always fail if POSIX_SPAWN_SETSID is set (it would be a caller
>> error, but I think from API viewpoint it should be better if we could
>> minimize the possible error scenarios).
>>
> 
> I haven't checked how the current implementation works. Do you use a pipe
> or something
> for the child to report back if something went wrong, or does the parent
> somehow check if the
> asked-for actions in the child will likely succeed?  If the former, doing
> setsid first, and then,
> if given, doing the tcsetpgrp call will fail as you say and this failure
> would be reported back
> to the caller. Then they know they've misused the API.
> 
> If you do tcsetpgrp first and then setsid, the tcsetpgrp will succeed, but
> will be ineffective, and
> the caller won't ever know.
> 

As Florian has said we use a helper process created with CLONE_VM and
CLONE_VFORK, meaning that it will share the memory with its parent and 
CLONE_VFORK avoids any potential race condition.

And you raised a good point, I agree we should not hide the possible misuse
from the caller.

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

[Godmar Back]

On Wed, Jun 9, 2021 at 8:00 AM Adhemerval Zanella <
adhemerval.zanella@linaro.org> wrote:

>
> The libc interface should be as generic possible to cover most users cases,
> I really don't want to add a posix_spawnattr_tcsetpgrp_np that uses the
> groupid implicit from POSIX_SPAWN_SETPGROUP to someone ask for a
> posix_spawnattr_tcsetpgrp_np_ex so one can set the groupid.
>
> Also, requiring POSIX_SPAWN_SETPGROUP for posix_spawnattr_tcsetpgrp_np is
> not really a good API, it adds subtle semantics (should it use a default
> value set by posix_spawnattr_init or should we fail with EINVAL), and adds
> complexity in the error path (we will need to either pre validate the
> posix_spawnattr_t input before start the process creation or handle a
> possible invalid combination on the helper process itself).
>
> That's why I am more inclined to follow the tcsetpgrp on the posix_spawn
> extension and let the caller set the required groups.
>
>
I just checked and I don't think this will even work.  The caller cannot
provide the process group id because the caller can't know it yet.
A typical sequence in the first child of a pipeline is "setpgid(0, 0);
tcsetpgrp(fd, getpgrp());" and AFAIK tcsetpgrp doesn't accept '0' to stand
for the
current process's process group.

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

[Adhemerval Zanella]

On 09/06/2021 10:12, Godmar Back wrote:
> On Wed, Jun 9, 2021 at 8:00 AM Adhemerval Zanella <
> adhemerval.zanella@linaro.org> wrote:
> 
>>
>> The libc interface should be as generic possible to cover most users cases,
>> I really don't want to add a posix_spawnattr_tcsetpgrp_np that uses the
>> groupid implicit from POSIX_SPAWN_SETPGROUP to someone ask for a
>> posix_spawnattr_tcsetpgrp_np_ex so one can set the groupid.
>>
>> Also, requiring POSIX_SPAWN_SETPGROUP for posix_spawnattr_tcsetpgrp_np is
>> not really a good API, it adds subtle semantics (should it use a default
>> value set by posix_spawnattr_init or should we fail with EINVAL), and adds
>> complexity in the error path (we will need to either pre validate the
>> posix_spawnattr_t input before start the process creation or handle a
>> possible invalid combination on the helper process itself).
>>
>> That's why I am more inclined to follow the tcsetpgrp on the posix_spawn
>> extension and let the caller set the required groups.
>>
>>
> I just checked and I don't think this will even work.  The caller cannot
> provide the process group id because the caller can't know it yet.
> A typical sequence in the first child of a pipeline is "setpgid(0, 0);
> tcsetpgrp(fd, getpgrp());" and AFAIK tcsetpgrp doesn't accept '0' to stand
> for the
> current process's process group.

Indeed I think it wouldn't make sense to provide the input process group
argument, in only does if the created process wants to become de process
 group leader (POSIX_SPAWN_SETPGROUP plus posix_spawnattr_setpgroup (0)).

However I don't think adding a flag as QNX does (POSIX_SPAWN_TCSETPGROUP) 
is the best approach: it requires posix_spawn opening a new file descriptor 
to obtain the controlling terminal.  It add extra error path and put pressure
on the total file descriptor process has (assuming that caller expects no
file operation is done by posix_spawn).

So maybe this following:

  int posix_spawnattr_tcsetpgrp_np (posix_spawnattr_t *__attr, int fd);

  Similar to tcgetpgrp, it make the process group of the created process the
  foreground process group on the terminal associated to FD.  It is similar
  to tcsetpgrp(), where FD must be controlling terminal of the calling process,
  and still be associated with its session.

  It can be used along with POSIX_SPAWN_SETPGROUP to make the created process
  a new group leader to be used as the process group.


And then you can make a new process the foreground process group with:

  int fd = open (_PATH_TTY, O_RDWR | O_CLOEXEC);
  
  posix_spawnattr_t attr;
  posix_spawnattr_init (&attr);
  posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETPGROUP)
  posix_spawnattr_tcsetpgrp_np (&attr, fd);

  pid_t pid;
  char *args[] = { NULL };
  posix_spawn (&pid, "/path/to/program", NULL, &attr, args, NULL);

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

[Godmar Back]

On Fri, Jun 11, 2021 at 9:45 AM Adhemerval Zanella <
adhemerval.zanella@linaro.org> wrote:

>
> So maybe this following:
>
>   int posix_spawnattr_tcsetpgrp_np (posix_spawnattr_t *__attr, int fd);
>
>   Similar to tcgetpgrp, it make the process group of the created process
> the
>   foreground process group on the terminal associated to FD.  It is similar
>   to tcsetpgrp(), where FD must be controlling terminal of the calling
> process,
>   and still be associated with its session.
>
>   It can be used along with POSIX_SPAWN_SETPGROUP to make the created
> process
>   a new group leader to be used as the process group.
>
>
As I stated before, that would be fine, and the caller in most cases will
likely already have an open file descriptor referring to their controlling
terminal.