From 47677f2edc815e85d0383a89b09733e95e5d7302 Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella Date: Mon, 22 Aug 2016 09:39:46 -0300 Subject: [PATCH] nptl: Fix sem_wait and sem_timedwait cancellation (BZ#18243) This patch fixes both sem_wait and sem_timedwait cancellation point for uncontended case. In this scenario only atomics are involved and thus the futex cancellable call is not issue and a pending cancellation signal is not handled. The fix is straighforward by calling pthread_testcancel is both function start. Although it would be simpler to call CANCELLATION_P directly, I decided to add an internal pthread_testcancel alias and use it to export less internal implementation on such function. A possible change on how pthread_testcancel is internally implemented would lead to either continue to force use CANCELLATION_P or to adjust its every use. GLIBC testcase also does have tests for uncontended cases, test-cancel12 and test-cancel14.c, however both are flawed by adding another cancellation point just after thread pthread_cleanup_pop: 47 static void * 48 tf (void *arg) 49 { 50 pthread_cleanup_push (cleanup, NULL); 51 52 int e = pthread_barrier_wait (&bar); 53 if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) 54 { 55 puts ("tf: 1st barrier_wait failed"); 56 exit (1); 57 } 58 59 /* This call should block and be cancelable. */ 60 sem_wait (&sem); 61 62 pthread_cleanup_pop (0); 63 64 puts ("sem_wait returned"); 65 66 return NULL; 67 } So sem_{timed}wait does not act on cancellation, pthread_cleanup_pop executes 'cleanup' and then 'puts' acts on cancellation. Since pthread_cleanup_pop removed the clean-up handler, it will ran only once and thus it won't accuse an error to indicate sem_wait has not acted on the cancellation signal. This patch also fixes this behavior by removing the cancellation point 'puts'. It also adds some cleanup on all sem_{timed}wait cancel tests. It partially fixes BZ #18243. Checked on x86_64. [BZ #18243] * nptl/pthreadP.h (__pthread_testcancel): Add prototype and hidden_proto. * nptl/pthread_testcancel.c (pthread_cancel): Add internal aliais definition. * nptl/sem_timedwait.c (sem_timedwait): Add cancellation check for uncontended case. * nptl/sem_wait.c (__new_sem_wait): Likewise. * nptl/tst-cancel12.c (cleanup): Remove wrong cancellation point. (tf): Fix check for uncontended case. (do_test): Likewise. * nptl/tst-cancel13.c (cleanup): Remove wrong cancellation point. (tf): Fix check for uncontended case. (do_test): Likewise. * nptl/tst-cancel14.c (cleanup): Remove wrong cancellation point. (tf): Fix check for uncontended case. (do_test): Likewise. * nptl/tst-cancel15.c (cleanup): Remove wrong cancellation point. (tf): Fix check for uncontended case. (do_test): Likewise. --- ChangeLog | 20 ++++++++++++++++++++ nptl/pthreadP.h | 2 ++ nptl/pthread_testcancel.c | 4 +++- nptl/sem_timedwait.c | 3 +++ nptl/sem_wait.c | 13 +++++++++++++ nptl/tst-cancel12.c | 29 +++++++++++++---------------- nptl/tst-cancel13.c | 23 +++++++++++------------ nptl/tst-cancel14.c | 17 +++++++---------- nptl/tst-cancel15.c | 23 ++++++++++------------- 9 files changed, 82 insertions(+), 52 deletions(-) diff --git a/ChangeLog b/ChangeLog index 065adb52a0..6225e195f1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,25 @@ 2016-09-15 Adhemerval Zanella + [BZ #18243] + * nptl/pthreadP.h (__pthread_testcancel): Add prototype and hidden_proto. + * nptl/pthread_testcancel.c (pthread_cancel): Add internal aliais + definition. + * nptl/sem_timedwait.c (sem_timedwait): Add cancellation check for + uncontended case. + * nptl/sem_wait.c (__new_sem_wait): Likewise. + * nptl/tst-cancel12.c (cleanup): Remove wrong cancellation point. + (tf): Fix check for uncontended case. + (do_test): Likewise. + * nptl/tst-cancel13.c (cleanup): Remove wrong cancellation point. + (tf): Fix check for uncontended case. + (do_test): Likewise. + * nptl/tst-cancel14.c (cleanup): Remove wrong cancellation point. + (tf): Fix check for uncontended case. + (do_test): Likewise. + * nptl/tst-cancel15.c (cleanup): Remove wrong cancellation point. + (tf): Fix check for uncontended case. + (do_test): Likewise. + * sysdeps/sparc/sparc32/sem_wait.c: Remove file. * sysdeps/sparc/sparc32/sparcv9/sem_wait.c: Likewise. diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 4edc74b1ef..6e0dd09f4f 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -491,6 +491,7 @@ extern int __pthread_setcanceltype (int type, int *oldtype); extern int __pthread_enable_asynccancel (void) attribute_hidden; extern void __pthread_disable_asynccancel (int oldtype) internal_function attribute_hidden; +extern void __pthread_testcancel (void); #if IS_IN (libpthread) hidden_proto (__pthread_mutex_init) @@ -505,6 +506,7 @@ hidden_proto (__pthread_getspecific) hidden_proto (__pthread_setspecific) hidden_proto (__pthread_once) hidden_proto (__pthread_setcancelstate) +hidden_proto (__pthread_testcancel) #endif extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond); diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c index 51be09fc5b..fdef69967b 100644 --- a/nptl/pthread_testcancel.c +++ b/nptl/pthread_testcancel.c @@ -21,7 +21,9 @@ void -pthread_testcancel (void) +__pthread_testcancel (void) { CANCELLATION_P (THREAD_SELF); } +strong_alias (__pthread_testcancel, pthread_testcancel) +hidden_def (__pthread_testcancel) diff --git a/nptl/sem_timedwait.c b/nptl/sem_timedwait.c index 1aab25a7cf..a02e1785b7 100644 --- a/nptl/sem_timedwait.c +++ b/nptl/sem_timedwait.c @@ -30,6 +30,9 @@ sem_timedwait (sem_t *sem, const struct timespec *abstime) return -1; } + /* Check sem_wait.c for a more detailed explanation why it is required. */ + __pthread_testcancel (); + if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0) return 0; else diff --git a/nptl/sem_wait.c b/nptl/sem_wait.c index 84b523a768..b242c08d2e 100644 --- a/nptl/sem_wait.c +++ b/nptl/sem_wait.c @@ -23,6 +23,19 @@ int __new_sem_wait (sem_t *sem) { + /* We need to check whether we need to act upon a cancellation request here + because POSIX specifies that cancellation points "shall occur" in + sem_wait and sem_timedwait, which also means that they need to check + this regardless whether they block or not (unlike "may occur" + functions). See the POSIX Rationale for this requirement: Section + "Thread Cancellation Overview" [1] and austin group issue #1076 [2] + for thoughs on why this may be a suboptimal design. + + [1] http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html + [2] http://austingroupbugs.net/view.php?id=1076 for thoughts on why this + */ + __pthread_testcancel (); + if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0) return 0; else diff --git a/nptl/tst-cancel12.c b/nptl/tst-cancel12.c index ac8f5a0559..a720ccfaf0 100644 --- a/nptl/tst-cancel12.c +++ b/nptl/tst-cancel12.c @@ -1,4 +1,5 @@ -/* Copyright (C) 2003-2016 Free Software Foundation, Inc. +/* Test sem_wait cancellation for uncontended case. + Copyright (C) 2003-2016 Free Software Foundation, Inc. This file is part of the GNU C Library. Contributed by Ulrich Drepper , 2003. @@ -39,8 +40,6 @@ cleanup (void *arg) puts ("second call to cleanup"); exit (1); } - - printf ("cleanup call #%d\n", ncall); } @@ -52,17 +51,15 @@ tf (void *arg) int e = pthread_barrier_wait (&bar); if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) { - puts ("tf: 1st barrier_wait failed"); + puts ("error: tf: 1st barrier_wait failed"); exit (1); } - /* This call should block and be cancelable. */ + /* This call should be cancelable. */ sem_wait (&sem); pthread_cleanup_pop (0); - puts ("sem_wait returned"); - return NULL; } @@ -74,47 +71,47 @@ do_test (void) if (pthread_barrier_init (&bar, NULL, 2) != 0) { - puts ("barrier_init failed"); + puts ("error: barrier_init failed"); exit (1); } + /* A value higher than 0 will check for uncontended pthread cancellation, + where the sem_wait operation will return immediatelly. */ if (sem_init (&sem, 0, 1) != 0) { - puts ("sem_init failed"); + puts ("error: sem_init failed"); exit (1); } if (pthread_create (&th, NULL, tf, NULL) != 0) { - puts ("create failed"); + puts ("error: create failed"); exit (1); } - /* Check whether cancellation is honored even before sem_wait does - anything. */ if (pthread_cancel (th) != 0) { - puts ("1st cancel failed"); + puts ("error: pthread_cancel failed"); exit (1); } int e = pthread_barrier_wait (&bar); if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) { - puts ("1st barrier_wait failed"); + puts ("error: 1st barrier_wait failed"); exit (1); } void *r; if (pthread_join (th, &r) != 0) { - puts ("join failed"); + puts ("error: pthread_join failed"); exit (1); } if (r != PTHREAD_CANCELED) { - puts ("thread not canceled"); + puts ("error: thread not canceled"); exit (1); } diff --git a/nptl/tst-cancel13.c b/nptl/tst-cancel13.c index c84780ded3..ee39b963ad 100644 --- a/nptl/tst-cancel13.c +++ b/nptl/tst-cancel13.c @@ -1,4 +1,5 @@ -/* Copyright (C) 2003-2016 Free Software Foundation, Inc. +/* Test sem_wait cancellation for contended case. + Copyright (C) 2003-2016 Free Software Foundation, Inc. This file is part of the GNU C Library. Contributed by Ulrich Drepper , 2003. @@ -39,8 +40,6 @@ cleanup (void *arg) puts ("second call to cleanup"); exit (1); } - - printf ("cleanup call #%d\n", ncall); } @@ -52,7 +51,7 @@ tf (void *arg) int e = pthread_barrier_wait (&bar); if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) { - puts ("tf: 1st barrier_wait failed"); + puts ("error: tf: 1st barrier_wait failed"); exit (1); } @@ -61,8 +60,6 @@ tf (void *arg) pthread_cleanup_pop (0); - puts ("sem_wait returned"); - return NULL; } @@ -74,26 +71,28 @@ do_test (void) if (pthread_barrier_init (&bar, NULL, 2) != 0) { - puts ("barrier_init failed"); + puts ("error: barrier_init failed"); exit (1); } + /* A value equal to 0 will check for contended pthread cancellation, + where the sem_wait operation will block. */ if (sem_init (&sem, 0, 0) != 0) { - puts ("sem_init failed"); + puts ("error: sem_init failed"); exit (1); } if (pthread_create (&th, NULL, tf, NULL) != 0) { - puts ("create failed"); + puts ("error: create failed"); exit (1); } int e = pthread_barrier_wait (&bar); if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) { - puts ("1st barrier_wait failed"); + puts ("error: 1st barrier_wait failed"); exit (1); } @@ -103,14 +102,14 @@ do_test (void) /* Check whether cancellation is honored when waiting in sem_wait. */ if (pthread_cancel (th) != 0) { - puts ("1st cancel failed"); + puts ("error: 1st cancel failed"); exit (1); } void *r; if (pthread_join (th, &r) != 0) { - puts ("join failed"); + puts ("error: join failed"); exit (1); } diff --git a/nptl/tst-cancel14.c b/nptl/tst-cancel14.c index 3810d2b289..b6fcbb774d 100644 --- a/nptl/tst-cancel14.c +++ b/nptl/tst-cancel14.c @@ -1,4 +1,5 @@ -/* Copyright (C) 2003-2016 Free Software Foundation, Inc. +/* Test sem_timedwait cancellation for uncontended case. + Copyright (C) 2003-2016 Free Software Foundation, Inc. This file is part of the GNU C Library. Contributed by Ulrich Drepper , 2003. @@ -40,8 +41,6 @@ cleanup (void *arg) puts ("second call to cleanup"); exit (1); } - - printf ("cleanup call #%d\n", ncall); } @@ -53,7 +52,7 @@ tf (void *arg) int e = pthread_barrier_wait (&bar); if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) { - puts ("tf: 1st barrier_wait failed"); + puts ("error: tf: 1st barrier_wait failed"); exit (1); } @@ -71,8 +70,6 @@ tf (void *arg) pthread_cleanup_pop (0); - puts ("sem_timedwait returned"); - return NULL; } @@ -84,19 +81,19 @@ do_test (void) if (pthread_barrier_init (&bar, NULL, 2) != 0) { - puts ("barrier_init failed"); + puts ("error: barrier_init failed"); exit (1); } if (sem_init (&sem, 0, 1) != 0) { - puts ("sem_init failed"); + puts ("error: sem_init failed"); exit (1); } if (pthread_create (&th, NULL, tf, NULL) != 0) { - puts ("create failed"); + puts ("error: create failed"); exit (1); } @@ -104,7 +101,7 @@ do_test (void) anything. */ if (pthread_cancel (th) != 0) { - puts ("1st cancel failed"); + puts ("error: 1st cancel failed"); exit (1); } diff --git a/nptl/tst-cancel15.c b/nptl/tst-cancel15.c index f413839ec2..c8db3e42b2 100644 --- a/nptl/tst-cancel15.c +++ b/nptl/tst-cancel15.c @@ -1,4 +1,5 @@ -/* Copyright (C) 2003-2016 Free Software Foundation, Inc. +/* Test sem_timedwait cancellation for contended case. + Copyright (C) 2003-2016 Free Software Foundation, Inc. This file is part of the GNU C Library. Contributed by Ulrich Drepper , 2003. @@ -40,8 +41,6 @@ cleanup (void *arg) puts ("second call to cleanup"); exit (1); } - - printf ("cleanup call #%d\n", ncall); } @@ -55,7 +54,7 @@ tf (void *arg) e = pthread_barrier_wait (&bar); if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) { - puts ("tf: 1st barrier_wait failed"); + puts ("error: tf: 1st barrier_wait failed"); exit (1); } @@ -74,8 +73,6 @@ tf (void *arg) pthread_cleanup_pop (0); - printf ("sem_timedwait returned, e = %d, errno = %d\n", e, errno); - return NULL; } @@ -87,26 +84,26 @@ do_test (void) if (pthread_barrier_init (&bar, NULL, 2) != 0) { - puts ("barrier_init failed"); + puts ("error: barrier_init failed"); exit (1); } if (sem_init (&sem, 0, 0) != 0) { - puts ("sem_init failed"); + puts ("error: sem_init failed"); exit (1); } if (pthread_create (&th, NULL, tf, NULL) != 0) { - puts ("create failed"); + puts ("error: create failed"); exit (1); } int e = pthread_barrier_wait (&bar); if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD) { - puts ("1st barrier_wait failed"); + puts ("error: 1st barrier_wait failed"); exit (1); } @@ -116,20 +113,20 @@ do_test (void) /* Check whether cancellation is honored when waiting in sem_timedwait. */ if (pthread_cancel (th) != 0) { - puts ("1st cancel failed"); + puts ("error: 1st cancel failed"); exit (1); } void *r; if (pthread_join (th, &r) != 0) { - puts ("join failed"); + puts ("error: join failed"); exit (1); } if (r != PTHREAD_CANCELED) { - puts ("thread not canceled"); + puts ("error: thread not canceled"); exit (1); } -- 2.43.5