BUGS: FreeRTOS v9.0.0 StaticTask_t size too small with INCLUDE_xTaskAbortDelay or portUSING_MPU_WRAPPERS

Kernel
1

danhepler wrote on Tuesday, September 13, 2016:

There are a total of three bugs.

Two bugs are found in StaticTask_t which are evident comparing the definition of StaticTask_t in FreeRTOS.h and TCB_t in task.c. This causes a false stack overflow failure because the stack overflow check fails on the first byte (ucDelayAborted is written as pdFALSE or pdTRUE). These may also cause weird behaviour with xTaskAbortDelayalthough I have not verified this.

typedef struct xSTATIC_TCB
{
	void				*pxDummy1;
	#if ( portUSING_MPU_WRAPPERS == 1 )
		xMPU_SETTINGS	xDummy2;
	#endif
	StaticListItem_t	xDummy3[ 2 ];
	UBaseType_t			uxDummy5;
	void				*pxDummy6;
	uint8_t				ucDummy7[ configMAX_TASK_NAME_LEN ];
	#if ( portSTACK_GROWTH > 0 )
		void			*pxDummy8;
	#endif
	#if ( portCRITICAL_NESTING_IN_TCB == 1 )
		UBaseType_t		uxDummy9;
	#endif
	#if ( configUSE_TRACE_FACILITY == 1 )
		UBaseType_t		uxDummy10[ 2 ];
	#endif
	#if ( configUSE_MUTEXES == 1 )
		UBaseType_t		uxDummy12[ 2 ];
	#endif
	#if ( configUSE_APPLICATION_TASK_TAG == 1 )
		void			*pxDummy14;
	#endif
	#if( configNUM_THREAD_LOCAL_STORAGE_POINTERS > 0 )
		void			*pvDummy15[ configNUM_THREAD_LOCAL_STORAGE_POINTERS ];
	#endif
	#if ( configGENERATE_RUN_TIME_STATS == 1 )
		uint32_t		ulDummy16;
	#endif
	#if ( configUSE_NEWLIB_REENTRANT == 1 )
		struct	_reent	xDummy17;
	#endif
	#if ( configUSE_TASK_NOTIFICATIONS == 1 )
		uint32_t 		ulDummy18;
		uint8_t 		ucDummy19;
	#endif
	#if( ( configSUPPORT_STATIC_ALLOCATION == 1 ) && ( configSUPPORT_DYNAMIC_ALLOCATION == 1 ) )
		uint8_t			uxDummy20;
	#endif

} StaticTask_t;
  1. StaticTask_t should have the last uxDummy20 conditional compile flag changed to the following to include the ports with portUSING_MPU_WRAPPERS enabled:
	#if( ( ( configSUPPORT_STATIC_ALLOCATION == 1 ) && ( configSUPPORT_DYNAMIC_ALLOCATION == 1 ) ) || ( portUSING_MPU_WRAPPERS == 1 ) )
		uint8_t			uxDummy20;
	#endif
  1. StaticTask_t should add a missing ucDummy21 to account for ucDelayAborted if INCLUDE_xTaskAbortDelay is enabled:
	#if( INCLUDE_xTaskAbortDelay == 1 )
		uint8_t			ucDummy21;
	#endif
  1. Additionally, there is a very misleading comment in xTaskCreateStatic() in tasks.c:
	TaskHandle_t xTaskCreateStatic(	TaskFunction_t pxTaskCode,
									const char * const pcName,
									const uint32_t ulStackDepth,
									void * const pvParameters,
									UBaseType_t uxPriority,
									StackType_t * const puxStackBuffer,
									StaticTask_t * const pxTaskBuffer ) /*lint !e971 Unqualified char types are allowed for strings and single characters only. */
	{
	TCB_t *pxNewTCB;
	TaskHandle_t xReturn;

		configASSERT( puxStackBuffer != NULL );
		configASSERT( pxTaskBuffer != NULL );

		if( ( pxTaskBuffer != NULL ) && ( puxStackBuffer != NULL ) )
		{
			/* The memory used for the task's TCB and stack are passed into this
			function - use them. */
			pxNewTCB = ( TCB_t * ) pxTaskBuffer; /*lint !e740 Unusual cast is ok as the structures are designed to have the same alignment, and the size is checked by an assert. */

The size most certainly is NOT checked by an assert. A compile-time assert is preferable:

_Static_assert( sizeof(StaticTask_t) == sizeof(TCB_t), "StaticTask_t must be same size as TCB_t" );

Or at least a run-time assert would be OK:

configASSERT( sizeof(StaticTask_t) == sizeof(TCB_t) );

Hope this helps someone and can make it into the next rev of FreeRTOS.

2

rtel wrote on Wednesday, September 14, 2016:

Thanks for taking the time to point this out. I have updated the StaticTask_t structure accordingly and will post a ‘known issue’.

I will have to check the assert() - there definitely used to be one but the issue was getting it through all the compilers we use without warnings being generated a “condition is always false” warning.

3

rtel wrote on Wednesday, September 14, 2016:

Added in:

#if( configASSERT_DEFINED == 1 )
{
		/* Sanity check that the size of the structure used to declare a
		variable of type StaticTask_t equals the size of the real task
		structure. */
		volatile size_t xSize = sizeof( StaticTask_t );
		configASSERT( xSize == sizeof( tskTCB ) );
}
#endif /* configASSERT_DEFINED */

Which is how it was done with the static queue structure.

4

danhepler wrote on Wednesday, September 14, 2016:

No problem, keep up the great work!