[Bug libc/28730] New: Extend posix_spawnattr_* interface with more useful features

[Bug libc/28730] New: Extend posix_spawnattr_* interface with more useful features

[crrodriguez at opensuse dot org]

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

            Bug ID: 28730
           Summary: Extend posix_spawnattr_* interface with more useful
                    features
           Product: glibc
           Version: unspecified
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: crrodriguez at opensuse dot org
                CC: drepper.fsp at gmail dot com
  Target Milestone: ---

I have reviewed a number of software to possible make them use posix_spawn
interfaces and found a few missing features, (other than the terminal control
stuff whicb I am aware it is being worked on) I do not expect all of this
suggestions to be implemented and I know it is not intended as a 100%
fork/execve replacement

The first one .. if I:

posix_spawn(&child_pid, "/bin/sleep" , NULL, NULL, (char *const[]) { "sleep",
"1000", NULL}, environ)

and kill the process the invoked the function, the orphan keeps running as good
ole unix behaviour mandates, but this behaviour is highly undersirable except
when you are spawning long running process or you are sure it will not do
anything evil unsupervised by the calling process. 

Therefore I propose a linux only extension (unless the hurd has it too?) with
prototype:

posix_spawnattr_setpdeathsig_np(posix_spawnattr_t *attr, int deathsignal)

that escentially calls 

prctl(PR_SET_PDEATHSIG, deathsignal);

in the "child" process.

Unless standards say otherwise, there shall be no default death signal
configured.


Second feature request will be having an equivalent to pthread_setname_np as 
posix_spawnattr_setname_np which does the same as pthread_setname_np but on
this case for the "child" process..

Unless the standards say otherwise there shall be no default name set, but it
will be nice if glibc tags its own created processes with for example a
(system), (popen), (wordexp) name, but this is just a nice to have conveniance
for humans that are looking the process table..


Third is a way to set the "child" rlimit, like
posix_spawnattr_setrlimit_np(posix_spawnattr_t *attr, int resource, struct
rlimit *rlim)

parent may be running with a high RLIMIT_NOFILE but execute a program that uses
select() and things will end badly when RLIMIT_NOFILE > FD_SETSIZE.

I could try to implement this but Im not sure how I should exactly extend
posix_spawnattr_t in a binary compatible way..

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[crrodriguez at opensuse dot org]

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

