[PATCH 2/6] nptl: Revert robust list head on pthread_mutex_timedlock failure (bug 34542)

Florian Weimer <[email protected]>
Newsgroups gmane.comp.lib.glibc.alpha
Message-ID <1e0afb7818ca47e3c41fc0665dbee8284680a5e1.1787168028.git.fweimer@redhat.com>
For some error scenarios, the robust list head is left pointed at
the mutex after the return.  This can cause the kernel to update
the mutex lock field after it has been reallocated for something
else.
---
 nptl/pthread_mutex_timedlock.c |  51 +++++-----
 sysdeps/pthread/Makefile       |   1 +
 sysdeps/pthread/tst-robust12.c | 166 +++++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 27 deletions(-)
 create mode 100644 sysdeps/pthread/tst-robust12.c

diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index 9efca2c779..3b8bb1ddfd 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -28,6 +28,14 @@
 
 #include <stap-probe.h>
 
+/* Return the error code ERR after clearing the robust list head.  */
+static int
+__pthread_mutex_robust_error (int err)
+{
+  THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
+  return err;
+}
+
 int
 __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 				  clockid_t clockid,
@@ -162,13 +170,12 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	      ENQUEUE_MUTEX (mutex);
 	      /* We need to clear op_pending after we enqueue the mutex.  */
 	      __asm ("" ::: "memory");
-	      THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 	      /* Note that we deliberately exit here.  If we fall
 		 through to the end of the function __nusers would be
 		 incremented which is not correct because the old
 		 owner has to be discounted.  */
-	      return EOWNERDEAD;
+	      return __pthread_mutex_robust_error (EOWNERDEAD);
 	    }
 
 	  /* Check whether we already hold the mutex.  */
@@ -176,13 +183,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	    {
 	      int kind = PTHREAD_MUTEX_TYPE (mutex);
 	      if (kind == PTHREAD_MUTEX_ROBUST_ERRORCHECK_NP)
-		{
-		  /* We do not need to ensure ordering wrt another memory
-		     access.  Also see comments at ENQUEUE_MUTEX. */
-		  THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending,
-				 NULL);
-		  return EDEADLK;
-		}
+		/* We do not need to ensure ordering wrt another memory
+		   access.  Also see comments at ENQUEUE_MUTEX. */
+		return __pthread_mutex_robust_error (EDEADLK);
 
 	      if (kind == PTHREAD_MUTEX_ROBUST_RECURSIVE_NP)
 		{
@@ -206,11 +209,11 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 
 	  /* We are about to block; check whether the timeout is invalid.  */
 	  if (! valid_nanoseconds (abstime->tv_nsec))
-	    return EINVAL;
+	    return __pthread_mutex_robust_error (EINVAL);
 	  /* Work around the fact that the kernel rejects negative timeout
 	     values despite them being valid.  */
 	  if (__glibc_unlikely (abstime->tv_sec < 0))
-	    return ETIMEDOUT;
+	    return __pthread_mutex_robust_error (ETIMEDOUT);
 
 	  /* We cannot acquire the mutex nor has its owner died.  Thus, try
 	     to block using futexes.  Set FUTEX_WAITERS if necessary so that
@@ -242,7 +245,7 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	      PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
 	  /* The futex call timed out.  */
 	  if (err == ETIMEDOUT || err == EOVERFLOW)
-	    return err;
+	    return __pthread_mutex_robust_error (err);
 	  /* Reload current lock value.  */
 	  oldval = mutex->__data.__lock;
 	}
@@ -257,8 +260,7 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	  lll_unlock (mutex->__data.__lock, private);
 	  /* FIXME This violates the mutex destruction requirements.  See
 	     __pthread_mutex_unlock_full.  */
-	  THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
-	  return ENOTRECOVERABLE;
+	  return __pthread_mutex_robust_error (ENOTRECOVERABLE);
 	}
 
       mutex->__data.__count = 1;
