Possible bug in _CORE_mutex_Seize()

[Joel Sherrill]

Phil Torre wrote:

>On Tue, Sep 30, 2003 at 03:19:30PM -0700, Till Straumann wrote:
>  
>
>>Joel Sherrill wrote:
>>    
>>
>>>Phil Torre wrote:
>>>
>>>      
>>>
>>>>I'm running now with some of the changes suggested by Till and Joel:
>>>>
>>>>1.    "Paranoia" added to _CORE_mutex_Seize() which checks
>>>>   _Thread_Dispatch_disable_level and throws an internal error if
>>>>   its nonzero when it shouldn't be.
>>>>        
>>>>
>>>Be careful with this one.  I don't think this is the right place to
>>>check it. You can attempt to obtain a mutex in an ISR and
>>>_Thread_Dispatch_disable_level will definitely be non-zero.  You
>>>can't use the Allocator Mutex when _Thread_Dispatch_disable_level
>>>is non-zero but it is conceivable that you could use another mutex.
>>>      
>>>
>
>I am running our application now with the patches attached.  The paranoia
>check in coremutex.h is there, along with the actual bugfix in newlibc.c.
>(If the call to _Workspace_Allocate() fails, we would rather have an error
>propagated back up to the application than have an RTEMS fatal error
>thrown.  I don't know what the majority opinion would be of that.)
>  
>
It is a case that is normally an API level error code because it is 
equivalent to running out of
memory for task stacks on a task create or memory for pending messages 
on a message queue
create.

>I can file a PR and attach my patches to it as soon as a consensus is
>reached about the validity of the paranoia check.
>  
>
Please do.  I am waiting on this one to cut a new tarball.

>-Phil
>
>
>
>
>  
>
>    
>
>  
>
>------------------------------------------------------------------------
>
>--- /tmp/coremutex.h	2003-09-30 15:40:06.000000000 -0700
>+++ cpukit/score/include/rtems/score/coremutex.h	2003-09-29 17:53:23.000000000 -0700
>@@ -26,6 +26,8 @@
> #include <rtems/score/threadq.h>
> #include <rtems/score/priority.h>
> #include <rtems/score/watchdog.h>
>+#include <rtems/score/interr.h>
>+#include <rtems/score/sysstate.h>
>  
> /*
>  *  The following type defines the callout which the API provides
>@@ -152,6 +154,7 @@
>  *         a macro that uses two support routines.
>  */
> 
>+
> #ifndef __RTEMS_APPLICATION__
> RTEMS_INLINE_ROUTINE int _CORE_mutex_Seize_interrupt_trylock(
>   CORE_mutex_Control  *the_mutex,
>@@ -166,6 +169,15 @@
> #define _CORE_mutex_Seize( \
>   _the_mutex, _id, _wait, _timeout, _level ) \
>   do { \
>+	if ( _Thread_Dispatch_disable_level \
>+		&& (_wait) \
>+		&& (_System_state_Get() >= SYSTEM_STATE_BEGIN_MULTITASKING ) \
>+	   ) { \
>+		_Internal_error_Occurred( \
>+  							INTERNAL_ERROR_CORE, \
>+							FALSE, \
>+							18 /* called from wrong environment */); \
>+	} \
>     if ( _CORE_mutex_Seize_interrupt_trylock( _the_mutex, &_level ) ) {  \
>       if ( !_wait ) { \
>         _ISR_Enable( _level ); \
>  
>
>------------------------------------------------------------------------
>
>--- /tmp/old_newlibc.c	2003-09-30 15:29:13.000000000 -0700
>+++ cpukit/libcsupport/src/newlibc.c	2003-09-30 15:37:15.000000000 -0700
>@@ -96,55 +96,72 @@
>   fclose (stderr);
> }
> 
>-
>+/* 
>+ * reent struct allocation moved here from libc_start_hook() to avoid
>+ * mutual exclusion problems when memory is allocated from the start hook.
>+ *
>+ * Memory is also now allocated from the workspace rather than the heap.
>+ *  -- ptorre 9/30/03
>+ */
> rtems_boolean libc_create_hook(
>   rtems_tcb *current_task,
>   rtems_tcb *creating_task
> )
> {
>-  creating_task->libc_reent = NULL;
>-  return TRUE;
>-}
>-
>-/*
>- * Called for all user TASKS (system tasks are MPCI Receive Server and IDLE)
>- */
>-
>-rtems_extension libc_start_hook(
>-  rtems_tcb *current_task,
>-  rtems_tcb *starting_task
>-)
>-{
>   struct _reent *ptr;
> 
>   /*  NOTE: The RTEMS malloc is reentrant without a reent ptr since
>    *        it is based on the Classic API Region Manager.
>    */
> 
>+#define REENT_MALLOCED 0
>+#if REENT_MALLOCED
>   ptr = (struct _reent *) calloc(1, sizeof(struct _reent));
>+#else
>+  /* It is OK to allocate from the workspace because these
>+   * hooks run with thread dispatching disabled.
>+   */
>+  ptr = (struct _reent *) _Workspace_Allocate(sizeof(struct _reent));
>+#endif
> 
>-  if (!ptr)
>-     rtems_fatal_error_occurred(RTEMS_NO_MEMORY);
>+  if (ptr)
>+  {   
>  
> #ifdef __GNUC__
>-  /* GCC extension: structure constants */
>-  _REENT_INIT_PTR((ptr));
>+	  /* GCC extension: structure constants */
>+	  _REENT_INIT_PTR((ptr));
> #else
>-  /* 
>-   *  WARNING: THIS IS VERY DEPENDENT ON NEWLIB!!! 
>-   *           Last visual check was against newlib 1.8.2 but last known
>-   *           use was against 1.7.0.  This is basically an exansion of
>-   *           REENT_INIT() in <sys/reent.h>.
>-   *  NOTE:    calloc() takes care of zeroing fields.
>-   */
>-  ptr->_stdin = &ptr->__sf[0];
>-  ptr->_stdout = &ptr->__sf[1];
>-  ptr->_stderr = &ptr->__sf[2];
>-  ptr->_current_locale = "C";
>-  ptr->_new._reent._rand_next = 1;
>+	  /* 
>+	   *  WARNING: THIS IS VERY DEPENDENT ON NEWLIB!!! 
>+	   *           Last visual check was against newlib 1.8.2 but last known
>+	   *           use was against 1.7.0.  This is basically an exansion of
>+	   *           REENT_INIT() in <sys/reent.h>.
>+	   */
>+	  memset(ptr, 0, sizeof(*ptr));
>+	  ptr->_stdin = &ptr->__sf[0];
>+	  ptr->_stdout = &ptr->__sf[1];
>+	  ptr->_stderr = &ptr->__sf[2];
>+	  ptr->_current_locale = "C";
>+	  ptr->_new._reent._rand_next = 1;
> #endif
> 
>-  starting_task->libc_reent = ptr;
>+	  creating_task->libc_reent = ptr;
>+	  return TRUE;
>+  }	  
>+  else
>+      return FALSE;
>+
>+}
>+
>+/*
>+ * Called for all user TASKS (system tasks are MPCI Receive Server and IDLE)
>+ */
>+
>+rtems_extension libc_start_hook(
>+  rtems_tcb *current_task,
>+  rtems_tcb *starting_task
>+)
>+{
> }
> 
> /*
>@@ -204,7 +221,11 @@
>   if (ptr && ptr != &libc_global_reent) {
>     _wrapup_reent(ptr);
>     _reclaim_reent(ptr);
>+#if REENT_MALLOCED
>     free(ptr);
>+#else
>+	_Workspace_Free(ptr);
>+#endif
>   }
> 
>   deleted_task->libc_reent = NULL;
>  
>