--- Comment #1 from Cristian Rodríguez <crrodriguez at opensuse dot org> ---
The other way of course would be to just have a just-before-exec callback,
clearly documenting what can be done there and everything else is
undefined/evil/"will bring the demise of the human race"

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[adhemerval.zanella at linaro dot org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |adhemerval.zanella at linaro dot o
                   |                            |rg

--- Comment #2 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Cristian Rodríguez from comment #0)
> I have reviewed a number of software to possible make them use posix_spawn
> interfaces and found a few missing features, (other than the terminal
> control stuff whicb I am aware it is being worked on) I do not expect all of
> this suggestions to be implemented and I know it is not intended as a 100%
> fork/execve replacement
> 
> The first one .. if I:
> 
> posix_spawn(&child_pid, "/bin/sleep" , NULL, NULL, (char *const[]) {
> "sleep", "1000", NULL}, environ)
> 
> and kill the process the invoked the function, the orphan keeps running as
> good ole unix behaviour mandates, but this behaviour is highly undersirable
> except when you are spawning long running process or you are sure it will
> not do anything evil unsupervised by the calling process. 
> 
> Therefore I propose a linux only extension (unless the hurd has it too?)
> with prototype:
> 
> posix_spawnattr_setpdeathsig_np(posix_spawnattr_t *attr, int deathsignal)
> 
> that escentially calls 
> 
> prctl(PR_SET_PDEATHSIG, deathsignal);

I think it makes sense, although it has the the inherent issue with the
set-user-ID or set-group-ID binary and it would require a lot of funcionality
to make it work (for instance, a possible solution is described
https://stackoverflow.com/questions/42496478/prctlpr-set-pdeathsig-race-condition),
that is also out of the scope of posix_spawn.

I checked systemd code and it does seem to use PR_SET_PDEATHSIG as default. 
However, it also implements a 'safe_fork' that issues clone directly with extra
arguments to handle the pid and filesystem namespace creation.  It makes me
wonder if usual posix_spawn usage will really benefit with a Linux specific
feature, since it aims to be a potable solution (although other POSIX systems
do add a lot of extensions).

> 
> in the "child" process.
> 
> Unless standards say otherwise, there shall be no default death signal
> configured.
> 
> 
> Second feature request will be having an equivalent to pthread_setname_np as 
> posix_spawnattr_setname_np which does the same as pthread_setname_np but on
> this case for the "child" process..
> 
> Unless the standards say otherwise there shall be no default name set, but
> it will be nice if glibc tags its own created processes with for example a
> (system), (popen), (wordexp) name, but this is just a nice to have
> conveniance for humans that are looking the process table..

It would require to alloc/free the name itself (to avoid waste space on
posix_spawnattr_t), but I think it should be feasible.

But again, it this a feature that would be really used? Linux limits the name
to just TASK_COMM_LEN (16), which makes me doubt you can pack really useful
information.

> 
> 
> Third is a way to set the "child" rlimit, like
> posix_spawnattr_setrlimit_np(posix_spawnattr_t *attr, int resource, struct
> rlimit *rlim)
> 
> parent may be running with a high RLIMIT_NOFILE but execute a program that
> uses select() and things will end badly when RLIMIT_NOFILE > FD_SETSIZE.

The rlimit does make sense to me, QNX has posix_spawnattr_setstackmax for
instance.

> 
> I could try to implement this but Im not sure how I should exactly extend
> posix_spawnattr_t in a binary compatible way..

Best option would to use the __pad space member space from posix_spawnattr_t. 
To PR_SET_PDEATHSIG it would be a matter of add another field and decrease
__pad size.

For the new process name and rlimit I think it would be better add extra points
where the posix_spawnattr_* would allocate memory and posix_spawnattr_destroy
would free them.

The rlimit would require list of resources to apply on spawned processed, I
would go allocate a vector of an initial size and reallocate by doubling it
size. 

The only issue would be to add the newer pointers fields without changing
posix_spawnattr_t, since the internal fields alignment might require to use a
different value for the __pad size.

> The other way of course would be to just have a just-before-exec callback,
> clearly documenting what can be done there and everything else is 
>  undefined/evil/"will bring the demise of the human race"

Florian has suggested it some time ago, but I not really sure about it.  Even
now users still use non-async-safe functions on signal handler or after fork
(such as malloc) that only work due an implementation detail on GLIBC. I really
like to avoid it, even though it would require to add multiple posix_spawn
non-portable extensions. The posix_spawn helper process at Linux is really
tricky because it uses CLONE_VM, so we will need to evaluate exaclty what is
allowed to be called in such context which I think it not worth the trouble.

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[crrodriguez at opensuse dot org]

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

--- Comment #3 from Cristian Rodríguez <crrodriguez at opensuse dot org> ---
do you know by any chance why it was decided to make posix_spawnattr_t and
posix_spawn_file_actions_t transparent ? posix says otherwise..

"for extensibility and consistency with the newer POSIX interfaces, the
attributes interface has been changed to an opaque data type. This interface
now consists of the type posix_spawnattr_t.."  

MacOS and opebsd at least do it this way..

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[adhemerval.zanella at linaro dot org]

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

--- Comment #4 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Cristian Rodríguez from comment #3)
> do you know by any chance why it was decided to make posix_spawnattr_t and
> posix_spawn_file_actions_t transparent ? posix says otherwise..
> 
> "for extensibility and consistency with the newer POSIX interfaces, the
> attributes interface has been changed to an opaque data type. This interface
> now consists of the type posix_spawnattr_t.."  
> 
> MacOS and opebsd at least do it this way..

POSIX states posix_spawnattr_t and posix_spawn_file_actions_t must be actual
types, it does not imposes any restriction to on how it is actually
implemented. The example provided does use a struct with some internal fields
[1].

But the current definition does provide a lot of room to extensability, since
we can embedded extra internal points if required.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap03.html

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[crrodriguez at opensuse dot org]

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

--- Comment #5 from Cristian Rodríguez <crrodriguez at opensuse dot org> ---
(In reply to Adhemerval Zanella from comment #4)
> (In reply to Cristian Rodríguez from comment #3)
> > do you know by any chance why it was decided to make posix_spawnattr_t and
> > posix_spawn_file_actions_t transparent ? posix says otherwise..
> > 
> > "for extensibility and consistency with the newer POSIX interfaces, the
> > attributes interface has been changed to an opaque data type. This interface
> > now consists of the type posix_spawnattr_t.."  
> > 
> > MacOS and opebsd at least do it this way..
> 
> POSIX states posix_spawnattr_t and posix_spawn_file_actions_t must be actual
> types, it does not imposes any restriction to on how it is actually
> implemented. The example provided does use a struct with some internal
> fields [1].
> 


So the best way way would be to use the pad space to introduce a an internal
opaque struct  which we can modify or extend at will ? even with all
implementation-defined leisure we still shouldn't break the ABI..

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[adhemerval.zanella at linaro dot org]

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

--- Comment #6 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Cristian Rodríguez from comment #5)
> So the best way way would be to use the pad space to introduce a an internal
> opaque struct  which we can modify or extend at will ? even with all
> implementation-defined leisure we still shouldn't break the ABI..

Yes, although I would first use the current space available and keep space for
a pointer to allow extend the functionaly.  Something like:

  typedef struct
  {
    short int __flags;
    pid_t __pgrp;
    sigset_t __sd;
    sigset_t __ss;
    struct sched_param __sp;
    int __policy;
    /* Add newer fields here.  */
    int __pad[N];
    void *__reserved/
  } posix_spawnattr_t;

So for posix_spawnattr_setpdeathsig_np it would be something like:

  typedef struct
  {
    short int __flags;
    pid_t __pgrp;
    sigset_t __sd;
    sigset_t __ss;
    struct sched_param __sp;
    int __policy;
    int __deathsignal;
    int __pad[M];
  } posix_spawnattr_t;

With M adjusted to keep the current size. For posix_spawnattr_setrlimit_np I
would do:

  typedef struct
  {
    short int __flags;
    pid_t __pgrp;
    sigset_t __sd;
    sigset_t __ss;
    struct sched_param __sp;
    int __policy;
    int __deathsignal;
    void *__resource;
    int __pad[M];
  } posix_spawnattr_t;

And internally '__resource' will be array of:

  struct posix_spawn_resource
  {
    int resource;
    struct rlimit64 rlim;
  }

(The rlimit has the LFS issue, so I think it shold be supported only for LFS
mode).

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[fweimer at redhat dot com]

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

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-
                 CC|                            |fweimer at redhat dot com

--- Comment #7 from Florian Weimer <fweimer at redhat dot com> ---
Adding a void * could increase alignment on some architectures, so we'd have to
use a different mechanism for that.  We can store a (potentially unaligned)
pointer field in the current padding and access it using memcpy, for example.

We should definitely not change the struct size to implement this.

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[crrodriguez at opensuse dot org]

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

--- Comment #8 from Cristian Rodríguez <crrodriguez at opensuse dot org> ---

>   struct posix_spawn_resource
>   {
>     int resource;
>     struct rlimit64 rlim;
>   }
> 
> (The rlimit has the LFS issue, so I think it shold be supported only for LFS
> mode).


The closer one looks.. :-) we should do it like QNX does..

int posix_spawnattr_setstackmax(
       posix_spawnattr_t *attrp,
       a_reasonable_non_lfs_dependant_type (uint64_t?) size);

sets   RLIMIT_STACK. _setfilemax() or similar sets NOFILE..and so on..only add
those that are useful in many cases.. unfortunately this is the only sane way I
see forward, it makes the interface even more annoyingly verbose but IMHO we
shouldn't make this functions to take LFS dependant types at all.

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

[Bug libc/28730] Extend posix_spawnattr_* interface with more useful features

[adhemerval.zanella at linaro dot org]

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

--- Comment #9 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Florian Weimer from comment #7)
> Adding a void * could increase alignment on some architectures, so we'd have
> to use a different mechanism for that.  We can store a (potentially
> unaligned) pointer field in the current padding and access it using memcpy,
> for example.
> 
> We should definitely not change the struct size to implement this.

That's why I wrote on a previous comment:

> The only issue would be to add the newer pointers fields without changing 
> posix_spawnattr_t, since the internal fields alignment might require to use a 
> different value for the __pad size.

Since we can't use offsetof on spawn.h (since it will be a namespace
violation), 
I think it would be better to use something similar to what we do already for 
some pthread structs:

  #define __SIZEOF_POSIX_SPAWNATTR_T 336

  typedef struct
  {
    char __size[__SIZEOF_POSIX_SPAWNATTR_T];
    long int __align;
  } posix_spawnattr_t;

The __SIZEOF_POSIX_SPAWNATTR_T will need to be arch-specific since m68k
aligment 
oddity makes the posix_spawnattr_t size 332 instead.

Then internally we will have:

  struct posix_spawnattr
  { 
    short int __flags;
    pid_t __pgrp;
    sigset_t __sd;
    sigset_t __ss;
    struct sched_param __sp;
    int __policy;
    int __deathsignal;
    char *__procname;
    void *__resource;
    void *__extension;
  };

  _Static_assert (sizeof (struct posix_spawnattr) <= sizeof
(posix_spawnattr_t),
                "sizeof (struct posix_spawnattr) > sizeof
(posix_spawnattr_t)");

Once the _Static_assert starts to fail we will need use '__extension'.

> The closer one looks.. :-) we should do it like QNX does..
> 
> int posix_spawnattr_setstackmax(
>        posix_spawnattr_t *attrp,
>        a_reasonable_non_lfs_dependant_type (uint64_t?) size);
> 
> sets   RLIMIT_STACK. _setfilemax() or similar sets NOFILE..and so on..only add > those that are useful in many cases.. unfortunately this is the only sane way > I see forward, it makes the interface even more annoyingly verbose but IMHO we > shouldn't make this functions to take LFS dependant types at all.

I am not very found of adding one interface to each resource, since we already
have a current map to it (used by getrlimit/setrlimit/prlimit).  Also using 
uint64_t is not possible due name polution (so we will need to either create
a new type, or have different prototypes depending of the wordsize), and 
we will need to map all current rlimit members (POSIX states the minimum
member, so any new member would require a new symbol).

And taking in consideration that LFS is deprecated and that we already added
newer interfaces that are LFS-only (getdents64), I think

  int posix_spawnattr_setrlimit_np (posix_spawnattr_t *attrp, int resource, 
                                    const struct rlimit64 *rlim);

makes even more sense.

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