pthreads: Add more robustness characteristics: pthread_mutex_lock() and trylock() will now return EOWNERDEAD if the mutex is locked by a thread that no longer exists. Add pthread_mutex_consistent() to recover from this situation.

TODO

@@ -1,4 +1,4 @@
-NuttX TODO List (Last updated March 14, 2017)
+NuttX TODO List (Last updated March 26, 2017)
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 This file summarizes known NuttX bugs, limitations, inconsistencies with
@@ -14,7 +14,7 @@ nuttx/:
   (1)  Memory Management (mm/)
   (0)  Power Management (drivers/pm)
   (3)  Signals (sched/signal, arch/)
-  (2)  pthreads (sched/pthread)
+  (4)  pthreads (sched/pthread)
   (0)  Message Queues (sched/mqueue)
   (8)  Kernel/Protected Build
   (3)  C++ Support
@@ -346,7 +346,7 @@ o Signals (sched/signal, arch/)
   Priority:    Low. Even if there are only 31 usable signals, that is still a lot.
 
 o pthreads (sched/pthreads)
-  ^^^^^^^^^^^^^^^^^
+  ^^^^^^^^^^^^^^^^^^^^^^^^^
 
   Title:       PTHREAD_PRIO_PROTECT
   Description: Extend pthread_mutexattr_setprotocol().  It should support
@@ -448,6 +448,36 @@ o pthreads (sched/pthreads)
   Status:      Not really open.  This is just the way it is.
   Priority:    Nothing additional is planned.
 
+  Title:       PTHREAD FILES IN WRONG LOCATTION
+  Description: There are many pthread interface functions in files located in
+               sched/pthread.  These should be moved from that location to
+               libc/pthread.  In the flat build, this really does not matter,
+               but in the protected build that location means that system calls
+               are required to access the pthread interface functions.
+  Status:      Open
+  Priority:    Medium-low. Priority may be higher if system call overheade becomes
+               an issue.
+
+  Title:       ROBUST MUTEX ATTRIBUTE NOT SUPPORTED
+  Description: In NuttX, all mutexes are 'robust' in the sense that an attmpt
+               to lock a mutex will return EOWNDERDEAD if the holder of the
+               mutex has died.  Unlocking of a mutex will fail if the caller
+               is not the holder of the mutex.
+
+               POSIX, however, requires that there be a mutex attribute called
+               robust that determines which behavior is supported.  non-robust
+               should be the default.  NuttX does not support this attribute
+               and robust behavior is the default and only supported behavior.
+
+               The spec is not clear, but I think there there is also missing
+               logic when the thread exits.  I believe that the next highest
+               prority thread waiting for the mutex should be awakend and
+               pthread_mutex_lock() should return EOWNERDEAD.
+               That does not happen now.  They will just remain blocked.
+  Status:      Open
+  Priority:    Low.  The non-robust behavior is dangerous and really should never
+               be used.
+
 o Message Queues (sched/mqueue)
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 

include/pthread.h

@@ -1,7 +1,7 @@
 /********************************************************************************
  * include/pthread.h
  *
- *   Copyright (C) 2007-2009, 2011-2012, 2015-2016 Gregory Nutt. All rights reserved.
+ *   Copyright (C) 2007-2009, 2011-2012, 2015-2017 Gregory Nutt. All rights reserved.
  *   Author: Gregory Nutt <gnutt@nuttx.org>
  *
  * Redistribution and use in source and binary forms, with or without
@@ -424,6 +424,10 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex);
 int pthread_mutex_trylock(FAR pthread_mutex_t *mutex);
 int pthread_mutex_unlock(FAR pthread_mutex_t *mutex);
 
+/* Make sure that the pthread mutex is in a consistent state */
+
+int pthread_mutex_consistent(FAR pthread_mutex_t *mutex);
+
 /* Operations on condition variables */
 
 int pthread_condattr_init(FAR pthread_condattr_t *attr);

sched/pthread/Make.defs

@@ -39,6 +39,7 @@ CSRCS += pthread_create.c pthread_exit.c pthread_join.c pthread_detach.c
 CSRCS += pthread_yield.c pthread_getschedparam.c pthread_setschedparam.c
 CSRCS += pthread_mutexinit.c pthread_mutexdestroy.c
 CSRCS += pthread_mutexlock.c pthread_mutextrylock.c pthread_mutexunlock.c
