| Message ID | 1473879370-6731-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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; }