From f94bc89108ffca538cf91d5856149960a5d4be81 Mon Sep 17 00:00:00 2001 From: Kody Stribrny <89810515+kstribrnAmzn@users.noreply.github.com> Date: Tue, 28 Jan 2025 09:13:17 -0800 Subject: [PATCH 1/2] fix: SA violation fixes and simplification for idle task length restrictions (#1227) fix: SA violation fixes and simplification for idle task length restrictions This change: * Removes the dependency on strings.h for the prvCreateIdleTask function * Resolves several static analysis violations reported by tools like Parasoft Builds off of - https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/1203 --- MISRA.md | 19 +++++++++++++++++++ tasks.c | 46 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/MISRA.md b/MISRA.md index 4355ec678..b5941037f 100644 --- a/MISRA.md +++ b/MISRA.md @@ -115,6 +115,25 @@ _Ref 11.5.5_ because data storage buffers are implemented as uint8_t arrays for the ease of sizing, alignment and access. +#### Rule 14.3 + +MISRA C-2012 Rule 14.3: Controlling expressions shall not be invariant. + +_Ref 14.3_ + - The `configMAX_TASK_NAME_LEN` and `taskRESERVED_TASK_NAME_LENGTH` are + evaluated to constants at compile time and may vary based on the build + configuration. + +#### Rule 18.1 + +MISRA C-2012 Rule 18.1: A pointer resulting from arithmetic on a pointer operand +shall address an element of the same array as that pointer operand. + +_Ref 18.1_ + - Array access remains within bounds since either the null terminator in + the IDLE task name will break the loop, or the loop will break normally + if the array size is smaller than the IDLE task name length. + #### Rule 21.6 MISRA C-2012 Rule 21.6: The Standard Library input/output functions shall not diff --git a/tasks.c b/tasks.c index d7153f680..518c9e87f 100644 --- a/tasks.c +++ b/tasks.c @@ -156,6 +156,23 @@ #define configIDLE_TASK_NAME "IDLE" #endif +#if ( configNUMBER_OF_CORES > 1 ) + /* Reserve space for Core ID and null termination. */ + #if ( configMAX_TASK_NAME_LEN < 2U ) + #error Minimum required task name length is 2. Please increase configMAX_TASK_NAME_LEN. + #endif + #define taskRESERVED_TASK_NAME_LENGTH 2U + +#elif ( configNUMBER_OF_CORES > 9 ) + #warning Please increase taskRESERVED_TASK_NAME_LENGTH. 1 character is insufficient to store the core ID. +#else + /* Reserve space for null termination. */ + #if ( configMAX_TASK_NAME_LEN < 1U ) + #error Minimum required task name length is 1. Please increase configMAX_TASK_NAME_LEN. + #endif + #define taskRESERVED_TASK_NAME_LENGTH 1U +#endif /* if ( ( configNUMBER_OF_CORES > 1 ) */ + #if ( configUSE_PORT_OPTIMISED_TASK_SELECTION == 0 ) /* If configUSE_PORT_OPTIMISED_TASK_SELECTION is 0 then task selection is @@ -3527,21 +3544,26 @@ static BaseType_t prvCreateIdleTasks( void ) BaseType_t xCoreID; char cIdleName[ configMAX_TASK_NAME_LEN ] = { 0 }; TaskFunction_t pxIdleTaskFunction = NULL; - BaseType_t xIdleTaskNameIndex; - BaseType_t xIdleNameLen; - BaseType_t xCopyLen; + UBaseType_t xIdleTaskNameIndex; - configASSERT( ( configIDLE_TASK_NAME != NULL ) && ( configMAX_TASK_NAME_LEN > 3 ) ); - - /* The length of the idle task name is limited to the minimum of the length - * of configIDLE_TASK_NAME and configMAX_TASK_NAME_LEN - 2, keeping space - * for the core ID suffix and the null-terminator. */ - xIdleNameLen = strlen( configIDLE_TASK_NAME ); - xCopyLen = xIdleNameLen < ( configMAX_TASK_NAME_LEN - 2 ) ? xIdleNameLen : ( configMAX_TASK_NAME_LEN - 2 ); - - for( xIdleTaskNameIndex = ( BaseType_t ) 0; xIdleTaskNameIndex < xCopyLen; xIdleTaskNameIndex++ ) + /* MISRA Ref 14.3.1 [Configuration dependent invariant] */ + /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-143. */ + /* coverity[misra_c_2012_rule_14_3_violation] */ + for( xIdleTaskNameIndex = 0U; xIdleTaskNameIndex < ( configMAX_TASK_NAME_LEN - taskRESERVED_TASK_NAME_LENGTH ); xIdleTaskNameIndex++ ) { + /* MISRA Ref 18.1.1 [Configuration dependent bounds checking] */ + /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-181. */ + /* coverity[misra_c_2012_rule_18_1_violation] */ cIdleName[ xIdleTaskNameIndex ] = configIDLE_TASK_NAME[ xIdleTaskNameIndex ]; + + if( cIdleName[ xIdleTaskNameIndex ] == ( char ) 0x00 ) + { + break; + } + else + { + mtCOVERAGE_TEST_MARKER(); + } } /* Ensure null termination. */ From ad4e7238293cbe994e7450e1bc415136875da322 Mon Sep 17 00:00:00 2001 From: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com> Date: Wed, 29 Jan 2025 07:27:30 +0530 Subject: [PATCH 2/2] Mark mutex as robust to prevent deadlocks (#1233) Mark mutex as robust to prevent deadlocks Prevent application hangs that occur when a thread dies while holding a mutex, particularly during vTaskEndScheduler or exit calls. This is achieved by setting the PTHREAD_MUTEX_ROBUST attribute on the mutex. Fixes: - GitHub issue: FreeRTOS/FreeRTOS-Kernel#1217 - Forum thread: freertos.org/t/22287 Signed-off-by: Gaurav Aggarwal --- portable/ThirdParty/GCC/Posix/port.c | 67 +++++++++++++------ .../GCC/Posix/utils/wait_for_event.c | 39 +++++++++-- 2 files changed, 80 insertions(+), 26 deletions(-) diff --git a/portable/ThirdParty/GCC/Posix/port.c b/portable/ThirdParty/GCC/Posix/port.c index c4eacb2ba..4f7d8b609 100644 --- a/portable/ThirdParty/GCC/Posix/port.c +++ b/portable/ThirdParty/GCC/Posix/port.c @@ -119,14 +119,20 @@ static void prvResumeThread( Thread_t * xThreadId ); static void vPortSystemTickHandler( int sig ); static void vPortStartFirstTask( void ); static void prvPortYieldFromISR( void ); +static void prvThreadKeyDestructor( void * pvData ); +static void prvInitThreadKey( void ); +static void prvMarkAsFreeRTOSThread( void ); +static BaseType_t prvIsFreeRTOSThread( void ); +static void prvDestroyThreadKey( void ); /*-----------------------------------------------------------*/ -void prvThreadKeyDestructor( void * data ) +static void prvThreadKeyDestructor( void * pvData ) { - free( data ); + free( pvData ); } +/*-----------------------------------------------------------*/ -static void prvInitThreadKey() +static void prvInitThreadKey( void ) { pthread_mutex_lock( &xThreadMutex ); @@ -137,24 +143,39 @@ static void prvInitThreadKey() pthread_mutex_unlock( &xThreadMutex ); } +/*-----------------------------------------------------------*/ -static void prvMarkAsFreeRTOSThread( pthread_t thread ) +static void prvMarkAsFreeRTOSThread( void ) { + uint8_t * pucThreadData = NULL; + prvInitThreadKey(); - uint8_t * thread_data = malloc( 1 ); - configASSERT( thread_data != NULL ); - *thread_data = 1; - pthread_setspecific( xThreadKey, thread_data ); -} -static BaseType_t prvIsFreeRTOSThread( pthread_t thread ) + pucThreadData = malloc( 1 ); + configASSERT( pucThreadData != NULL ); + + *pucThreadData = 1; + + pthread_setspecific( xThreadKey, pucThreadData ); +} +/*-----------------------------------------------------------*/ + +static BaseType_t prvIsFreeRTOSThread( void ) { - uint8_t * thread_data = ( uint8_t * ) pthread_getspecific( xThreadKey ); + uint8_t * pucThreadData = NULL; + BaseType_t xRet = pdFALSE; - return thread_data != NULL && *thread_data == 1; + pucThreadData = ( uint8_t * ) pthread_getspecific( xThreadKey ); + if( ( pucThreadData != NULL ) && ( *pucThreadData == 1 ) ) + { + xRet = pdTRUE; + } + + return xRet; } +/*-----------------------------------------------------------*/ -static void prvDestroyThreadKey() +static void prvDestroyThreadKey( void ) { pthread_key_delete( xThreadKey ); } @@ -309,7 +330,7 @@ void vPortEndScheduler( void ) ( void ) pthread_kill( hMainThread, SIG_RESUME ); /* Waiting to be deleted here. */ - if( prvIsFreeRTOSThread( pthread_self() ) == pdTRUE ) + if( prvIsFreeRTOSThread() == pdTRUE ) { pxCurrentThread = prvGetThreadFromTask( xTaskGetCurrentTaskHandle() ); event_wait( pxCurrentThread->ev ); @@ -369,7 +390,7 @@ void vPortYield( void ) void vPortDisableInterrupts( void ) { - if( prvIsFreeRTOSThread( pthread_self() ) == pdTRUE ) + if( prvIsFreeRTOSThread() == pdTRUE ) { pthread_sigmask(SIG_BLOCK, &xAllSignals, NULL); } @@ -378,9 +399,9 @@ void vPortDisableInterrupts( void ) void vPortEnableInterrupts( void ) { - if( prvIsFreeRTOSThread( pthread_self() ) == pdTRUE ) + if( prvIsFreeRTOSThread() == pdTRUE ) { - pthread_sigmask(SIG_UNBLOCK, &xAllSignals, NULL); + pthread_sigmask( SIG_UNBLOCK, &xAllSignals, NULL ); } } /*-----------------------------------------------------------*/ @@ -417,9 +438,9 @@ static void * prvTimerTickHandler( void * arg ) { ( void ) arg; - prvMarkAsFreeRTOSThread( pthread_self() ); + prvMarkAsFreeRTOSThread(); - prvPortSetCurrentThreadName("Scheduler timer"); + prvPortSetCurrentThreadName( "Scheduler timer" ); while( xTimerTickThreadShouldRun ) { @@ -451,7 +472,7 @@ void prvSetupTimerInterrupt( void ) static void vPortSystemTickHandler( int sig ) { - if( prvIsFreeRTOSThread( pthread_self() ) == pdTRUE ) + if( prvIsFreeRTOSThread() == pdTRUE ) { Thread_t * pxThreadToSuspend; Thread_t * pxThreadToResume; @@ -473,7 +494,9 @@ static void vPortSystemTickHandler( int sig ) } uxCriticalNesting--; - } else { + } + else + { fprintf( stderr, "vPortSystemTickHandler called from non-FreeRTOS thread\n" ); } } @@ -508,7 +531,7 @@ static void * prvWaitForStart( void * pvParams ) { Thread_t * pxThread = pvParams; - prvMarkAsFreeRTOSThread( pthread_self() ); + prvMarkAsFreeRTOSThread(); prvSuspendSelf( pxThread ); diff --git a/portable/ThirdParty/GCC/Posix/utils/wait_for_event.c b/portable/ThirdParty/GCC/Posix/utils/wait_for_event.c index bf744e27f..55fd7bbfc 100644 --- a/portable/ThirdParty/GCC/Posix/utils/wait_for_event.c +++ b/portable/ThirdParty/GCC/Posix/utils/wait_for_event.c @@ -35,9 +35,11 @@ struct event { pthread_mutex_t mutex; + pthread_mutexattr_t mutexattr; pthread_cond_t cond; bool event_triggered; }; +/*-----------------------------------------------------------*/ struct event * event_create( void ) { @@ -46,23 +48,36 @@ struct event * event_create( void ) if( ev != NULL ) { ev->event_triggered = false; - pthread_mutex_init( &ev->mutex, NULL ); + pthread_mutexattr_init( &ev->mutexattr ); + #ifndef __APPLE__ + pthread_mutexattr_setrobust( &ev->mutexattr, PTHREAD_MUTEX_ROBUST ); + #endif + pthread_mutex_init( &ev->mutex, &ev->mutexattr ); pthread_cond_init( &ev->cond, NULL ); } return ev; } +/*-----------------------------------------------------------*/ void event_delete( struct event * ev ) { pthread_mutex_destroy( &ev->mutex ); + pthread_mutexattr_destroy( &ev->mutexattr ); pthread_cond_destroy( &ev->cond ); free( ev ); } +/*-----------------------------------------------------------*/ bool event_wait( struct event * ev ) { - pthread_mutex_lock( &ev->mutex ); + if( pthread_mutex_lock( &ev->mutex ) == EOWNERDEAD ) + { + #ifndef __APPLE__ + /* If the thread owning the mutex died, make the mutex consistent. */ + pthread_mutex_consistent( &ev->mutex ); + #endif + } while( ev->event_triggered == false ) { @@ -73,6 +88,8 @@ bool event_wait( struct event * ev ) pthread_mutex_unlock( &ev->mutex ); return true; } +/*-----------------------------------------------------------*/ + bool event_wait_timed( struct event * ev, time_t ms ) { @@ -82,7 +99,13 @@ bool event_wait_timed( struct event * ev, clock_gettime( CLOCK_REALTIME, &ts ); ts.tv_sec += ms / 1000; ts.tv_nsec += ( ( ms % 1000 ) * 1000000 ); - pthread_mutex_lock( &ev->mutex ); + if( pthread_mutex_lock( &ev->mutex ) == EOWNERDEAD ) + { + #ifndef __APPLE__ + /* If the thread owning the mutex died, make the mutex consistent. */ + pthread_mutex_consistent( &ev->mutex ); + #endif + } while( ( ev->event_triggered == false ) && ( ret == 0 ) ) { @@ -98,11 +121,19 @@ bool event_wait_timed( struct event * ev, pthread_mutex_unlock( &ev->mutex ); return true; } +/*-----------------------------------------------------------*/ void event_signal( struct event * ev ) { - pthread_mutex_lock( &ev->mutex ); + if( pthread_mutex_lock( &ev->mutex ) == EOWNERDEAD ) + { + #ifndef __APPLE__ + /* If the thread owning the mutex died, make the mutex consistent. */ + pthread_mutex_consistent( &ev->mutex ); + #endif + } ev->event_triggered = true; pthread_cond_signal( &ev->cond ); pthread_mutex_unlock( &ev->mutex ); } +/*-----------------------------------------------------------*/