Merge pull request #472 from jphickey/fix-470-binsem-lock

Fix #470, Binary sem task delete issues

src/os/posix/src/os-impl-binsem.c

@@ -28,9 +28,38 @@
 #include "os-shared-binsem.h"
 #include "os-impl-binsem.h"
 
+/*
+ * This controls the maximum time the that the calling thread will wait to
+ * acquire the condition mutex before returning an error.
+ *
+ * Under normal conditions, this lock is held by giving/taking threads very
+ * briefly, so the lock should be available with minimal delay. However,
+ * if the "taking" thread is canceled or exits abnormally without releasing the
+ * lock, it means any other task accessing the sem can get blocked indefinitely.
+ *
+ * There should be no reason for a user to configure this, as it should
+ * not be relevant in a normally operating system. This only prevents a
+ * deadlock condition in off-nominal circumstances.
+ */
+static const struct timespec OS_POSIX_BINSEM_MAX_WAIT =
+{
+ .tv_sec = 2,
+ .tv_nsec = 0
+};
+
+
 /* Tables where the OS object information is stored */
 OS_impl_binsem_internal_record_t OS_impl_bin_sem_table [OS_MAX_BIN_SEMAPHORES];
 
+/*---------------------------------------------------------------------------------------
+ * Helper function for releasing the mutex in case the thread
+ * executing pthread_condwait() is canceled.
+ ----------------------------------------------------------------------------------------*/
+void OS_Posix_BinSemReleaseMutex(void *mut)
+{
+ pthread_mutex_unlock(mut);
+}
+
 /****************************************************************************************
  BINARY SEMAPHORE API
  ***************************************************************************************/
@@ -244,10 +273,13 @@ int32 OS_BinSemGive_Impl ( uint32 sem_id )
  * alternative of having a BinSemGive not wake up the other thread is a bigger issue.
  *
  * Counting sems do not suffer from this, as there is a native POSIX mechanism for those.
+ *
+ * Note: This lock should be readily available, with only minimal delay if any.
+ * If a long delay occurs here, it means something is fundamentally wrong.
  */
 
  /* Lock the mutex ( not the table! ) */
- if ( pthread_mutex_lock(&(sem->id)) != 0 )
+ if ( pthread_mutex_timedlock(&sem->id, &OS_POSIX_BINSEM_MAX_WAIT) != 0 )
  {
  return(OS_SEM_FAILURE);
  }
@@ -279,7 +311,7 @@ int32 OS_BinSemFlush_Impl (uint32 sem_id)
  sem = &OS_impl_bin_sem_table[sem_id];
 
  /* Lock the mutex ( not the table! ) */
- if ( pthread_mutex_lock(&(sem->id)) != 0 )
+ if ( pthread_mutex_timedlock(&sem->id, &OS_POSIX_BINSEM_MAX_WAIT) != 0 )
  {
  return(OS_SEM_FAILURE);
  }
@@ -311,12 +343,21 @@ static int32 OS_GenericBinSemTake_Impl (OS_impl_binsem_internal_record_t *sem, c
  sig_atomic_t flush_count;
  int32 return_code;
 
+ /*
+ * Note - this lock should be quickly available - should not delay here.
+ * The main delay is in the pthread_cond_wait() below.
+ */
  /* Lock the mutex ( not the table! ) */
- if ( pthread_mutex_lock(&(sem->id)) != 0 )
+ if ( pthread_mutex_timedlock(&sem->id, &OS_POSIX_BINSEM_MAX_WAIT) != 0 )
  {
  return(OS_SEM_FAILURE);
  }
 
+ /* because pthread_cond_wait() is also a cancellation point,
+ * this uses a cleanup handler to ensure that if canceled during this call,
+ * the mutex is also released */
+ pthread_cleanup_push(OS_Posix_BinSemReleaseMutex, &sem->id);
+
  return_code = OS_SUCCESS;
 
  /*
@@ -362,7 +403,12 @@ static int32 OS_GenericBinSemTake_Impl (OS_impl_binsem_internal_record_t *sem, c
  sem->current_value = 0;
  }
 
- pthread_mutex_unlock(&(sem->id));
+ /*
+ * Pop the cleanup handler.
+ * Passing "true" means it will be executed, which
+ * handles releasing the mutex.
+ */
+ pthread_cleanup_pop(true);
 
  return return_code;
 } /* end OS_GenericBinSemTake_Impl */

src/tests/bin-sem-test/bin-sem-test.c

@@ -137,10 +137,27 @@ void task_1(void)
 void BinSemCheck(void)
 {
  uint32 status;
+ OS_bin_sem_prop_t bin_sem_prop;
+
+ /* Delete the task, which should be pending in OS_BinSemTake() */
+ status = OS_TaskDelete(task_1_id);
+ UtAssert_True(status == OS_SUCCESS, "OS_TaskDelete Rc=%d", (int)status);
 
  status = OS_TimerDelete(timer_id);
  UtAssert_True(status == OS_SUCCESS, "OS_TimerDelete Rc=%d", (int)status);
 
+ OS_TaskDelay(100);
+
+ /* Confirm that the semaphore itself is still operational after task deletion */
+ status = OS_BinSemGive(bin_sem_id);
+ UtAssert_True(status == OS_SUCCESS, "BinSem give Rc=%d", (int)status);
+ status = OS_BinSemGetInfo (bin_sem_id, &bin_sem_prop);
+ UtAssert_True(status == OS_SUCCESS, "BinSem value=%d Rc=%d", (int)bin_sem_prop.value, (int)status);
+ status = OS_BinSemTake(bin_sem_id);
+ UtAssert_True(status == OS_SUCCESS, "BinSem take Rc=%d", (int)status);
+ status = OS_BinSemDelete(bin_sem_id);
+ UtAssert_True(status == OS_SUCCESS, "BinSem delete Rc=%d", (int)status);
+
  UtAssert_True(counter < timer_counter, "Task counter (%d) < timer counter (%d)", (int)counter, (int)timer_counter);
  UtAssert_True(task_1_failures == 0, "Task 1 failures = %u", (unsigned int)task_1_failures);
  UtAssert_True(timer_failures == 0, "Timer failures = %u", (unsigned int)timer_failures);