[PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[daurnimator]

This patch adds support for the POSIX_SPAWN_SETSID flag.

It was recently accepted by the Austin Group:
http://austingroupbugs.net/view.php?id=1044

---
 conform/data/spawn.h-data        | 1 +
 posix/spawn.h                    | 1 +
 posix/spawnattr_setflags.c       | 1 +
 sysdeps/mach/hurd/spawni.c       | 3 +++
 sysdeps/posix/spawni.c           | 7 ++++++-
 sysdeps/unix/sysv/linux/spawni.c | 4 ++++
 6 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/conform/data/spawn.h-data b/conform/data/spawn.h-data
index fb206f7ecf..bcba36c492 100644
--- a/conform/data/spawn.h-data
+++ b/conform/data/spawn.h-data
@@ -14,6 +14,7 @@ constant POSIX_SPAWN_SETSCHEDPARAM
 constant POSIX_SPAWN_SETSCHEDULER
 constant POSIX_SPAWN_SETSIGDEF
 constant POSIX_SPAWN_SETSIGMASK
+constant POSIX_SPAWN_SETSID
 
 function int posix_spawnattr_destroy (posix_spawnattr_t*)
 function int posix_spawnattr_getsigdefault (const posix_spawnattr_t*, sigset_t*)
diff --git a/posix/spawn.h b/posix/spawn.h
index 36e3867e17..8d2ace1b87 100644
--- a/posix/spawn.h
+++ b/posix/spawn.h
@@ -60,6 +60,7 @@ typedef struct
 #ifdef __USE_GNU
 # define POSIX_SPAWN_USEVFORK		0x40
 #endif
+#define POSIX_SPAWN_SETSID		0x80
 
 
 __BEGIN_DECLS
diff --git a/posix/spawnattr_setflags.c b/posix/spawnattr_setflags.c
index 9b3d1e022a..62d2f00c20 100644
--- a/posix/spawnattr_setflags.c
+++ b/posix/spawnattr_setflags.c
@@ -25,6 +25,7 @@
 		   | POSIX_SPAWN_SETSIGMASK				      \
 		   | POSIX_SPAWN_SETSCHEDPARAM				      \
 		   | POSIX_SPAWN_SETSCHEDULER				      \
+		   | POSIX_SPAWN_SETSID					      \
 		   | POSIX_SPAWN_USEVFORK)
 
 /* Store flags in the attribute structure.  */
diff --git a/sysdeps/mach/hurd/spawni.c b/sysdeps/mach/hurd/spawni.c
index 284875ac30..74303839e4 100644
--- a/sysdeps/mach/hurd/spawni.c
+++ b/sysdeps/mach/hurd/spawni.c
@@ -281,6 +281,9 @@ __spawni (pid_t *pid, const char *file,
     }
 #endif
 
+  if (!err && (flags & POSIX_SPAWN_SETSID) != 0)
+    err = __proc_setsid (proc);
+
   /* Set the process group ID.  */
   if (!err && (flags & POSIX_SPAWN_SETPGROUP) != 0)
     err = __proc_setpgrp (proc, new_pid, attrp->__pgrp);
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
index 5cc2ad1363..bc23a1e6ab 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -101,7 +101,8 @@ __spawni (pid_t *pid, const char *file,
 	 to POSIX.  */
       || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF
 		    | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER
-		    | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS)) == 0
+		    | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS
+		    | POSIX_SPAWN_SETSID)) == 0
 	  && file_actions == NULL))
     new_pid = __vfork ();
   else
@@ -159,6 +160,10 @@ __spawni (pid_t *pid, const char *file,
     }
 #endif
 
+  if ((flags & POSIX_SPAWN_SETSID) != 0
+      && __setsid () != 0)
+    _exit (SPAWN_ERROR);
+
   /* Set the process group ID.  */
   if ((flags & POSIX_SPAWN_SETPGROUP) != 0
       && __setpgid (0, attrp->__pgrp) != 0)
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index b82a5e8f3c..318c39a8af 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -177,6 +177,10 @@ __spawni_child (void *arguments)
     }
 #endif
 
+  if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
+      && (ret = __setsid ()) != 0)
+    goto fail;
+
   /* Set the process group ID.  */
   if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
       && (ret = __setpgid (0, attr->__pgrp)) != 0)
