[v3,BZ,21340] add support for POSIX_SPAWN_SETSID

Submitted by Daurnimator on April 5, 2017, 5:41 a.m.

Details

Message ID 20170405054116.9007-1-quae@daurnimator.com
State New
Headers show

Commit Message

Daurnimator April 5, 2017, 5:41 a.m.
I forgot to actually run my new test when I sent in v2.
Turns out I made a terrible mistake and didn't actually have the right
result check in the linux code. (setsid returns the new sid on success; not 0)

--- >8 ---
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

Checked on x86_64

2017-04-05  Daurnimator  <quae@daurnimator.com>

    [BZ #21340]
    * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag.
    * posix/Makefile: Add tst-posix_spawn-setsid to list of tests.
    * posix/spawn.h: define POSIX_SPAWN_SETSID flag.
    * posix/spawnattr_setflags.c: Add POSIX_SPAWN_SETSID to valid flags.
    * posix/tst-posix_spawn-setsid.c: Add test for POSIX_SPAWN_SETSID.
    * sysdeps/mach/hurd/spawni.c: Implementation of POSIX_SPAWN_SETSID.
    * sysdeps/posix/spawni.c: Likewise.
    * sysdeps/unix/sysv/linux/spawni.c: Likewise.
---
 conform/data/spawn.h-data        |  1 +
 posix/Makefile                   |  2 +-
 posix/spawn.h                    |  1 +
 posix/spawnattr_setflags.c       |  1 +
 posix/tst-posix_spawn-setsid.c   | 70 ++++++++++++++++++++++++++++++++++++++++
 sysdeps/mach/hurd/spawni.c       |  3 ++
 sysdeps/posix/spawni.c           |  7 +++-
 sysdeps/unix/sysv/linux/spawni.c |  4 +++
 8 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100644 posix/tst-posix_spawn-setsid.c

Comments

Florian Weimer April 5, 2017, 8:34 a.m.
On 04/05/2017 07:41 AM, daurnimator wrote:

> +static int
> +do_test (void)
> +{
> +  posix_spawnattr_t attrp;
> +  int res;
> +  int child_pid;
> +  int sid, child_sid;
> +  char *args[2];

Thanks for writing the test.  There are some style issues.  We prefer 
in-line declarations these days, and there should be a space before “(” 
in a function call.

Please use the new test skeleton in <support/test-driver.c>.  (Sorry, 
still need to update the wiki.)

> +  posix_spawnattr_init(&attrp);
> +  if (posix_spawnattr_setflags(&attrp, POSIX_SPAWN_SETSID))
> +    {
> +      printf("error: posix_spawnattr_setflags: %m\n");

You need to capture the return value of posix_spawnattr_setflags and 
assign it to errno prior to using %m.  You can include <support/check.h> 
and use FAIL_EXIT1.

> +  /* run the program 'true' */
> +  args[0] = (char *)"true";
> +  args[1] = NULL;
> +
> +  res = posix_spawnp(&child_pid, "true", NULL, &attrp, args, environ);
> +  posix_spawnattr_destroy(&attrp);
> +  if (res)
> +    {
> +      printf("error: posix_spawnp: %m\n");

See above, needs to set errno.

> +      return 1;
> +    }
> +
> +  /* child should have a different sid */
> +  child_sid = getsid(child_pid);

Please add error checking for the getsid result.

> +  if (child_sid == sid)
> +    {
> +      printf("error: child sid matches\n");
> +      return 1;
> +    }

I think you should run the test case twice, and whether 
POSIX_SPAWN_SETSID is applied should be controlled by a flag.  Then you 
can check if the getsid result remains unchanged or is changed, as 
appropriate.

You aren't listed in MAINTAINERS on the wiki.  Do you have a GNU 
copyright assignment on file?

Thanks,
Florian
Daurnimator April 5, 2017, 11:01 a.m.
On 5 April 2017 at 18:34, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/05/2017 07:41 AM, daurnimator wrote:
>
>> +static int
>> +do_test (void)
>> +{
>> +  posix_spawnattr_t attrp;
>> +  int res;
>> +  int child_pid;
>> +  int sid, child_sid;
>> +  char *args[2];
>
>
> Thanks for writing the test.  There are some style issues.  We prefer
> in-line declarations these days

Do you mean putting 'static int do_test (void) {' on one line?

> and there should be a space before “(” in a
> function call.

There already was?

> Please use the new test skeleton in <support/test-driver.c>.  (Sorry, still
> need to update the wiki.)

I just worked from the existing test 'tst-posix_spawn-fd.c'.

> You aren't listed in MAINTAINERS on the wiki.  Do you have a GNU copyright
> assignment on file?

I do not.
Joseph S. Myers April 5, 2017, 12:31 p.m.
On Wed, 5 Apr 2017, daurnimator wrote:

> 2017-04-05  Daurnimator  <quae@daurnimator.com>
> 
>     [BZ #21340]
>     * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag.

You should never add a symbol to the conform/ expectations unless it is 
actually defined by a released standard.  In a case like this, of a symbol 
intended to be added in a future version, it should not be in the conform/ 
data, and should be conditioned on __USE_GNU in the headers.

Note that http://austingroupbugs.net/view.php?id=1044 has the issue8 tag, 
meaning it's for the next major revision of POSIX, not a bug fix for a TC 
for the current revision.
Joseph S. Myers April 5, 2017, 12:43 p.m.
Also, new user-visible features should have a NEWS entry explaining the 
feature (which might in this case say it's accepted for the next major 
revision of POSIX, as well as something about what it does).
Szabolcs Nagy April 5, 2017, 12:58 p.m.
On 05/04/17 13:31, Joseph Myers wrote:
> On Wed, 5 Apr 2017, daurnimator wrote:
>> 2017-04-05  Daurnimator  <quae@daurnimator.com>
>>
>>     [BZ #21340]
>>     * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag.
> 
> You should never add a symbol to the conform/ expectations unless it is 
> actually defined by a released standard.  In a case like this, of a symbol 
> intended to be added in a future version, it should not be in the conform/ 
> data, and should be conditioned on __USE_GNU in the headers.

i dont see why the __USE_GNU is necessary.
Joseph S. Myers April 5, 2017, 3:02 p.m.
On Wed, 5 Apr 2017, Szabolcs Nagy wrote:

> On 05/04/17 13:31, Joseph Myers wrote:
> > On Wed, 5 Apr 2017, daurnimator wrote:
> >> 2017-04-05  Daurnimator  <quae@daurnimator.com>
> >>
> >>     [BZ #21340]
> >>     * conform/data/spawn.h-data: Add POSIX_SPAWN_SETSID flag.
> > 
> > You should never add a symbol to the conform/ expectations unless it is 
> > actually defined by a released standard.  In a case like this, of a symbol 
> > intended to be added in a future version, it should not be in the conform/ 
> > data, and should be conditioned on __USE_GNU in the headers.
> 
> i dont see why the __USE_GNU is necessary.

Although there are a few exceptions (e.g. errno numbers), common practice 
in glibc is not to rely on reserved namespaces other than _[_A-Z]* and to 
declare symbols only for the relevant standards (and likewise for 
link-time namespace).
Florian Weimer April 6, 2017, 12:37 p.m.
On 04/05/2017 01:01 PM, Daurnimator wrote:

>> You aren't listed in MAINTAINERS on the wiki.  Do you have a GNU copyright
>> assignment on file?
>
> I do not.

Would you be willing to assign copyright for this and future changes?

If yes, please pick request-assign.future from here:

<https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>

and send the filled-out form to <assign@gnu.org>.  As far as I 
understand it, citizens from most countries can complete the process 
electronically these days.

Thanks,
Florian
Zack Weinberg April 6, 2017, 1:22 p.m.
On Thu, Apr 6, 2017 at 8:37 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/05/2017 01:01 PM, Daurnimator wrote:
>
>>> You aren't listed in MAINTAINERS on the wiki.  Do you have a GNU
>>> copyright
>>> assignment on file?
>>
>> I do not.
>
> Would you be willing to assign copyright for this and future changes?

It would obviously be better to have a copyright assignment, but this
particular patch may be uncopyrightable on the grounds that there is
no other way to implement the specification.  I think if I had been
given the assignment I would have written token-for-token identical
code.

zw
Florian Weimer April 6, 2017, 1:22 p.m.
On 04/06/2017 03:22 PM, Zack Weinberg wrote:
> On Thu, Apr 6, 2017 at 8:37 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 04/05/2017 01:01 PM, Daurnimator wrote:
>>
>>>> You aren't listed in MAINTAINERS on the wiki.  Do you have a GNU
>>>> copyright
>>>> assignment on file?
>>>
>>> I do not.
>>
>> Would you be willing to assign copyright for this and future changes?
>
> It would obviously be better to have a copyright assignment, but this
> particular patch may be uncopyrightable on the grounds that there is
> no other way to implement the specification.  I think if I had been
> given the assignment I would have written token-for-token identical
> code.

This argument does not apply to the test case.

Thanks,
Florian
Zack Weinberg April 6, 2017, 1:24 p.m.
On Thu, Apr 6, 2017 at 9:22 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/06/2017 03:22 PM, Zack Weinberg wrote:
>>
>> It would obviously be better to have a copyright assignment, but this
>> particular patch may be uncopyrightable on the grounds that there is
>> no other way to implement the specification.  I think if I had been
>> given the assignment I would have written token-for-token identical
>> code.
>
> This argument does not apply to the test case.

Oh, good point, I didn't notice that there was a test case in the revised patch.

zw
Daurnimator April 7, 2017, 12:27 p.m.
On 6 April 2017 at 23:24, Zack Weinberg <zackw@panix.com> wrote:
> On Thu, Apr 6, 2017 at 9:22 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 04/06/2017 03:22 PM, Zack Weinberg wrote:
>>>
>>> It would obviously be better to have a copyright assignment, but this
>>> particular patch may be uncopyrightable on the grounds that there is
>>> no other way to implement the specification.  I think if I had been
>>> given the assignment I would have written token-for-token identical
>>> code.
>>
>> This argument does not apply to the test case.
>
> Oh, good point, I didn't notice that there was a test case in the revised patch.
>
> zw

FWIW I'd find it easier to resubmit the patch without the test case
than to fill out the copyright assignment...
Zack Weinberg April 7, 2017, 1:46 p.m.
On Fri, Apr 7, 2017 at 8:27 AM, Daurnimator <quae@daurnimator.com> wrote:
> On 6 April 2017 at 23:24, Zack Weinberg <zackw@panix.com> wrote:
>> Oh, good point, I didn't notice that there was a test case in the revised patch.
>
> FWIW I'd find it easier to resubmit the patch without the test case
> than to fill out the copyright assignment...

I understand that the paperwork is a pain, but we really do want the test case.

zw
Daurnimator April 9, 2017, 1:21 p.m.
On 6 April 2017 at 22:37, Florian Weimer <fweimer@redhat.com> wrote:
> Would you be willing to assign copyright for this and future changes?
>
> If yes, please pick request-assign.future from here:
>
> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
>
> and send the filled-out form to <assign@gnu.org>.  As far as I understand
> it, citizens from most countries can complete the process electronically
> these days.

Sorry, but I'm not willing to link my legal name to this pseudonym.

Please still consider merging my changes (as they should be uncopyrightable).
However as I understand it, you will have to re-write the test case.
Daurnimator April 21, 2017, 5:03 a.m.
On 9 April 2017 at 23:21, Daurnimator <quae@daurnimator.com> wrote:
> On 6 April 2017 at 22:37, Florian Weimer <fweimer@redhat.com> wrote:
>> Would you be willing to assign copyright for this and future changes?
>>
>> If yes, please pick request-assign.future from here:
>>
>> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
>>
>> and send the filled-out form to <assign@gnu.org>.  As far as I understand
>> it, citizens from most countries can complete the process electronically
>> these days.
>
> Sorry, but I'm not willing to link my legal name to this pseudonym.
>
> Please still consider merging my changes (as they should be uncopyrightable).
> However as I understand it, you will have to re-write the test case.

I'd still love to get this patch merged; is someone willing to rewrite
the test case?
Adhemerval Zanella April 21, 2017, 12:39 p.m.
On 21/04/2017 02:03, Daurnimator wrote:
> On 9 April 2017 at 23:21, Daurnimator <quae@daurnimator.com> wrote:
>> On 6 April 2017 at 22:37, Florian Weimer <fweimer@redhat.com> wrote:
>>> Would you be willing to assign copyright for this and future changes?
>>>
>>> If yes, please pick request-assign.future from here:
>>>
>>> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
>>>
>>> and send the filled-out form to <assign@gnu.org>.  As far as I understand
>>> it, citizens from most countries can complete the process electronically
>>> these days.
>>
>> Sorry, but I'm not willing to link my legal name to this pseudonym.
>>
>> Please still consider merging my changes (as they should be uncopyrightable).
>> However as I understand it, you will have to re-write the test case.
> 
> I'd still love to get this patch merged; is someone willing to rewrite
> the test case?
> 

I will work on this.

Patch hide | download patch | download mbox

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/Makefile b/posix/Makefile
index ae17646323..a9f53a973d 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -90,7 +90,7 @@  tests		:= test-errno tstgetopt testfnm runtests runptests \
 		   bug-getopt5 tst-getopt_long1 bug-regex34 bug-regex35 \
 		   tst-pathconf tst-getaddrinfo4 tst-rxspencer-no-utf8 \
 		   tst-fnmatch3 bug-regex36 tst-getaddrinfo5 \
-		   tst-posix_spawn-fd \
+		   tst-posix_spawn-fd tst-posix_spawn-setsid \
 		   tst-posix_fadvise tst-posix_fadvise64
 xtests		:= bug-ga2
 ifeq (yes,$(build-shared))
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/posix/tst-posix_spawn-setsid.c b/posix/tst-posix_spawn-setsid.c
new file mode 100644
index 0000000000..b3cd5e54b1
--- /dev/null
+++ b/posix/tst-posix_spawn-setsid.c
@@ -0,0 +1,70 @@ 
+/* Test that spawn attribute setsid works.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <spawn.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <sys/resource.h>
+#include <unistd.h>
+
+static int
+do_test (void)
+{
+  posix_spawnattr_t attrp;
+  int res;
+  int child_pid;
+  int sid, child_sid;
+  char *args[2];
+
+  /* record sid for current process */
+  sid = getsid(0);
+
+  posix_spawnattr_init(&attrp);
+  if (posix_spawnattr_setflags(&attrp, POSIX_SPAWN_SETSID))
+    {
+      printf("error: posix_spawnattr_setflags: %m\n");
+      return 1;
+    }
+
+  /* run the program 'true' */
+  args[0] = (char *)"true";
+  args[1] = NULL;
+
+  res = posix_spawnp(&child_pid, "true", NULL, &attrp, args, environ);
+  posix_spawnattr_destroy(&attrp);
+  if (res)
+    {
+      printf("error: posix_spawnp: %m\n");
+      return 1;
+    }
+
+  /* child should have a different sid */
+  child_sid = getsid(child_pid);
+  if (child_sid == sid)
+    {
+      printf("error: child sid matches\n");
+      return 1;
+    }
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
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..9cad25ca4e 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 d7f9e83575..3cf77d50c3 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)