Possible bug in _CORE_mutex_Seize()

[Till Straumann]

The patches look good to me...

-- T.

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