+CSRCS += pthread_mutexconsistent.c
 CSRCS += pthread_condinit.c pthread_conddestroy.c
 CSRCS += pthread_condwait.c pthread_condsignal.c pthread_condbroadcast.c
 CSRCS += pthread_barrierinit.c pthread_barrierdestroy.c pthread_barrierwait.c

sched/pthread/pthread_mutexconsistent.c

@@ -0,0 +1,138 @@
+/****************************************************************************
+ * sched/pthread/pthread_mutexconsistent.c
+ *
+ *   Copyright (C) 2017 Gregory Nutt. All rights reserved.
+ *   Author: Gregory Nutt <gnutt@nuttx.org>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in
+ *    the documentation and/or other materials provided with the
+ *    distribution.
+ * 3. Neither the name NuttX nor the names of its contributors may be
+ *    used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+ * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+ * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
+ * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <pthread.h>
+#include <sched.h>
+#include <assert.h>
+#include <errno.h>
+
+#include <nuttx/semaphore.h>
+
+#include "pthread/pthread.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: pthread_mutex_consistent
+ *
+ * Description:
+ *   If mutex is a robust mutex in an inconsistent state, the
+ *   pthread_mutex_consistent() function can be used to mark the state
+ *   protected by the mutex referenced by mutex as consistent again.
+ *
+ *   If an owner of a robust mutex terminates while holding the mutex, the
+ *   mutex becomes inconsistent and the next thread that acquires the mutex
+ *   lock will be notified of the state by the return value EOWNERDEAD.
+ *   In this case, the mutex does not become normally usable again until the
+ *   state is marked consistent.
+ *
+ *   If the thread which acquired the mutex lock with the return value
+ *   EOWNERDEAD terminates before calling either pthread_mutex_consistent()
+ *   or pthread_mutex_unlock(), the next thread that acquires the mutex lock
+ *   will be notified about the state of the mutex by the return value
+ *   EOWNERDEAD.
+ *
+ *   The behavior is undefined if the value specified by the mutex argument
+ *   to pthread_mutex_consistent() does not refer to an initialized mutex.
+ *
+ * Input Parameters:
+ *   mutex -- a reference to the mutex to be made consistent
+ *
+ * Returned Value:
+ *   Upon successful completion, the pthread_mutex_consistent() function
+ *   will return zero. Otherwise, an error value will be returned to
+ *   indicate the error.
+ *
+ ****************************************************************************/
+
+int pthread_mutex_consistent(FAR pthread_mutex_t *mutex)
+{
+  int ret = EINVAL;
+  int status;
+
+  sinfo("mutex=0x%p\n", mutex);
+  DEBUGASSERT(mutex != NULL);
+
+  if (mutex != NULL)
+    {
+      /* Make sure the mutex is stable while we make the following checks. */
+
+      sched_lock();
+
+      /* Is the mutex available? */
+
+      DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */
+      if (mutex->pid >= 0)
+        {
+          /* No.. Verify that the PID still exists.  We may be destroying
+           * the mutex after cancelling a pthread and the mutex may have
+           * been in a bad state owned by the dead pthread.  NOTE: The
+           * folling is unspecified behavior (see pthread_mutex_consistent()).
+           *
+           * If the holding thread is still valid, then we should be able to
+           * map its PID to the underlying TCB. That is what sched_gettcb()
+           * does.
+           */
+
+          if (sched_gettcb(mutex->pid) == NULL)
+            {
+              /* The thread associated with the PID no longer exists */
+
+              mutex->pid = -1;
+
+              /* Reset the semaphore.  This has the same affect as if the
+               * dead task had called pthread_mutex_unlock().
+               */
+
+              status = sem_reset((FAR sem_t *)&mutex->sem, 1);
+              ret = (status != OK) ? get_errno() : OK;
+            }
+        }
+
+      sched_unlock();
+      ret = OK;
+    }
+
+  sinfo("Returning %d\n", ret);
+  return ret;
+}

sched/pthread/pthread_mutexdestroy.c

