From f756e762bae323b64e629c266a8ef5719766023c Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Wed, 19 May 2021 15:32:41 -0400 Subject: [PATCH] Fix #1027, defer cancellation when BSP locked Resolves two related issues: - OS_TaskGetId does not return a valid value for tasks where cancellation is pending, but they are still running. This in turn is likely to trigger other (bogus) debug checks which invoke OS_DEBUG and in turn do console writes. - The console write itself is a cancellation point, which is now done while holding a BSP mutex. If canceled here, then the mutex is not released. Solution is in two parts: - OS_TaskGetId should return the task ID it knows about, regardless of whether the task is pending cancellation or not. - Defer cancellation of the task while the BSP is locked, ensure it reaches the unlock, then restore the previous cancel state. --- src/bsp/generic-linux/src/bsp_start.c | 14 ++++++++++++++ .../generic-linux/src/generic_linux_bsp_internal.h | 1 + src/os/shared/src/osapi-task.c | 10 +--------- .../shared/src/coveragetest-task.c | 4 ---- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/bsp/generic-linux/src/bsp_start.c b/src/bsp/generic-linux/src/bsp_start.c index 4900f2259..67e21158d 100644 --- a/src/bsp/generic-linux/src/bsp_start.c +++ b/src/bsp/generic-linux/src/bsp_start.c @@ -110,6 +110,15 @@ void OS_BSP_Lock_Impl(void) { BSP_DEBUG("pthread_mutex_lock: %s\n", strerror(status)); } + else + { + /* + * Temporarily Disable/Defer thread cancellation. + * Note that OS_BSP_ConsoleOutput_Impl() calls write() which is a cancellation point. + * So if this calling task is canceled, it risks leaving the BSP locked. + */ + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &OS_BSP_GenericLinuxGlobal.AccessCancelState); + } } /*---------------------------------------------------------------- @@ -125,6 +134,11 @@ void OS_BSP_Unlock_Impl(void) { BSP_DEBUG("pthread_mutex_unlock: %s\n", strerror(status)); } + else + { + /* Restore previous cancelability state */ + pthread_setcancelstate(OS_BSP_GenericLinuxGlobal.AccessCancelState, NULL); + } } /* --------------------------------------------------------- diff --git a/src/bsp/generic-linux/src/generic_linux_bsp_internal.h b/src/bsp/generic-linux/src/generic_linux_bsp_internal.h index d61a1e478..4c51c5122 100644 --- a/src/bsp/generic-linux/src/generic_linux_bsp_internal.h +++ b/src/bsp/generic-linux/src/generic_linux_bsp_internal.h @@ -42,6 +42,7 @@ typedef struct { bool EnableTermControl; /**< Will be set "true" when invoked from a TTY device, false otherwise */ pthread_mutex_t AccessMutex; + int AccessCancelState; } OS_BSP_GenericLinuxGlobalData_t; /* diff --git a/src/os/shared/src/osapi-task.c b/src/os/shared/src/osapi-task.c index bbf817aae..9124b1349 100644 --- a/src/os/shared/src/osapi-task.c +++ b/src/os/shared/src/osapi-task.c @@ -339,18 +339,10 @@ int32 OS_TaskSetPriority(osal_id_t task_id, osal_priority_t new_priority) *-----------------------------------------------------------------*/ osal_id_t OS_TaskGetId(void) { - OS_object_token_t token; - osal_id_t task_id; + osal_id_t task_id; task_id = OS_TaskGetId_Impl(); - /* Confirm the task master table entry matches the expected. - * If not it means we have some stale/leftover value */ - if (OS_ObjectIdGetById(OS_LOCK_MODE_NONE, LOCAL_OBJID_TYPE, task_id, &token) != OS_SUCCESS) - { - task_id = OS_OBJECT_ID_UNDEFINED; - } - return (task_id); } /* end OS_TaskGetId */ diff --git a/src/unit-test-coverage/shared/src/coveragetest-task.c b/src/unit-test-coverage/shared/src/coveragetest-task.c index 1da6564aa..58f51904f 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-task.c +++ b/src/unit-test-coverage/shared/src/coveragetest-task.c @@ -197,10 +197,6 @@ void Test_OS_TaskGetId(void) UT_SetDefaultReturnValue(UT_KEY(OS_TaskGetId_Impl), idbuf.val); objid = OS_TaskGetId(); OSAPI_TEST_OBJID(objid, ==, idbuf.id); - - UT_SetDefaultReturnValue(UT_KEY(OS_ObjectIdGetById), OS_ERROR); - objid = OS_TaskGetId(); - OSAPI_TEST_OBJID(objid, ==, OS_OBJECT_ID_UNDEFINED); } void Test_OS_TaskGetIdByName(void)