Skip to content

Commit 2e0c623

Browse files
authored
Fix race in prvProcessSimulatedInterrupts (#1055)
Earlier the code was suspending the current thread after calling vTaskSwitchContext. This left a gap where the current thread could access incorrect pxCurrentTCB after it was changed by vTaskSwitchContext. This commit addresses the problem by suspending the current thread before calling vTaskSwitchContext. It was reported here - #1054. Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
1 parent 29b202a commit 2e0c623

File tree

1 file changed

+21
-38
lines changed

1 file changed

+21
-38
lines changed

portable/MSVC-MingW/port.c

Lines changed: 21 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -435,48 +435,31 @@ static void prvProcessSimulatedInterrupts( void )
435435

436436
if( ulSwitchRequired != pdFALSE )
437437
{
438-
void * pvOldCurrentTCB;
439-
440-
pvOldCurrentTCB = pxCurrentTCB;
438+
/* Suspend the old thread. */
439+
pxThreadState = ( ThreadState_t * ) *( ( size_t * ) pxCurrentTCB );
440+
SuspendThread( pxThreadState->pvThread );
441+
442+
/* Ensure the thread is actually suspended by performing a
443+
* synchronous operation that can only complete when the thread
444+
* is actually suspended. The below code asks for dummy register
445+
* data. Experimentation shows that these two lines don't appear
446+
* to do anything now, but according to
447+
* https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743
448+
* they do - so as they do not harm (slight run-time hit). */
449+
xContext.ContextFlags = CONTEXT_INTEGER;
450+
( void ) GetThreadContext( pxThreadState->pvThread, &xContext );
441451

442452
/* Select the next task to run. */
443453
vTaskSwitchContext();
444454

445-
/* If the task selected to enter the running state is not the task
446-
* that is already in the running state. */
447-
if( pvOldCurrentTCB != pxCurrentTCB )
448-
{
449-
/* Suspend the old thread. In the cases where the (simulated)
450-
* interrupt is asynchronous (tick event swapping a task out rather
451-
* than a task blocking or yielding) it doesn't matter if the
452-
* 'suspend' operation doesn't take effect immediately - if it
453-
* doesn't it would just be like the interrupt occurring slightly
454-
* later. In cases where the yield was caused by a task blocking
455-
* or yielding then the task will block on a yield event after the
456-
* yield operation in case the 'suspend' operation doesn't take
457-
* effect immediately. */
458-
pxThreadState = ( ThreadState_t * ) *( ( size_t * ) pvOldCurrentTCB );
459-
SuspendThread( pxThreadState->pvThread );
460-
461-
/* Ensure the thread is actually suspended by performing a
462-
* synchronous operation that can only complete when the thread is
463-
* actually suspended. The below code asks for dummy register
464-
* data. Experimentation shows that these two lines don't appear
465-
* to do anything now, but according to
466-
* https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743
467-
* they do - so as they do not harm (slight run-time hit). */
468-
xContext.ContextFlags = CONTEXT_INTEGER;
469-
( void ) GetThreadContext( pxThreadState->pvThread, &xContext );
470-
471-
/* Obtain the state of the task now selected to enter the
472-
* Running state. */
473-
pxThreadState = ( ThreadState_t * ) ( *( size_t * ) pxCurrentTCB );
474-
475-
/* pxThreadState->pvThread can be NULL if the task deleted
476-
* itself - but a deleted task should never be resumed here. */
477-
configASSERT( pxThreadState->pvThread != NULL );
478-
ResumeThread( pxThreadState->pvThread );
479-
}
455+
/* Obtain the state of the task now selected to enter the
456+
* Running state. */
457+
pxThreadState = ( ThreadState_t * ) ( *( size_t * ) pxCurrentTCB );
458+
459+
/* pxThreadState->pvThread can be NULL if the task deleted
460+
* itself - but a deleted task should never be resumed here. */
461+
configASSERT( pxThreadState->pvThread != NULL );
462+
ResumeThread( pxThreadState->pvThread );
480463
}
481464

482465
/* If the thread that is about to be resumed stopped running

0 commit comments

Comments
 (0)