-- 
2.11.1

Re: [PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[Adhemerval Zanella]

It lacks the proper Changelog, but patch looks good.  Could you provide
one? Also, I assume you checked at least in one architecture (we usually
indicate which system we ran the patch and the results).

On 01/04/2017 11:29, daurnimator wrote:
> This patch adds support for the POSIX_SPAWN_SETSID flag.
> 
> It was recently accepted by the Austin Group:
> http://austingroupbugs.net/view.php?id=1044
> 
> ---
>  conform/data/spawn.h-data        | 1 +
>  posix/spawn.h                    | 1 +
>  posix/spawnattr_setflags.c       | 1 +
>  sysdeps/mach/hurd/spawni.c       | 3 +++
>  sysdeps/posix/spawni.c           | 7 ++++++-
>  sysdeps/unix/sysv/linux/spawni.c | 4 ++++
>  6 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/conform/data/spawn.h-data b/conform/data/spawn.h-data
> index fb206f7ecf..bcba36c492 100644
> --- a/conform/data/spawn.h-data
> +++ b/conform/data/spawn.h-data
> @@ -14,6 +14,7 @@ constant POSIX_SPAWN_SETSCHEDPARAM
>  constant POSIX_SPAWN_SETSCHEDULER
>  constant POSIX_SPAWN_SETSIGDEF
>  constant POSIX_SPAWN_SETSIGMASK
> +constant POSIX_SPAWN_SETSID
>  
>  function int posix_spawnattr_destroy (posix_spawnattr_t*)
>  function int posix_spawnattr_getsigdefault (const posix_spawnattr_t*, sigset_t*)
> diff --git a/posix/spawn.h b/posix/spawn.h
> index 36e3867e17..8d2ace1b87 100644
> --- a/posix/spawn.h
> +++ b/posix/spawn.h
> @@ -60,6 +60,7 @@ typedef struct
>  #ifdef __USE_GNU
>  # define POSIX_SPAWN_USEVFORK		0x40
>  #endif
> +#define POSIX_SPAWN_SETSID		0x80
>  
>  
>  __BEGIN_DECLS
> diff --git a/posix/spawnattr_setflags.c b/posix/spawnattr_setflags.c
> index 9b3d1e022a..62d2f00c20 100644
> --- a/posix/spawnattr_setflags.c
> +++ b/posix/spawnattr_setflags.c
> @@ -25,6 +25,7 @@
>  		   | POSIX_SPAWN_SETSIGMASK				      \
>  		   | POSIX_SPAWN_SETSCHEDPARAM				      \
>  		   | POSIX_SPAWN_SETSCHEDULER				      \
> +		   | POSIX_SPAWN_SETSID					      \
>  		   | POSIX_SPAWN_USEVFORK)
>  
>  /* Store flags in the attribute structure.  */
> diff --git a/sysdeps/mach/hurd/spawni.c b/sysdeps/mach/hurd/spawni.c
> index 284875ac30..74303839e4 100644
> --- a/sysdeps/mach/hurd/spawni.c
> +++ b/sysdeps/mach/hurd/spawni.c
> @@ -281,6 +281,9 @@ __spawni (pid_t *pid, const char *file,
>      }
>  #endif
>  
> +  if (!err && (flags & POSIX_SPAWN_SETSID) != 0)
> +    err = __proc_setsid (proc);
> +
>    /* Set the process group ID.  */
>    if (!err && (flags & POSIX_SPAWN_SETPGROUP) != 0)
>      err = __proc_setpgrp (proc, new_pid, attrp->__pgrp);
> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
> index 5cc2ad1363..bc23a1e6ab 100644
> --- a/sysdeps/posix/spawni.c
> +++ b/sysdeps/posix/spawni.c
> @@ -101,7 +101,8 @@ __spawni (pid_t *pid, const char *file,
>  	 to POSIX.  */
>        || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF
>  		    | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER
> -		    | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS)) == 0
> +		    | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS
> +		    | POSIX_SPAWN_SETSID)) == 0
>  	  && file_actions == NULL))
>      new_pid = __vfork ();
>    else
> @@ -159,6 +160,10 @@ __spawni (pid_t *pid, const char *file,
>      }
>  #endif
>  
> +  if ((flags & POSIX_SPAWN_SETSID) != 0
> +      && __setsid () != 0)
> +    _exit (SPAWN_ERROR);
> +
>    /* Set the process group ID.  */
>    if ((flags & POSIX_SPAWN_SETPGROUP) != 0
>        && __setpgid (0, attrp->__pgrp) != 0)
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index b82a5e8f3c..318c39a8af 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -177,6 +177,10 @@ __spawni_child (void *arguments)
>      }
>  #endif
>  
> +  if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
> +      && (ret = __setsid ()) != 0)
> +    goto fail;
> +
>    /* Set the process group ID.  */
>    if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
>        && (ret = __setpgid (0, attr->__pgrp)) != 0)
> 

