[1/2] posix: Correctly enable/disable cancellation on Linux posix_spawn

  This patch correctly enable and disable asynchronous cancellation on
Linux posix_spawn.  Current code invert the logic by enabling and
disabling instead.  A new test that checks if posix_spawn is not in fact
a cancellation entrypoint is added.

Checked on x86_64, i686, powerpc64le, and aarch64.

	* nptl/Makefile (tests): Add tst-exec5.
	* nptl/tst-exec5.c: New file.
	* sysdeps/unix/sysv/linux/spawni.c (__spawni): Correctly enable and disable
	asynchronous cancellation.
---
 nptl/Makefile                    |   2 +-
 nptl/tst-exec5.c                 | 200 +++++++++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/spawni.c |   6 +-
 4 files changed, 212 insertions(+), 3 deletions(-)
 create mode 100644 nptl/tst-exec5.c
  

If no one opposes I will commit it shortly.

On 14/09/2016 15:56, Adhemerval Zanella wrote:
> This patch correctly enable and disable asynchronous cancellation on
> Linux posix_spawn.  Current code invert the logic by enabling and
> disabling instead.  A new test that checks if posix_spawn is not in fact
> a cancellation entrypoint is added.
> 
> Checked on x86_64, i686, powerpc64le, and aarch64.
> 
> 	* nptl/Makefile (tests): Add tst-exec5.
> 	* nptl/tst-exec5.c: New file.
> 	* sysdeps/unix/sysv/linux/spawni.c (__spawni): Correctly enable and disable
> 	asynchronous cancellation.
> ---
>  nptl/Makefile                    |   2 +-
>  nptl/tst-exec5.c                 | 200 +++++++++++++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/spawni.c |   6 +-
>  4 files changed, 212 insertions(+), 3 deletions(-)
>  create mode 100644 nptl/tst-exec5.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 2ddcd2b..390e1f7 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -268,7 +268,7 @@ tests = tst-typesizes \
>  	tst-flock1 tst-flock2 \
>  	tst-signal1 tst-signal2 tst-signal3 tst-signal4 tst-signal5 \
>  	tst-signal6 tst-signal7 \
> -	tst-exec1 tst-exec2 tst-exec3 tst-exec4 \
> +	tst-exec1 tst-exec2 tst-exec3 tst-exec4 tst-exec5 \
>  	tst-exit1 tst-exit2 tst-exit3 \
>  	tst-stdio1 tst-stdio2 \
>  	tst-stack1 tst-stack2 tst-stack3 tst-stack4 tst-pthread-getattr \
> diff --git a/nptl/tst-exec5.c b/nptl/tst-exec5.c
> new file mode 100644
> index 0000000..5527b23
> --- /dev/null
> +++ b/nptl/tst-exec5.c
> @@ -0,0 +1,200 @@
> +/* Check if posix_spawn does not act as a cancellation entrypoint.
> +   Copyright (C) 2016 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 <paths.h>
> +#include <pthread.h>
> +#include <signal.h>
> +#include <spawn.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
> +
> +static pthread_barrier_t b;
> +
> +static struct thread_args
> +{
> +  pid_t pid;
> +  int fd[2];
> +} targ;
> +
> +static void *
> +tf (void *arg)
> +{
> +  int e = pthread_barrier_wait (&b);
> +  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
> +    {
> +      puts ("error: pthread_barrier_wait failed");
> +      exit (1);
> +    }
> +
> +  posix_spawn_file_actions_t a;
> +  if (posix_spawn_file_actions_init (&a) != 0)
> +    {
> +      puts ("error: spawn_file_actions_init failed");
> +      exit (1);
> +    }
> +
> +  if (posix_spawn_file_actions_adddup2 (&a, targ.fd[1], STDOUT_FILENO) != 0)
> +    {
> +      puts ("error: spawn_file_actions_adddup2 failed");
> +      exit (1);
> +    }
> +
> +  if (posix_spawn_file_actions_addclose (&a, targ.fd[0]) != 0)
> +    {
> +      puts ("error: spawn_file_actions_addclose");
> +      exit (1);
> +    }
> +
> +  char *argv[] = { (char *) _PATH_BSHELL, (char *) "-c", (char *) "echo $$",
> +		   NULL };
> +  if (posix_spawn (&targ.pid, _PATH_BSHELL, &a, NULL, argv, NULL) != 0)
> +    {
> +      puts ("error: spawn failed");
> +      exit (1);
> +    }
> +
> +  return NULL;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> +  /* The test basically pipes a 'echo $$' created by a thread with a
> +     cancellation pending.  It then checks if the thread is not
> +     cancelled, the process is created, and if the output is the expected
> +     one (the process pid).  */
> +
> +  if (pipe (targ.fd) != 0)
> +    {
> +      puts ("error: pipe failed");
> +      exit (1);
> +    }
> +
> +  /* Not interested in knowing when the pipe is closed.  */
> +  if (sigignore (SIGPIPE) != 0)
> +    {
> +      puts ("error: sigignore failed");
> +      exit (1);
> +    }
> +
> +  if (pthread_barrier_init (&b, NULL, 2) != 0)
> +    {
> +      puts ("error: pthread_barrier_init failed");
> +      exit (1);
> +    }
> +
> +  pthread_t th;
> +  if (pthread_create (&th, NULL, tf, &targ) != 0)
> +    {
> +      puts ("error: pthread_create failed");
> +      exit (1);
> +    }
> +
> +  if (pthread_cancel (th) != 0)
> +    {
> +      puts ("error: pthread_cancel failed");
> +      return 1;
> +    }
> +
> +  int e = pthread_barrier_wait (&b);
> +  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
> +    {
> +      puts ("error: pthread_barrier_wait failed");
> +      exit (1);
> +    }
> +
> +  void *r;
> +  if (pthread_join (th, &r) != 0)
> +    {
> +      puts ("error: pthread_join failed");
> +      exit (1);
> +    }
> +
> +  if (r == PTHREAD_CANCELED)
> +    {
> +      puts ("error: thread cancelled");
> +      exit (1);
> +    }
> +
> +  close (targ.fd[1]);
> +
> +  /* The spawn_posix pid should be set by thread, check if it was executed
> +     correctly and with expected output.  */
> +
> +  char buf[64];
> +  ssize_t n;
> +  bool seen_pid = false;
> +  while (TEMP_FAILURE_RETRY ((n = read (targ.fd[0], buf, sizeof (buf)))) > 0)
> +    {
> +      /* We only expect to read the PID.  */
> +      char *endp;
> +      long int rpid = strtol (buf, &endp, 10);
> +
> +      if (*endp != '\n')
> +	{
> +	  printf ("error: didn't parse whole line: \"%s\"\n", buf);
> +	  exit (1);
> +	}
> +      if (endp == buf)
> +	{
> +	  puts ("error: read empty line");
> +	  exit (1);
> +	}
> +
> +      if (rpid != targ.pid)
> +	{
> +	  printf ("error: found \"%s\", expected PID %ld\n", buf,
> +		 (long int) targ.pid);
> +	  exit (1);
> +	}
> +
> +      if (seen_pid)
> +	{
> +	  puts ("error: found more than one PID line");
> +	  exit (1);
> +	}
> +
> +      seen_pid = true;
> +    }
> +
> +  close (targ.fd[0]);
> +
> +  int status;
> +  int err = waitpid (targ.pid, &status, 0);
> +  if (err != targ.pid)
> +    {
> +      puts ("errnor: waitpid failed");
> +      exit (1);
> +    }
> +
> +  if (!seen_pid)
> +    {
> +      puts ("error: didn't get PID");
> +      exit (1);
> +    }
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include <test-skeleton.c>
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 0b76657..f7cb96f 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -317,7 +317,9 @@ __spawnix (pid_t * pid, const char *file,
>      return errno;
>  
>    /* Disable asynchronous cancellation.  */
> -  int cs = LIBC_CANCEL_ASYNC ();
> +  int state;
> +  __libc_ptf_call (__pthread_setcancelstate,
> +                   (PTHREAD_CANCEL_DISABLE, &state), 0);
>  
>    args.file = file;
>    args.exec = exec;
> @@ -357,7 +359,7 @@ __spawnix (pid_t * pid, const char *file,
>  
>    __sigprocmask (SIG_SETMASK, &args.oldmask, 0);
>  
> -  LIBC_CANCEL_RESET (cs);
> +  __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
>  
>    return ec;
>  }
>
  

* Adhemerval Zanella:

> +  int e = pthread_barrier_wait (&b);
> +  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
> +    {
> +      puts ("error: pthread_barrier_wait failed");
> +      exit (1);
> +    }

We have xpthread_barrier_wait now (also below).

> +  /* The test basically pipes a 'echo $$' created by a thread with a
> +     cancellation pending.  It then checks if the thread is not
> +     cancelled, the process is created, and if the output is the expected
> +     one (the process pid).  */

Don't you have to try this multiple times, to see if you can hit the
race?

> +
> +  if (pipe (targ.fd) != 0)
> +    {
> +      puts ("error: pipe failed");
> +      exit (1);
> +    }
> +
> +  /* Not interested in knowing when the pipe is closed.  */
> +  if (sigignore (SIGPIPE) != 0)
> +    {
> +      puts ("error: sigignore failed");
> +      exit (1);
> +    }
> +
> +  if (pthread_barrier_init (&b, NULL, 2) != 0)
> +    {
> +      puts ("error: pthread_barrier_init failed");
> +      exit (1);
> +    }
> +
> +  pthread_t th;
> +  if (pthread_create (&th, NULL, tf, &targ) != 0)

xpthread_create.

> +  if (pthread_join (th, &r) != 0)

xpthread_join.
  

On 20/09/2016 16:24, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> +  int e = pthread_barrier_wait (&b);
>> +  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
>> +    {
>> +      puts ("error: pthread_barrier_wait failed");
>> +      exit (1);
>> +    }
> 
> We have xpthread_barrier_wait now (also below).
> 

Ack, I will change to xpthread_barrier_wait.

>> +  /* The test basically pipes a 'echo $$' created by a thread with a
>> +     cancellation pending.  It then checks if the thread is not
>> +     cancelled, the process is created, and if the output is the expected
>> +     one (the process pid).  */
> 
> Don't you have to try this multiple times, to see if you can hit the
> race?

Not in this test because the idea is to check if posix_spawn acts or not
on a pending cancellation, not if an asynchronous cancellation cancels the
thread while the thread executes posix_spawn.

> 
>> +
>> +  if (pipe (targ.fd) != 0)
>> +    {
>> +      puts ("error: pipe failed");
>> +      exit (1);
>> +    }
>> +
>> +  /* Not interested in knowing when the pipe is closed.  */
>> +  if (sigignore (SIGPIPE) != 0)
>> +    {
>> +      puts ("error: sigignore failed");
>> +      exit (1);
>> +    }
>> +
>> +  if (pthread_barrier_init (&b, NULL, 2) != 0)
>> +    {
>> +      puts ("error: pthread_barrier_init failed");
>> +      exit (1);
>> +    }
>> +
>> +  pthread_t th;
>> +  if (pthread_create (&th, NULL, tf, &targ) != 0)
> 
> xpthread_create.

Ack.

> 
>> +  if (pthread_join (th, &r) != 0)
> 
> xpthread_join.
> 

Ack.
  

* Adhemerval Zanella:

>> Don't you have to try this multiple times, to see if you can hit the
>> race?
>
> Not in this test because the idea is to check if posix_spawn acts or not
> on a pending cancellation, not if an asynchronous cancellation cancels the
> thread while the thread executes posix_spawn.

Ah, I'm still trying to wrap my head around the different forms of
cancellation.  Patch is good as-is then.  Thanks.