Repository navigation
Check queue set pointer size before loading data - #1527
kstribrnAmzn wants to merge 2 commits into
Conversation
Verifies the supplied queue set reference has the correct item size to receive an item from the underlying queue. We thank Claude and Ada Logics for their help identifying this issue.
| traceENTER_xQueueSelectFromSet( xQueueSet, xTicksToWait ); | ||
|
|
||
| ( void ) xQueueReceive( ( QueueHandle_t ) xQueueSet, &xReturn, xTicksToWait ); | ||
| if( ( ( Queue_t * ) xQueueSet )->uxItemSize == ( UBaseType_t ) sizeof( Queue_t * ) ) |
There was a problem hiding this comment.
This dereferences xQueueSet before xQueueReceive() reaches its configASSERT( pxQueue ), so a NULL handle no longer trips the assert. It now faults on this read. On Cortex-M parts without an MPU, address 0 is readable flash, so the read usually succeeds and the function silently returns NULL with no assert at all.
Suggest adding configASSERT( xQueueSet ); before the check, here and in xQueueSelectFromSetFromISR() (line 3335). MPU v2 isn't affected because the wrapper never passes NULL, but non-MPU and MPU v1 builds are.
There was a problem hiding this comment.
I'll add the assertions
| if( ( ( Queue_t * ) xQueueSet )->uxItemSize == ( UBaseType_t ) sizeof( Queue_t * ) ) | ||
| { | ||
| ( void ) xQueueReceive( ( QueueHandle_t ) xQueueSet, &xReturn, xTicksToWait ); | ||
| } |
There was a problem hiding this comment.
When the check fails, this returns NULL right away and ignores xTicksToWait. The documented pattern is for(;;) { m = xQueueSelectFromSet( set, portMAX_DELAY ); ... }, and with a bad handle that turns into a busy loop that starves lower-priority tasks and idle, with nothing pointing at the cause. That's still better than the stack overflow it replaces, but the failure is now completely silent.
Could we make it visible? For example, configASSERT( ( ( Queue_t * ) xQueueSet )->uxItemSize == ( UBaseType_t ) sizeof( Queue_t * ) ) ahead of the runtime check, and/or an else { mtCOVERAGE_TEST_MARKER(); } branch to match the style elsewhere in the file.
There was a problem hiding this comment.
The documented pattern is for(;;) { m = xQueueSelectFromSet( set, portMAX_DELAY ); ... }, and with a bad handle that turns into a busy loop that starves lower-priority tasks and idle, with nothing pointing at the cause.
This would be a fairly obvious issue during development. I do not think we need to add further assertions to aid in visibility during development. This is also easy to avoid without assertions: If you expect this sort of behavior then make the task waiting on the queue set the lowest priority.
|
|
Trusted firmware website is down (429). The link should be valid still. |
Description
Verifies the supplied queue set reference has the
correct item size to receive an item from the
underlying queue.
We thank Claude and Ada Logics for their help identifying this issue.
Test Steps
Verified with CMock unit tests. Related change -
Checklist:
Related Issue
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.