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

On 19/08/26 16:34, Florian Weimer wrote:
> 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.

LGTM with a change below for testing.

Reviewed-by: Adhemerval Zanella  <[email protected]>

> ---
>  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;

This does not build on Hurd:

../sysdeps/pthread/tst-robust12.c:35:23: error: ‘pthread_mutex_t’ {aka ‘struct __pthread_mutex’} has no member named ‘__data’
   35 | static __typeof (mutex.__data.__lock) futex_value;
      |                       ^

The easiest is just making this test NPTL specific. The rest looks ok. 

> +
> +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>