@@ -310,12 +312,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	if (__glibc_unlikely ((oldval & FUTEX_TID_MASK) == id))
 	  {
 	    if (kind == PTHREAD_MUTEX_ERRORCHECK_NP)
-	      {
-		/* We do not need to ensure ordering wrt another memory
-		   access.  */
-		THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
-		return EDEADLK;
-	      }
+	      /* We do not need to ensure ordering wrt another memory
+		 access.  */
+	      return __pthread_mutex_robust_error (EDEADLK);
 
 	    if (kind == PTHREAD_MUTEX_RECURSIVE_NP)
 	      {
@@ -350,7 +349,7 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	    int e = __futex_lock_pi64 (&mutex->__data.__lock, clockid, abstime,
 				       private);
 	    if (e == ETIMEDOUT)
-	      return ETIMEDOUT;
+	      return __pthread_mutex_robust_error (ETIMEDOUT);
 	    else if (e == ESRCH || e == EDEADLK)
 	      {
 		assert (e != EDEADLK
@@ -366,10 +365,10 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 		  e = __futex_abstimed_wait64 (&(unsigned int){0}, 0, clockid,
 					       abstime, private);
 		while (e != ETIMEDOUT);
-		return ETIMEDOUT;
+		return __pthread_mutex_robust_error (ETIMEDOUT);
 	      }
 	    else if (e != 0)
-	      return e;
+	      return __pthread_mutex_robust_error (e);
 
 	    oldval = mutex->__data.__lock;
 
@@ -391,13 +390,12 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	    ENQUEUE_MUTEX_PI (mutex);
 	    /* We need to clear op_pending after we enqueue the mutex.  */
 	    __asm ("" ::: "memory");
-	    THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 	    /* Note that we deliberately exit here.  If we fall
 	       through to the end of the function __nusers would be
 	       incremented which is not correct because the old owner
 	       has to be discounted.  */
-	    return EOWNERDEAD;
+	    return __pthread_mutex_robust_error (EOWNERDEAD);
 	  }
 
 	if (robust
@@ -412,8 +410,7 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 
 	    /* To the kernel, this will be visible after the kernel has
 	       acquired the mutex in the syscall.  */
-	    THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
-	    return ENOTRECOVERABLE;
+	    return __pthread_mutex_robust_error (ENOTRECOVERABLE);
 	  }
 
 	mutex->__data.__count = 1;
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index d0f3cd59ac..1429fa0205 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -233,6 +233,7 @@ tests += \
   tst-robust9 \
   tst-robust10 \
   tst-robust11 \
+  tst-robust12 \
   tst-rwlock-tryrdlock-stall \
   tst-rwlock-trywrlock-stall \
   tst-rwlock1 \
diff --git a/sysdeps/pthread/tst-robust12.c b/sysdeps/pthread/tst-robust12.c
new file mode 100644
index 0000000000..0da0a358af
--- /dev/null
+++ b/sysdeps/pthread/tst-robust12.c
@@ -0,0 +1,166 @@
+/* Test robust mutex head list management in case of error returns.
+   Copyright (C) 2026 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/timespec.h>
+#include <support/xthread.h>
+#include <support/xtime.h>
+
+/* This mutex is attempted to be locked by the thread test.  */
+static pthread_mutex_t mutex;
+
+/* This is the futex value when the mutex is locked on the test thread.
+   It is used to restore the kernel-expected bit pattern in the mutex
+   variable.  */
+static __typeof (mutex.__data.__lock) futex_value;
+
+static pthread_barrier_t barrier;
+
+/* This test covers various code paths in the locking implementation.  */
+static enum
+  {
+    /* Invalid nanoseconds value in timeout argument.  */
+    test_invalid_nsec,
+    /* Timeout before performing the futex operation.  */
+    test_early_timeout,
+    /* Timeout after performing the futex operation.  */
+    test_late_timeout,
+  } subtest;
+
+static void *
+threadfunc (void *ignored)
+{
+  /* Capture the expected futex value.  */
+  xpthread_mutex_lock (&mutex);
+  futex_value = mutex.__data.__lock;
+  xpthread_mutex_unlock (&mutex);
+
+  /* Wait for the main thread to lock.  */
+  xpthread_barrier_wait (&barrier);
+  xpthread_barrier_wait (&barrier);
+
+  switch (subtest)
+    {
+    case test_invalid_nsec:
+      {
+        struct timespec t = { 0, -1 };
+        TEST_COMPARE (pthread_mutex_timedlock (&mutex, &t), EINVAL);
+      }
+      break;
+    case test_early_timeout:
+      {
+        /* Time is in the past: times out immediately.  */
+        struct timespec t = { -1, 0 };
+        TEST_COMPARE (pthread_mutex_timedlock (&mutex, &t), ETIMEDOUT);
+      }
+      break;
+    case test_late_timeout:
+      {
+        /* Time is in the future: expected to block in the kernel.  */
+        struct timespec t;
+        xclock_gettime (CLOCK_REALTIME, &t);
+        t = timespec_add (t, make_timespec (0, 50 * 1000 * 1000));
+        TEST_COMPARE (pthread_mutex_timedlock (&mutex, &t), ETIMEDOUT);
+      }
+      break;
+    }
+
+  /* Tell the main thread that it is safe to destroy the mutex.  */
+  xpthread_barrier_wait (&barrier);
+  /* Wait for the main thread to destroy the mutex.  */
+  xpthread_barrier_wait (&barrier);
+
+  /* Exit the thread and trigger robust list processing.  */
+  return NULL;
+}
+
+static void
+test_one (bool use_pi)
+{
+  xpthread_barrier_init (&barrier, NULL, 2);
+
+  /* Create the robust mutex.  */
+  {
+    pthread_mutexattr_t a;
+    xpthread_mutexattr_init (&a);
+    TEST_COMPARE (pthread_mutexattr_setrobust (&a, PTHREAD_MUTEX_ROBUST), 0);
+    if (use_pi)
+      xpthread_mutexattr_setprotocol (&a, PTHREAD_PRIO_INHERIT);
+    xpthread_mutex_init (&mutex, &a);
+    xpthread_mutexattr_destroy (&a);
+  }
+
+  pthread_t thr = xpthread_create (NULL, threadfunc, NULL);
+
+  /* Wait for the thread to initialize futex_value.  */
+  xpthread_barrier_wait (&barrier);
+
+  /* Prevent the pthread_mutex_timedlock call from succeeding.  */
+  xpthread_mutex_lock (&mutex);
+
+  /* Allow the test thread to perform the lock.  */
+  xpthread_barrier_wait (&barrier);
+
+  /* Wait for the timeout in the test thread.  */
+  xpthread_barrier_wait (&barrier);
+
+  /* Destroy the mutex.  */
+  xpthread_mutex_unlock (&mutex);
+  xpthread_mutex_destroy (&mutex);
+
+  /* Overwrite the mutex with a distinct bit pattern.  */
+  char pattern[sizeof (mutex)];
+  memset (&pattern, 0xcc, sizeof (pattern));
+  /* Set a futex value that will be acted upon by the kernel.  */
+  memcpy (&pattern[offsetof (pthread_mutex_t, __data.__lock)],
+          &futex_value, sizeof (futex_value));
+  memcpy (&mutex, pattern, sizeof (mutex));
+
+  /* Allow the thread to exit.  */
+  xpthread_barrier_wait (&barrier);
+
+  /* Wait for the other thread to exit.  */
+  xpthread_join (thr);
+
+  /* Check that the mutex object was not written to.  */
+  TEST_COMPARE_BLOB (&mutex, sizeof (mutex), pattern, sizeof (pattern));
+
+  xpthread_barrier_destroy (&barrier);
+}
+
+static int
+do_test (void)
+{
+  for (int subtest_int = 0; subtest_int <= test_late_timeout; ++subtest_int)
+    for (int use_pi = 0; use_pi < 2; ++use_pi)
+      {
+        printf ("info: subtest %d, PI %s\n",
+                subtest_int, use_pi ? "active" : "inactive");
+        subtest = subtest_int;
+        test_one (use_pi);
+      }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.