Skip to content

Commit

Permalink
Fix #1027, defer cancellation when BSP locked
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jphickey committed May 19, 2021
1 parent 9756b03 commit 8a3e0ec
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 13 deletions.
14 changes: 14 additions & 0 deletions src/bsp/generic-linux/src/bsp_start.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

/*----------------------------------------------------------------
Expand All @@ -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);
}
}

/* ---------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions src/bsp/generic-linux/src/generic_linux_bsp_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/*
Expand Down
10 changes: 1 addition & 9 deletions src/os/shared/src/osapi-task.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand Down
4 changes: 0 additions & 4 deletions src/unit-test-coverage/shared/src/coveragetest-task.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 8a3e0ec

Please sign in to comment.