]> git.ipfire.org Git - thirdparty/glibc.git/commitdiff
Take lock in pthread_cond_wait cleanup handler only when needed
authorSiddhesh Poyarekar <siddhesh@redhat.com>
Wed, 10 Oct 2012 06:47:27 +0000 (12:17 +0530)
committerSiddhesh Poyarekar <siddhesh@redhat.com>
Wed, 10 Oct 2012 07:22:56 +0000 (12:52 +0530)
[BZ #14652]
When a thread waiting in pthread_cond_wait with a PI mutex is
cancelled after it has returned successfully from the futex syscall
but just before async cancellation is disabled, it enters its
cancellation handler with the mutex held and simply calling a
mutex_lock again will result in a deadlock.  Hence, it is necessary to
see if the thread owns the lock and try to lock it only if it doesn't.

NEWS
nptl/ChangeLog
nptl/Makefile
nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S
nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S
nptl/sysdeps/unix/sysv/linux/pthread-pi-defines.sym
nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S
nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S
nptl/tst-cond25.c [new file with mode: 0644]

diff --git a/NEWS b/NEWS
index 9fc081393a665a87e53fedcbd004299d8cd74b52..490e4dca150273f8ef688d99e0038bc25066f883 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -16,7 +16,7 @@ Version 2.17
   14336, 14337, 14347, 14349, 14376, 14417, 14459, 14476, 14477, 14505,
   14510, 14516, 14518, 14519, 14530, 14532, 14538, 14543, 14544, 14545,
   14557, 14562, 14568, 14576, 14579, 14583, 14587, 14602, 14621, 14638,
-  14645, 14648, 14660, 14661.
+  14645, 14648, 14652, 14660, 14661.
 
 * Support for STT_GNU_IFUNC symbols added for s390 and s390x.
   Optimized versions of memcpy, memset, and memcmp added for System z10 and
index 9eeeeb1bdedfd2226b2cc214eda031850fd28bb5..01ad0b97d6aecc1e7db6409744b652fe9f5f8a89 100644 (file)
@@ -1,3 +1,21 @@
+2012-10-10  Siddhesh Poyarekar  <siddhesh@redhat.com>
+
+       [BZ #14652]
+       * Makefile (tests): New test case tst-cond25.
+       (LDFLAGS-tst-cond25): Link tst-cond25 against librt.
+       * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S
+       (__condvar_tw_cleanup): Lock mutex only if we don't already
+       own it.
+       * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S
+       (__condvar_w_cleanup): Likewise.
+       * sysdeps/unix/sysv/linux/pthread-pi-defines.sym: Add TID_MASK.
+       * sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S
+       (__condvar_cleanup2): Lock mutex only if we don't already
+       own it.
+       * sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S
+       (__condvar_cleanup1): Likewise.
+       * tst-cond25.c: New test case.
+
 2012-10-09  Roland McGrath  <roland@hack.frob.com>
 
        * sysdeps/pthread/configure: Regenerated.
index 67c262794f5f369f264a60c6d43d6acc2dc5570d..184057b5722bb6eaca0b3e5c6be62ff7c7b42539 100644 (file)
@@ -206,7 +206,7 @@ tests = tst-typesizes \
        tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond6 tst-cond7 \
        tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond12 tst-cond13 \
        tst-cond14 tst-cond15 tst-cond16 tst-cond17 tst-cond18 tst-cond19 \
-       tst-cond20 tst-cond21 tst-cond22 tst-cond23 tst-cond24 \
+       tst-cond20 tst-cond21 tst-cond22 tst-cond23 tst-cond24 tst-cond25 \
        tst-cond-except \
        tst-robust1 tst-robust2 tst-robust3 tst-robust4 tst-robust5 \
        tst-robust6 tst-robust7 tst-robust8 tst-robust9 \
@@ -276,6 +276,7 @@ gen-as-const-headers = pthread-errnos.sym
 LDFLAGS-pthread.so = -Wl,--enable-new-dtags,-z,nodelete,-z,initfirst
 
 LDFLAGS-tst-cond24 = -lrt
+LDFLAGS-tst-cond25 = -lrt
 
 include ../Makeconfig
 
index 6761c136e508b8de431a8d24c7563b4140ba992b..884987cf502d28990cfc3b5f9433ce0b0c36ef3b 100644 (file)
@@ -649,10 +649,24 @@ __condvar_tw_cleanup:
        movl    $0x7fffffff, %edx
        ENTER_KERNEL
 
+       /* Lock the mutex only if we don't own it already.  This only happens
+          in case of PI mutexes, if we got cancelled after a successful
+          return of the futex syscall and before disabling async
+          cancellation.  */
 5:     movl    24+FRAME_SIZE(%esp), %eax
-       call    __pthread_mutex_cond_lock
+       movl    MUTEX_KIND(%eax), %ebx
+       andl    $(ROBUST_BIT|PI_BIT), %ebx
+       cmpl    $PI_BIT, %ebx
+       jne     8f
+
+       movl    (%eax), %ebx
+       andl    $TID_MASK, %ebx
+       cmpl    %ebx, %gs:TID
+       je      9f
+
+8:     call    __pthread_mutex_cond_lock
 
-       movl    %esi, (%esp)
+9:     movl    %esi, (%esp)
 .LcallUR:
        call    _Unwind_Resume
        hlt
index 0af06acad4ea2ae752756f9b8f38b8adb7ff7990..bf1e5fe788857cf31d10b88e7d4d21086fdb7c5e 100644 (file)
@@ -566,10 +566,24 @@ __condvar_w_cleanup:
        movl    $0x7fffffff, %edx
        ENTER_KERNEL
 
+       /* Lock the mutex only if we don't own it already.  This only happens
+          in case of PI mutexes, if we got cancelled after a successful
+          return of the futex syscall and before disabling async
+          cancellation.  */
 5:     movl    24+FRAME_SIZE(%esp), %eax
-       call    __pthread_mutex_cond_lock
+       movl    MUTEX_KIND(%eax), %ebx
+       andl    $(ROBUST_BIT|PI_BIT), %ebx
+       cmpl    $PI_BIT, %ebx
+       jne     8f
+
+       movl    (%eax), %ebx
+       andl    $TID_MASK, %ebx
+       cmpl    %ebx, %gs:TID
+       je      9f
+
+8:     call    __pthread_mutex_cond_lock
 
-       movl    %esi, (%esp)
+9:     movl    %esi, (%esp)
 .LcallUR:
        call    _Unwind_Resume
        hlt
index 46fbd0de74a6b2d31dbea92a8bfd83221bf0c93c..0ac51dba989ef0ee98c6463b5fdf2025c715fb75 100644 (file)
@@ -6,3 +6,4 @@ MUTEX_KIND      offsetof (pthread_mutex_t, __data.__kind)
 ROBUST_BIT     PTHREAD_MUTEX_ROBUST_NORMAL_NP
 PI_BIT         PTHREAD_MUTEX_PRIO_INHERIT_NP
 PS_BIT         PTHREAD_MUTEX_PSHARED_BIT
+TID_MASK       FUTEX_TID_MASK
index b669abb573d52e44a7be865ba61bf34f9c13eceb..eb133266c6ce28206c295980dd20027663fb3b0f 100644 (file)
@@ -771,10 +771,24 @@ __condvar_cleanup2:
        movl    $SYS_futex, %eax
        syscall
 
+       /* Lock the mutex only if we don't own it already.  This only happens
+          in case of PI mutexes, if we got cancelled after a successful
+          return of the futex syscall and before disabling async
+          cancellation.  */
 5:     movq    16(%rsp), %rdi
-       callq   __pthread_mutex_cond_lock
+       movl    MUTEX_KIND(%rdi), %eax
+       andl    $(ROBUST_BIT|PI_BIT), %eax
+       cmpl    $PI_BIT, %eax
+       jne     7f
+
+       movl    (%rdi), %eax
+       andl    $TID_MASK, %eax
+       cmpl    %eax, %fs:TID
+       je      8f
+
+7:     callq   __pthread_mutex_cond_lock
 
-       movq    24(%rsp), %rdi
+8:     movq    24(%rsp), %rdi
        movq    FRAME_SIZE(%rsp), %r15
        movq    FRAME_SIZE+8(%rsp), %r14
        movq    FRAME_SIZE+16(%rsp), %r13
index ec403cd9b8e7db8cb30ca7a05c6032fec728da0c..6c6dc0e74d42eeda33b12254c7e3cdf85c9249bb 100644 (file)
@@ -495,10 +495,24 @@ __condvar_cleanup1:
        movl    $SYS_futex, %eax
        syscall
 
+       /* Lock the mutex only if we don't own it already.  This only happens
+          in case of PI mutexes, if we got cancelled after a successful
+          return of the futex syscall and before disabling async
+          cancellation.  */
 5:     movq    16(%rsp), %rdi
-       callq   __pthread_mutex_cond_lock
+       movl    MUTEX_KIND(%rdi), %eax
+       andl    $(ROBUST_BIT|PI_BIT), %eax
+       cmpl    $PI_BIT, %eax
+       jne     7f
+
+       movl    (%rdi), %eax
+       andl    $TID_MASK, %eax
+       cmpl    %eax, %fs:TID
+       je      8f
+
+7:     callq   __pthread_mutex_cond_lock
 
-       movq    24(%rsp), %rdi
+8:     movq    24(%rsp), %rdi
 .LcallUR:
        call    _Unwind_Resume@PLT
        hlt
diff --git a/nptl/tst-cond25.c b/nptl/tst-cond25.c
new file mode 100644 (file)
index 0000000..4488e74
--- /dev/null
@@ -0,0 +1,282 @@
+/* Verify that condition variables synchronized by PI mutexes don't hang on
+   on cancellation.
+   Copyright (C) 2012 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 <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+#include <sys/time.h>
+#include <time.h>
+
+#define NUM 5
+#define ITERS 10000
+#define COUNT 100
+
+typedef void *(*thr_func) (void *);
+
+pthread_mutex_t mutex;
+pthread_cond_t cond;
+
+void cleanup (void *u)
+{
+  /* pthread_cond_wait should always return with the mutex locked.  */
+  if (pthread_mutex_unlock (&mutex))
+    abort ();
+}
+
+void *
+signaller (void *u)
+{
+  int i, ret = 0;
+  void *tret = NULL;
+
+  for (i = 0; i < ITERS; i++)
+    {
+      if ((ret = pthread_mutex_lock (&mutex)) != 0)
+        {
+         tret = (void *)1;
+         printf ("signaller:mutex_lock failed: %s\n", strerror (ret));
+         goto out;
+       }
+      if ((ret = pthread_cond_signal (&cond)) != 0)
+        {
+         tret = (void *)1;
+         printf ("signaller:signal failed: %s\n", strerror (ret));
+         goto unlock_out;
+       }
+      if ((ret = pthread_mutex_unlock (&mutex)) != 0)
+        {
+         tret = (void *)1;
+         printf ("signaller:mutex_unlock failed: %s\n", strerror (ret));
+         goto out;
+       }
+      pthread_testcancel ();
+    }
+
+out:
+  return tret;
+
+unlock_out:
+  if ((ret = pthread_mutex_unlock (&mutex)) != 0)
+    printf ("signaller:mutex_unlock[2] failed: %s\n", strerror (ret));
+  goto out;
+}
+
+void *
+waiter (void *u)
+{
+  int i, ret = 0;
+  void *tret = NULL;
+  int seq = (int)u;
+
+  for (i = 0; i < ITERS / NUM; i++)
+    {
+      if ((ret = pthread_mutex_lock (&mutex)) != 0)
+        {
+         tret = (void *)1;
+         printf ("waiter[%u]:mutex_lock failed: %s\n", seq, strerror (ret));
+         goto out;
+       }
+      pthread_cleanup_push (cleanup, NULL);
+
+      if ((ret = pthread_cond_wait (&cond, &mutex)) != 0)
+        {
+         tret = (void *)1;
+         printf ("waiter[%u]:wait failed: %s\n", seq, strerror (ret));
+         goto unlock_out;
+       }
+
+      if ((ret = pthread_mutex_unlock (&mutex)) != 0)
+        {
+         tret = (void *)1;
+         printf ("waiter[%u]:mutex_unlock failed: %s\n", seq, strerror (ret));
+         goto out;
+       }
+      pthread_cleanup_pop (0);
+    }
+
+out:
+  puts ("waiter tests done");
+  return tret;
+
+unlock_out:
+  if ((ret = pthread_mutex_unlock (&mutex)) != 0)
+    printf ("waiter:mutex_unlock[2] failed: %s\n", strerror (ret));
+  goto out;
+}
+
+void *
+timed_waiter (void *u)
+{
+  int i, ret;
+  void *tret = NULL;
+  int seq = (int)u;
+
+  for (i = 0; i < ITERS / NUM; i++)
+    {
+      struct timespec ts;
+
+      if ((ret = clock_gettime(CLOCK_REALTIME, &ts)) != 0)
+        {
+         tret = (void *)1;
+         printf ("%u:clock_gettime failed: %s\n", seq, strerror (errno));
+         goto out;
+       }
+      ts.tv_sec += 20;
+
+      if ((ret = pthread_mutex_lock (&mutex)) != 0)
+        {
+         tret = (void *)1;
+         printf ("waiter[%u]:mutex_lock failed: %s\n", seq, strerror (ret));
+         goto out;
+       }
+      pthread_cleanup_push (cleanup, NULL);
+
+      /* We should not time out either.  */
+      if ((ret = pthread_cond_timedwait (&cond, &mutex, &ts)) != 0)
+        {
+         tret = (void *)1;
+         printf ("waiter[%u]:timedwait failed: %s\n", seq, strerror (ret));
+         goto unlock_out;
+       }
+      if ((ret = pthread_mutex_unlock (&mutex)) != 0)
+        {
+         tret = (void *)1;
+         printf ("waiter[%u]:mutex_unlock failed: %s\n", seq, strerror (ret));
+         goto out;
+       }
+      pthread_cleanup_pop (0);
+    }
+
+out:
+  puts ("timed_waiter tests done");
+  return tret;
+
+unlock_out:
+  if ((ret = pthread_mutex_unlock (&mutex)) != 0)
+    printf ("waiter[%u]:mutex_unlock[2] failed: %s\n", seq, strerror (ret));
+  goto out;
+}
+
+int
+do_test_wait (thr_func f)
+{
+  pthread_t w[NUM];
+  pthread_t s;
+  pthread_mutexattr_t attr;
+  int i, j, ret = 0;
+  void *thr_ret;
+
+  for (i = 0; i < COUNT; i++)
+    {
+      if ((ret = pthread_mutexattr_init (&attr)) != 0)
+        {
+         printf ("mutexattr_init failed: %s\n", strerror (ret));
+         goto out;
+       }
+
+      if ((ret = pthread_mutexattr_setprotocol (&attr, PTHREAD_PRIO_INHERIT)) != 0)
+        {
+         printf ("mutexattr_setprotocol failed: %s\n", strerror (ret));
+         goto out;
+       }
+
+      if ((ret = pthread_cond_init (&cond, NULL)) != 0)
+        {
+         printf ("cond_init failed: %s\n", strerror (ret));
+         goto out;
+       }
+
+      if ((ret = pthread_mutex_init (&mutex, &attr)) != 0)
+        {
+         printf ("mutex_init failed: %s\n", strerror (ret));
+         goto out;
+       }
+
+      for (j = 0; j < NUM; j++)
+        if ((ret = pthread_create (&w[j], NULL, f, (void *)j)) != 0)
+         {
+           printf ("waiter[%d]: create failed: %s\n", j, strerror (ret));
+           goto out;
+         }
+
+      if ((ret = pthread_create (&s, NULL, signaller, NULL)) != 0)
+        {
+         printf ("signaller: create failed: %s\n", strerror (ret));
+         goto out;
+       }
+
+      for (j = 0; j < NUM; j++)
+        {
+          if ((ret = pthread_cancel (w[j])) != 0)
+           {
+             printf ("waiter[%d]: cancel failed: %s\n", j, strerror (ret));
+             goto out;
+           }
+
+          if ((ret = pthread_join (w[j], &thr_ret)) != 0)
+           {
+             printf ("waiter[%d]: join failed: %s\n", j, strerror (ret));
+             goto out;
+           }
+
+          if (thr_ret != NULL && thr_ret != PTHREAD_CANCELED)
+           {
+             ret = 1;
+             goto out;
+           }
+        }
+
+      /* The signalling thread could have ended before it was cancelled.  */
+      pthread_cancel (s);
+
+      if ((ret = pthread_join (s, &thr_ret)) != 0)
+        {
+         printf ("signaller: join failed: %s\n", strerror (ret));
+         goto out;
+       }
+
+      if (thr_ret != NULL && thr_ret != PTHREAD_CANCELED)
+        {
+          ret = 1;
+          goto out;
+        }
+    }
+
+out:
+  return ret;
+}
+
+int
+do_test (int argc, char **argv)
+{
+  int ret = do_test_wait (waiter);
+
+  if (ret)
+    return ret;
+
+  return do_test_wait (timed_waiter);
+}
+
+#define TIMEOUT 5
+#include "../test-skeleton.c"