possible overflow check in xTaskCheckForTimeOut

system · November 20, 2017, 2:33am

chuan-jiang wrote on Monday, November 20, 2017:

Hi,
I’ve noticed a possible overflow issue in xTaskCheckForTimeOut function. Could you please help to confirm it?

In the latest RTOSv9.0.1, task.c, line3154,
const TickType_t xElapsedTime = xConstTickCount - pxTimeOut->xTimeOnEntering;
Noticed that TickType_t might be defined as unsigned type, if xConstTickCount is less than pxTimeOut->xTimeOnEntering, the substraction will lead to an overflow.

and then in line3187,
*else if( xElapsedTime < pxTicksToWait )

I think this check condition should also take xNumOfOverflows into account.
(1) if xNumOfOverflows equals pxTimeOut->xOverflowCount+1, timeout might not be reached.
(2) but if xNumOfOverflows is larger than pxTimeOut->xOverflowCount+1, it might be the wrapped around, and the timeout has been passed away when this function is invoked. Then the return value should not be FALSE!

Also, xNumOfOverflows might also be wrapped around when come to xTaskCheckForTimeOut again.

Looking forwarding to your help and reply.
Thanks a lot

rtel · November 20, 2017, 4:27am

rtel wrote on Monday, November 20, 2017:

Not looked at this in detail as just on my mobile. Will look tomorrow. In the mean time if you can provide some values for the various variables that you think highlight an issue that would help greatly.

system · November 20, 2017, 5:51am

chuan-jiang wrote on Monday, November 20, 2017:

Hi,
Thanks a lot for your reply. I’d like to paste the relevent code pieces below for your convenience.
BaseType_t xTaskCheckForTimeOut( TimeOut_t * const pxTimeOut, TickType_t * const pxTicksToWait )
{
BaseType_t xReturn;
…
const TickType_t xConstTickCount = xTickCount;
const TickType_t xElapsedTime = xConstTickCount - pxTimeOut->xTimeOnEntering;
…
if( ( xNumOfOverflows != pxTimeOut->xOverflowCount ) && ( xConstTickCount >= pxTimeOut->xTimeOnEntering ) )
{
/* branch 1 */
xReturn = pdTRUE;
}
else if( xElapsedTime < pxTicksToWait )
{
/ branch 2 /
/ Not a genuine timeout. Adjust parameters for time remaining. */
pxTicksToWait -= xElapsedTime;
vTaskInternalSetTimeOutState( pxTimeOut );
xReturn = pdFALSE;
}
else
{
/ branch 3 */
xReturn = pdTRUE;
}
…
return xReturn;
}

port macro setting:
TickType_t set to uint32
suppose xTicksToWait interval is 5, i.e. *pxTicksToWait = 5

situation a:
time1: pxTimeOut->xOverflowCount=1, xNumOfOverflows=1, pxTimeOut->xTimeOnEntering=0xFFFFFFFFUL, xTickCount=0xFFFFFFFFUL
2 ticks elapsed, and xTaskCheckForTimeOut invoked
time2: pxTimeOut->xOverflowCount=1, xNumOfOverflows=2, pxTimeOut->xTimeOnEntering=0xFFFFFFFFUL, xTickCount=1
then, it’ll run into branch2, the return is FALSE. the actual elapsed ticks is 2, larger than 5.
Correct!

situation b:
time1: pxTimeOut->xOverflowCount=1, xNumOfOverflows=1, pxTimeOut->xTimeOnEntering=0xFFFFFFFFUL, xTickCount=0xFFFFFFFFUL
0xFFFFFFFFUL + 2 ticks elapsed, and xTaskCheckForTimeOut invoked
time2: pxTimeOut->xOverflowCount=1, xNumOfOverflows=3, pxTimeOut->xTimeOnEntering=0xFFFFFFFFUL, xTickCount=1
then, it’ll still run into branch2, the return is FALSE. However, the actual elapsed ticks is 0xFFFFFFFF+2=4294967297, definitly larger than 5.
Wrong!

The thing will come even worse when xNumOfOverflows overflows either. However, it might be reached rather seldomly.

Thanks

rtel · November 20, 2017, 6:52pm

rtel wrote on Monday, November 20, 2017:

I want to test your scenario b, but I don’t think it is a valid case
that I can generate. Below is the test code I was setting up. Note at
the bottom, where your scenario describes xNumOfOverflows to be 3, it is
actually 2, as adding two to the tick count only increments it once. Or
am I misunderstanding something:

void Testing( void )
{
TimeOut_t xTimeout;

   /* Set up initial condition as per situation b. */
   xTickCount = 0xffffffffUL;
   xNumOfOverflows = 1;

   /* Call vTaskSetTimeOutState() to latch the initial condition into the
   TimeOut_t structure. */
   vTaskSetTimeOutState( &xTimeout );

   /* Check the vTaskSetTimeOutState() set its internal members to the
   situation b starting condition. */
   configASSERT( xTimeout.xOverflowCount == 1 );
   configASSERT( xTimeout.xTimeOnEntering == 0xffffffffUL );

   /* Now increment the tick by two. */
   xTickCount += 2UL;

   /* That has caused an overflow, so, if the scheduler had been running,
   xNumOfOverflows will also have been incremented. */
   xNumOfOverflows++;


   /* NB - at this point scenario b says xNumOfOverflows would be 3, but
   it would actually be 2, so I'm not sure how to test from here. */
}

system · November 21, 2017, 2:17am

chuan-jiang wrote on Tuesday, November 21, 2017:

Hi,
Thanks a lot for your reply.
Actually in scenario b, I meant overflow occurs twice. As I wrote above, “0xFFFFFFFFUL + 2 ticks elapsed, and xTaskCheckForTimeOut invoked”, an extra 0xFFFFFFFFUL is for the overflow one more time. So in your test code, it might be like:

   xTickCount += 2UL;
   xNumOfOverflows += 2UL;

It’ll simulate the scenario b.

After reviewing the whole RTOS files, I think although this API is unsafe, such scenario might not occur in the code.
The reason is now this API is only used for QUEUE send & received which are an endless loop, between the vTaskInternalSetTimeOutState and xTaskCheckForTimeOut which updates the xTickCount and xNumOfOverflows, the ticks elipsed will not be so long as 0xFFFFFFFFUL.
The code pieces from Queue.c is:

                    vTaskInternalSetTimeOutState( &xTimeOut );
					xEntryTimeSet = pdTRUE;
				}
				else
				{
					/* Entry time was already set. */
					mtCOVERAGE_TEST_MARKER();
				}
			}
		}
		taskEXIT_CRITICAL();

		/* Interrupts and other tasks can send to and receive from the queue
		now the critical section has been exited. */

		vTaskSuspendAll();
		prvLockQueue( pxQueue );

		/* Update the timeout state to see if it has expired yet. */
		if( xTaskCheckForTimeOut( &xTimeOut, &xTicksToWait ) == pdFALSE )
		{

That might be the point. Could you please help to check if I’m right?
Thanks a lot.

rtel · November 21, 2017, 3:14am

rtel wrote on Tuesday, November 21, 2017:

A task can either block forever, in which case the code in your email
doesn’t apply because it actually ends up on the suspended task list
(even though it is in the blocked state), or it can block for a finite
time, in which case the longest time it can block is the maximum block
time that can be specified given the TickType_t data type. That means,
it can never block for more than one overflow because that is longer
than the longest time it can possible ask for - so I don’t think it is
actually possible for the scenario you describe to actually occur.

system · November 21, 2017, 11:12am

chuan-jiang wrote on Tuesday, November 21, 2017:

Hi,
Got it.
Thanks a lot for your reply.