Re: [PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[Florian Weimer]

On 04/01/2017 04:29 PM, daurnimator wrote:
> diff --git a/posix/spawn.h b/posix/spawn.h
> index 36e3867e17..8d2ace1b87 100644
> --- a/posix/spawn.h
> +++ b/posix/spawn.h
> @@ -60,6 +60,7 @@ typedef struct
>  #ifdef __USE_GNU
>  # define POSIX_SPAWN_USEVFORK		0x40
>  #endif
> +#define POSIX_SPAWN_SETSID		0x80
>

Doesn't this add the flag to past POSIX versions?

I'd prefer if there was a test case for the new functionality, perhaps 
using the getsid function.

I wonder if we should add a new symbol version for 
posix_spawnattr_setflags, so that applications which do not perform 
error checking for the function call fail in a predictable manner.

Thanks,
Florian

Re: [PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[Adhemerval Zanella]

On 03/04/2017 16:12, Florian Weimer wrote:
> On 04/01/2017 04:29 PM, daurnimator wrote:
>> diff --git a/posix/spawn.h b/posix/spawn.h
>> index 36e3867e17..8d2ace1b87 100644
>> --- a/posix/spawn.h
>> +++ b/posix/spawn.h
>> @@ -60,6 +60,7 @@ typedef struct
>>  #ifdef __USE_GNU
>>  # define POSIX_SPAWN_USEVFORK        0x40
>>  #endif
>> +#define POSIX_SPAWN_SETSID        0x80
>>
> 
> Doesn't this add the flag to past POSIX versions?

I do not think this is an issue since afaik POSIX does not state any 
constraint regarding the flags values [1].  For instance, the example
library implementation uses spawn as example and just use constant
different than glibc [2].

In fact I wish we had never added this constant functionality and 
now that it is a no-op on Linux we can start to think about signalling
it is deprecated.  One option would just remove it from default
posix implementation (and just use fork) and stop to exporting it.
New flags will still need to use 0x80 unfortunately. 

> 
> I'd prefer if there was a test case for the new functionality, perhaps using the getsid function.

It seems a good idea indeed.

> 
> I wonder if we should add a new symbol version for posix_spawnattr_setflags, so that applications which do not perform error checking for the function call fail in a predictable manner.
> 

I do not follow, which semantic difference are you proposing for
posix_spawnattr_setflags that are not covered currently?

> Thanks,
> Florian

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

Re: [PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[Florian Weimer]

On 04/03/2017 09:35 PM, Adhemerval Zanella wrote:
>
>
> On 03/04/2017 16:12, Florian Weimer wrote:
>> On 04/01/2017 04:29 PM, daurnimator wrote:
>>> diff --git a/posix/spawn.h b/posix/spawn.h
>>> index 36e3867e17..8d2ace1b87 100644
>>> --- a/posix/spawn.h
>>> +++ b/posix/spawn.h
>>> @@ -60,6 +60,7 @@ typedef struct
>>>  #ifdef __USE_GNU
>>>  # define POSIX_SPAWN_USEVFORK        0x40
>>>  #endif
>>> +#define POSIX_SPAWN_SETSID        0x80
>>>
>>
>> Doesn't this add the flag to past POSIX versions?
>
> I do not think this is an issue since afaik POSIX does not state any
> constraint regarding the flags values [1].  For instance, the example
> library implementation uses spawn as example and just use constant
> different than glibc [2].

Sorry, this is not what I meant.  I was wondering if it was acceptable, 
from a namespace point of view, to define the constant unconditionally, 
or if we have to use a feature test macro here.

>> I wonder if we should add a new symbol version for posix_spawnattr_setflags, so that applications which do not perform error checking for the function call fail in a predictable manner.
>>
>
> I do not follow, which semantic difference are you proposing for
> posix_spawnattr_setflags that are not covered currently?

I'm worried about applications which ignore the error return value from 
posix_spawnattr_setflags, use POSIX_SPAWN_SETSID, and accidentally spawn 
processes with the wrong flags.

Thanks,
Florian

Re: [PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[Szabolcs Nagy]

On 04/04/17 10:38, Florian Weimer wrote:
> On 04/03/2017 09:35 PM, Adhemerval Zanella wrote:
>> On 03/04/2017 16:12, Florian Weimer wrote:
>>> On 04/01/2017 04:29 PM, daurnimator wrote:
>>>> diff --git a/posix/spawn.h b/posix/spawn.h
>>>> index 36e3867e17..8d2ace1b87 100644
>>>> --- a/posix/spawn.h
>>>> +++ b/posix/spawn.h
>>>> @@ -60,6 +60,7 @@ typedef struct
>>>>  #ifdef __USE_GNU
>>>>  # define POSIX_SPAWN_USEVFORK        0x40
>>>>  #endif
>>>> +#define POSIX_SPAWN_SETSID        0x80
>>>>
>>>
>>> Doesn't this add the flag to past POSIX versions?
>>
>> I do not think this is an issue since afaik POSIX does not state any
>> constraint regarding the flags values [1].  For instance, the example
>> library implementation uses spawn as example and just use constant
>> different than glibc [2].
> 
> Sorry, this is not what I meant.  I was wondering if it was acceptable, from a namespace point of view, to
> define the constant unconditionally, or if we have to use a feature test macro here.
> 

POSIX_* is reserved so there is no namespace issue,
but it can be conditional if glibc wants to be
strict about what is visible under different
posix versions (i don't think that is useful here).

Re: [PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[Florian Weimer]

On 04/04/2017 12:41 PM, Szabolcs Nagy wrote:
> On 04/04/17 10:38, Florian Weimer wrote:
>> On 04/03/2017 09:35 PM, Adhemerval Zanella wrote:
>>> On 03/04/2017 16:12, Florian Weimer wrote:
>>>> On 04/01/2017 04:29 PM, daurnimator wrote:
>>>>> diff --git a/posix/spawn.h b/posix/spawn.h
>>>>> index 36e3867e17..8d2ace1b87 100644
>>>>> --- a/posix/spawn.h
>>>>> +++ b/posix/spawn.h
>>>>> @@ -60,6 +60,7 @@ typedef struct
>>>>>  #ifdef __USE_GNU
>>>>>  # define POSIX_SPAWN_USEVFORK        0x40
>>>>>  #endif
>>>>> +#define POSIX_SPAWN_SETSID        0x80
>>>>>
>>>>
>>>> Doesn't this add the flag to past POSIX versions?
>>>
>>> I do not think this is an issue since afaik POSIX does not state any
>>> constraint regarding the flags values [1].  For instance, the example
>>> library implementation uses spawn as example and just use constant
>>> different than glibc [2].
>>
>> Sorry, this is not what I meant.  I was wondering if it was acceptable, from a namespace point of view, to
>> define the constant unconditionally, or if we have to use a feature test macro here.
>>
>
> POSIX_* is reserved so there is no namespace issue,
> but it can be conditional if glibc wants to be
> strict about what is visible under different
> posix versions (i don't think that is useful here).

Okay, I just wasn't sure about that.  Thanks for clarifying.

Florian

Re: [PATCH][BZ 21340] add support for POSIX_SPAWN_SETSID

[Daurnimator]

(replying to self as I'm not subbed to list, and only by chance saw
Adhemerval Zanella's reply via patchwork)

> It lacks the proper Changelog, but patch looks good.  Could you provide
> one? Also, I assume you checked at least in one architecture (we usually
> indicate which system we ran the patch and the results).

Will send over new patch.
Tested on linux on x64.
I've also now written a test case for POSIX_SPAWN_SETSID.