@@ -1,7 +1,7 @@
 /****************************************************************************
  * sched/pthread/pthread_mutexdestroy.c
  *
- *   Copyright (C) 2007-2009 Gregory Nutt. All rights reserved.
+ *   Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved.
  *   Author: Gregory Nutt <gnutt@nuttx.org>
  *
  * Redistribution and use in source and binary forms, with or without
@@ -46,6 +46,8 @@
 #include <errno.h>
 #include <debug.h>
 
+#include <nuttx/semaphore.h>
+
 #include "pthread/pthread.h"
 
 /****************************************************************************
@@ -70,60 +72,86 @@
 
 int pthread_mutex_destroy(FAR pthread_mutex_t *mutex)
 {
-  int ret = OK;
+  int ret = EINVAL;
   int status;
 
   sinfo("mutex=0x%p\n", mutex);
+  DEBUGASSERT(mutex != NULL);
 
-  if (mutex == NULL)
+  if (mutex != NULL)
     {
-      ret = EINVAL;
+      /* Make sure the semaphore is stable while we make the following checks */
-    }
-  else
-    {
-      /* Make sure the semaphore is stable while we make the following
-       * checks
-       */
 
       sched_lock();
 
-      /* Is the semaphore available? */
+      /* Is the mutex available? */
 
-      if (mutex->pid != -1)
+      if (mutex->pid >= 0)
         {
-#ifndef CONFIG_DISABLE_SIGNALS
+          DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */
-          /* Verify that the PID still exists.  We may be destroying the
+
-           * mutex after cancelling a pthread and the mutex may have been
+          /* No.. Verify that the PID still exists.  We may be destroying
-           * in a bad state owned by the dead pthread.
+           * the mutex after cancelling a pthread and the mutex may have
+           * been in a bad state owned by the dead pthread.  NOTE: The
+           * following behavior is unspecified for pthread_mutex_destroy()
+           * (see pthread_mutex_consistent()).
+           *
+           * If the holding thread is still valid, then we should be able to
+           * map its PID to the underlying TCB. That is what sched_gettcb()
+           * does.
            */
 
-         ret = kill(mutex->pid, 0);
+          if (sched_gettcb(mutex->pid) == NULL)
-         if (ret < 0)
             {
               /* The thread associated with the PID no longer exists */
 
               mutex->pid = -1;
 
-             /* Destroy the semaphore */
+              /* Reset the semaphore.  If threads are were on this
+               * semaphore, then this will awakened them and make
+               * destruction of the semaphore impossible here.
+               */
 
+              status = sem_reset((FAR sem_t *)&mutex->sem, 1);
+              if (status < 0)
+                {
+                  ret = -status;
+                }
+
+              /* Check if the reset caused some other thread to lock the
+               * mutex.
+               */
+
+              else if (mutex->pid != -1)
+                {
+                  /* Yes.. then we cannot destroy the mutex now. */
+
+                  ret = EBUSY;
+                }
+
+              /* Destroy the underlying semaphore */
+
+              else
+                {
                   status = sem_destroy((FAR sem_t *)&mutex->sem);
                   ret = (status != OK) ? get_errno() : OK;
                 }
+            }
           else
-#endif
             {
               ret = EBUSY;
             }
         }
       else
         {
-          /* Destroy the semaphore */
+          /* Destroy the semaphore
+           *
+           * REVISIT:  What if there are threads waiting on the semaphore?
+           * Perhaps this logic should all sem_reset() first?
+           */
 
           status = sem_destroy((FAR sem_t *)&mutex->sem);
-          if (status != OK)
+          ret = ((status != OK) ? get_errno() : OK);
-            {
-              ret = get_errno();
-            }
         }
 
       sched_unlock();

sched/pthread/pthread_mutexlock.c

@@ -1,7 +1,7 @@
 /****************************************************************************
  * sched/pthread/pthread_mutexlock.c
  *
- *   Copyright (C) 2007-2009 Gregory Nutt. All rights reserved.
+ *   Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved.
  *   Author: Gregory Nutt <gnutt@nuttx.org>
  *
  * Redistribution and use in source and binary forms, with or without
@@ -42,9 +42,12 @@
 #include <unistd.h>
 #include <pthread.h>
 #include <sched.h>
+#include <assert.h>
 #include <errno.h>
 #include <debug.h>
 
+#include <nuttx/sched.h>
+
 #include "pthread/pthread.h"
 
 /****************************************************************************
@@ -104,15 +107,12 @@
 int pthread_mutex_lock(FAR pthread_mutex_t *mutex)
 {
   int mypid = (int)getpid();
-  int ret = OK;
+  int ret = EINVAL;
 
   sinfo("mutex=0x%p\n", mutex);
+  DEBUGASSERT(mutex != NULL);
 
-  if (!mutex)
+  if (mutex != NULL)
-    {
-      ret = EINVAL;
-    }
-  else
     {
       /* Make sure the semaphore is stable while we make the following
        * checks.  This all needs to be one atomic action.
@@ -120,7 +120,7 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex)
 
       sched_lock();
 
-      /* Does this task already hold the semaphore? */
+      /* Does this thread already hold the semaphore? */
 
       if (mutex->pid == mypid)
         {
@@ -129,9 +129,12 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex)
 #ifdef CONFIG_MUTEX_TYPES
           if (mutex->type == PTHREAD_MUTEX_RECURSIVE)
             {
-              /* Yes... just increment the number of locks held and return success */
+              /* Yes... just increment the number of locks held and return
+               * success.
+               */
 
               mutex->nlocks++;
+              ret = OK;
             }
           else
 #endif
@@ -139,19 +142,39 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex)
               /* No, then we would deadlock... return an error (default
                * behavior is like PTHREAD_MUTEX_ERRORCHECK)
                *
-               * NOTE:  This is non-compliant behavior for the case of a
+               * NOTE: This is the correct behavior for a 'robust', NORMAL
-               * NORMAL mutex.  In that case, it the deadlock condition should
+               * mutex.  Compiant behavior for non-robust mutex should not
-               * not be detected and the thread should be permitted to
+               * include these checks.  In that case, it the deadlock
-               * deadlock.
+               * condition should not be detected and the thread should be
+               * permitted to deadlock.
                */
 
               serr("ERROR: Returning EDEADLK\n");
               ret = EDEADLK;
             }
         }
