Refine heap_5 heap protector

[Saiiijchan]

heap_5 is used for multiple separated memory spaces. In the previous implementation, it only verifies the highest and lowest addresses. A pointer may not be within heap regions, but is still located between the highest and lowest addressed.

Description
heap_5 is used for multiple separated memory spaces. In the previous implementation, it only verifies the highest and lowest addresses. A pointer may not be within heap regions, but is still located between the highest and lowest addressed.

This PR will use a buffer to record the boundary of each heap region so the pointer detection can be made more accurate . This buffter is dynamically allocated at the end of vPortDefineHeapRegions(). At this time, the multi heap regions have been defined and there is only a free block and "pxFakeEnd" (the heap structure points to the next heap region) and pxEnd (At the last heap region).

As shown in the diagram above, the white area represents the valid pointer range.

Test Steps

Test on FreeRTO and runs correctly.
I have also manually corrupted a free list block and triggered assertion successfully.

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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.

[aggarg]

Thank you for sharing your proposal! I think it is a lot of complexity to bring to catch one specific type of pointer corruption. This also requires handling another corner case when pvPortMalloc for Region_Bound_t fails.

What do you think about letting the application to override heapVALIDATE_BLOCK_POINTER:

    #ifndef configVALIDATE_HEAP_BLOCK_POINTER
        #define heapVALIDATE_BLOCK_POINTER( pxBlock )                       \
        configASSERT( ( pucHeapHighAddress != NULL ) &&                     \
                    ( pucHeapLowAddress != NULL ) &&                        \
                    ( ( uint8_t * ) ( pxBlock ) >= pucHeapLowAddress ) &&   \
                    ( ( uint8_t * ) ( pxBlock ) < pucHeapHighAddress ) )
    #else
        #define heapVALIDATE_BLOCK_POINTER( pxBlock )                       \
            configVALIDATE_HEAP_BLOCK_POINTER( pxBlock )
    #endif

This way the interested application can implement configVALIDATE_HEAP_BLOCK_POINTER. Since the application already knows the number of memory regions it is using for heap, it does not need to do any dynamic allocation.

[Saiiijchan]

@aggarg Thanks for your review.

This way the interested application can implement configVALIDATE_HEAP_BLOCK_POINTER. Since the application already knows the number of memory regions it is using for heap, it does not need to do any dynamic allocation .

I agree with your point. When implementing this PR, the uncertainty about the number of memory regions is the reason why dynamic allocation of Region_Bound_t is needed in vPortDefinesRegion() after the heap is initialized, as at that point only free blocks are available.

configVALIDATE_HEAP_BLOCK_POINTER is a good idea. In cases where the number of memory regions is known, static allocation can reduce complexity. However, in this case, the user will need to manually calculate the highest and lowest aligned addresses for each memory region, which is something to be aware of.

This also requires handling another corner case when pvPortMalloc for Region_Bound_t fails.

I'm a bit confused about this point. pvPortMalloc() for Region_Bound_t is the first allocation in the system, so if it fails, doesn't that mean the first pvPortMalloc() will fail? I can't quite understand under what circumstances it would fail.

[aggarg]

In cases where the number of memory regions is known, static allocation can reduce complexity. However, in this case, the user will need to manually calculate the highest and lowest aligned addresses for each memory region, which is something to be aware of.

But those regions are known to the application, isn't it? One possible implementation is -

#define HEAP_1_SIZE ( 24 * 1024 )
#define HEAP_2_SIZE ( 48 * 1024 )

uint8_t ucHeap1[ HEAP_1_SIZE ] __attribute__( ( section( ".freertos_heap1" ) ) );
uint8_t ucHeap2[ HEAP_2_SIZE ] __attribute__( ( section( ".freertos_heap2" ) ) );


void AppInitializeHeap( void )
{
    HeapRegion_t xHeapRegions[] =
    {
        { ( unsigned char * ) ucHeap1, sizeof( ucHeap1 ) },
        { ( unsigned char * ) ucHeap2, sizeof( ucHeap2 ) },
        { NULL,                        0                 }
    };

    vPortDefineHeapRegions( xHeapRegions );
}

And in this case the application can implement the check like this -

#define configVALIDATE_HEAP_BLOCK_POINTER( pxBlock ) ValidateHeapPointer( pxBlock )

void ValidateHeapPointer( void * ptr )
{
    if( !( ptr >= &( ucHeap1[ 0 ] ) ) && ( ptr < &( ucHeap1[ HEAP_1_SIZE ] ) ) &&
        !( ptr >= &( ucHeap2[ 0 ] ) ) && ( ptr < &( ucHeap2[ HEAP_2_SIZE ] ) ) )
    {
        /* If pointer is not in ucHeap1 and ucHeap2, it is invalid. */
        configASSERT( pdFALSE );
    }
}

I'm a bit confused about this point. pvPortMalloc() for Region_Bound_t is the first allocation in the system, so if it fails, doesn't that mean the first pvPortMalloc() will fail? I can't quite understand under what circumstances it would fail.

You are right that if this fails, likely the first cal to pvPortMalloc will fail too. We'll still want an assert or if check to avoid NULL pointer de-reference. It is not needed if we go with the configVALIDATE_HEAP_BLOCK_POINTER solution.

[Saiiijchan]

Thank you for your reply, I agree with your point of view.

[aggarg]

Thank you! Would you please update your PR?

[Saiiijchan]

Of course, then I will modify the PR according to this suggestion.

    #ifndef configVALIDATE_HEAP_BLOCK_POINTER
        #define heapVALIDATE_BLOCK_POINTER( pxBlock )                       \
        configASSERT( ( pucHeapHighAddress != NULL ) &&                     \
                    ( pucHeapLowAddress != NULL ) &&                        \
                    ( ( uint8_t * ) ( pxBlock ) >= pucHeapLowAddress ) &&   \
                    ( ( uint8_t * ) ( pxBlock ) < pucHeapHighAddress ) )
    #else
        #define heapVALIDATE_BLOCK_POINTER( pxBlock )                       \
            configVALIDATE_HEAP_BLOCK_POINTER( pxBlock )
    #endif

[Saiiijchan]

I updated the PR and force pushed. Thanks for your suggestion!

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[aggarg]

@Saiiijchan Thank you for your contribution!