+
+      /* The calling thread does not hold the semaphore.  The correct
+       * behavior for the 'robust' mutex is to verify that the holder of the
+       * mutex is still valid.  This is protection from the case
+       * where the holder of the mutex has exitted without unlocking it.
+       */
+
+      else if (mutex->pid > 0 && sched_gettcb(mutex->pid) == NULL)
+        {
+          DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */
+
+          /* A thread holds the mutex, but there is no such thread.  POSIX
+           * requires that the 'robust' mutex return EOWNERDEAD in this case.
+           * It is then the caller's responsibility to call pthread_mutx_consistent()
+           * fo fix the mutex.
+           */
+
+          ret = EOWNERDEAD;
+        }
       else
         {
-          /* Take the semaphore */
+          /* Take the underlying semaphore, waiting if necessary */
 
           ret = pthread_takesemaphore((FAR sem_t *)&mutex->sem);
 
@@ -159,7 +182,7 @@ int pthread_mutex_lock(FAR pthread_mutex_t *mutex)
            * that we own it.
            */
 
-          if (!ret)
+          if (ret == OK)
             {
               mutex->pid    = mypid;
 #ifdef CONFIG_MUTEX_TYPES

sched/pthread/pthread_mutextrylock.c

@@ -1,7 +1,7 @@
 /****************************************************************************
  * sched/pthread/pthread_mutextrylock.c
  *
- *   Copyright (C) 2007-2009 Gregory Nutt. All rights reserved.
+ *   Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved.
  *   Author: Gregory Nutt <gnutt@nuttx.org>
  *
  * Redistribution and use in source and binary forms, with or without
@@ -43,6 +43,7 @@
 #include <pthread.h>
 #include <semaphore.h>
 #include <sched.h>
+#include <assert.h>
 #include <errno.h>
 #include <debug.h>
 
@@ -83,15 +84,13 @@
 
 int pthread_mutex_trylock(FAR pthread_mutex_t *mutex)
 {
-  int ret = OK;
+  int status;
+  int ret = EINVAL;
 
   sinfo("mutex=0x%p\n", mutex);
+  DEBUGASSERT(mutex != NULL);
 
-  if (!mutex)
+  if (mutex != NULL)
-    {
-      ret = EINVAL;
-    }
-  else
     {
       int mypid = (int)getpid();
 
@@ -103,7 +102,8 @@ int pthread_mutex_trylock(FAR pthread_mutex_t *mutex)
 
       /* Try to get the semaphore. */
 
-      if (sem_trywait((FAR sem_t *)&mutex->sem) == OK)
+      status = sem_trywait((FAR sem_t *)&mutex->sem);
+      if (status == OK)
         {
           /* If we successfully obtained the semaphore, then indicate
            * that we own it.
@@ -117,33 +117,63 @@ int pthread_mutex_trylock(FAR pthread_mutex_t *mutex)
               mutex->nlocks = 1;
             }
 #endif
+          ret = OK;
         }
 
-      /* Was it not available? */
+      /* sem_trywait failed */
 
-      else if (get_errno() == EAGAIN)
+      else
+        {
+          /* Did it fail because the semaphore was not avaialabl? */
+
+          int errcode = get_errno();
+          if (errcode == EAGAIN)
             {
 #ifdef CONFIG_MUTEX_TYPES
-
+              /* Check if recursive mutex was locked by the calling thread. */
-          /* Check if recursive mutex was locked by ourself. */
 
               if (mutex->type == PTHREAD_MUTEX_RECURSIVE && mutex->pid == mypid)
                 {
                   /* Increment the number of locks held and return successfully. */
 
                   mutex->nlocks++;
+                  ret = OK;
                 }
               else
-            {
-              ret = EBUSY;
-            }
-#else
-          ret = EBUSY;
 #endif
+              /* The calling thread does not hold the semaphore.  The correct
+               * behavior for the 'robust' mutex is to verify that the holder of
+               * the mutex is still valid.  This is protection from the case
+               * where the holder of the mutex has exitted without unlocking it.
+               */
+
+              if (mutex->pid > 0 && sched_gettcb(mutex->pid) == NULL)
+                {
+                  DEBUGASSERT(mutex->pid != 0); /* < 0: available, >0 owned, ==0 error */
+
+                  /* A thread holds the mutex, but there is no such thread. 
+                   * POSIX requires that the 'robust' mutex return EOWNERDEAD
+                   * in this case. It is then the caller's responsibility to
+                   * call pthread_mutx_consistent() fo fix the mutex.
+                   */
+
+                  ret = EOWNERDEAD;
                 }
+
+              /* The mutex is locked by another, active thread */
+
               else
                 {
-          ret = EINVAL;
+                  ret = EBUSY;
+                }
+            }
+
+          /* Some other, unhandled error occurred */
+
+          else
+            {
+              ret = errcode;
+            }
         }
 
       sched_unlock();

sched/pthread/pthread_mutexunlock.c

@@ -1,7 +1,7 @@
 /****************************************************************************
  * sched/pthread/pthread_mutexunlock.c
  *
- *   Copyright (C) 2007-2009 Gregory Nutt. All rights reserved.
+ *   Copyright (C) 2007-2009, 2017 Gregory Nutt. All rights reserved.
  *   Author: Gregory Nutt <gnutt@nuttx.org>
  *
  * Redistribution and use in source and binary forms, with or without
@@ -80,15 +80,12 @@
 
 int pthread_mutex_unlock(FAR pthread_mutex_t *mutex)
 {
-  int ret = OK;
+  int ret = EINVAL;
 
   sinfo("mutex=0x%p\n", mutex);
+  DEBUGASSERT(mutex != NULL);
 
-  if (!mutex)
+  if (mutex != NULL)
-    {
-      ret = EINVAL;
-    }
-  else
     {
       /* Make sure the semaphore is stable while we make the following
        * checks.  This all needs to be one atomic action.
@@ -108,8 +105,7 @@ int pthread_mutex_unlock(FAR pthread_mutex_t *mutex)
            * the mutex."
            *
            * For the case of the non-robust PTHREAD_MUTEX_NORMAL mutex,
-           * the behavior is undefined.  Here we treat that type as though
+           * the behavior is undefined.
-           * it were PTHREAD_MUTEX_ERRORCHECK type and just return an error.
            */
 
           serr("ERROR: Holder=%d returning EPERM\n", mutex->pid);
@@ -127,6 +123,7 @@ int pthread_mutex_unlock(FAR pthread_mutex_t *mutex)
            */
 
           mutex->nlocks--;
+          ret = OK;
         }
 #endif