Bug Report: Memory pool corruption in _lwmem_alloc_align_internal

[chrissolomon]

Hello,

I have just tracked down a tricky bug in lwmem.c.

When an aligned allocation is made, the following process occurs:

1) Find a free block

2) Figure out how large a shift is required to make the allocation aligned

3) If free block isn't big enough, go back to 1

4) Create a new free block at the beginning of the free block (size is the previously determined shift).

5) Set the allocated block pointer to the location for the new allocated block

     (Sets up the free block's 'nextblock' field to point to the new allocated block, & sets the free block's size)

6a) If there is enough extra space in the allocated block, split it, creating another free block.

     (The next block is set up - size, pool, nextblock, and the allocated block has it's size set)

6b) If there isn't enough extra space in the allocated block, the allocated block *should* use the remaining space in the block.

     BUT... here is the problem. The size of the newly allocated block is not set, and is undefined.

     This causes undefined behavior (usually watchdog resets) when a task's resources are freed (for example), as the allocations in the pool cannot be 'walked'

This bug is present in MQX versions 4.0.1 and 4.1.0 (possibly others).

Here is my fix (based on the MQX 4.1.0 code) - starts in lwmem.c at line 475

prev_block_ptr->BLOCKSIZE = shift;
block_ptr->U.NEXTBLOCK = prev_block_ptr->U.NEXTBLOCK;


next_block_size = block_size - requested_size - shift;
if (next_block_size >= LWMEM_MIN_MEMORY_STORAGE_SIZE)
{
    /*
     * The current block is big enough to split.
     * into 2 blocks.... the part to be allocated is one block,
     * and the rest remains as a free block on the free list.
     */
    next_block_ptr = (LWMEM_BLOCK_STRUCT_PTR) (void *) ((unsigned char *) block_ptr + requested_size);
    /* Initialize the new physical block values */
    next_block_ptr->BLOCKSIZE = next_block_size;
    /* Link new block into the free list */
    next_block_ptr->POOL = (void *) mem_pool_ptr;
    next_block_ptr->U.NEXTBLOCK = block_ptr->U.NEXTBLOCK;
    block_ptr->U.NEXTBLOCK = (void *) next_block_ptr;
}
else
{
     /* Take the entire block */
     requested_size = block_size - shift;
     next_block_ptr = block_ptr->U.NEXTBLOCK;
} /* Endif */

block_ptr->BLOCKSIZE = requested_size;

This should have a relatively small impact - aligned allocation is used in only a couple of places.

[martinbachem]

Hi there,

I'm suffering from quite similar problems, so I stumbled upon this bug report.

My system crashes when _task_destroy_internal finds a memory corruption as in MQX_CORRUPT_STORAGE_POOL_FREE_LIST

I'm using Cortex-M with MQX v4.2.0.2, so after reviewing lwmem.c I think the issue reported above is already fixed in MQX version 4.2.

Anyway, when using MQX_USE_MEM, my problems are gone.

So I'm re-reviewing lwsem.c... The Bug mentioned above seems to be alloc_aligned specific, so I started to check who is using aligned memory. There seems to be a certain need for aligned memory when allocating a dynamic task's stack memory, at least when following this code remark in task.c :

#if 0 /* we dont need this, because we using _mem_alloc_align function in _task_alloc_td_internal */
#if PSP_MEMORY_ALIGNMENT
 /* But we need to add size to allow for alignment of stack base */
 stack_size += PSP_STACK_ALIGNMENT + 1;
#endif
#endif‍‍‍‍‍‍

Alas, _task_alloc_td_internal is not using _mem_alloc_align for stack memory allocation.

Right now I'm testing if using _mem_alloc_align in _task_alloc_td_internal can help me out, or if I have to stick to MQX_USE_MEM instead of lwmem.

Maybe I need something like this to work well with lwmem ?

// new_td_ptr->STACK_ALLOC_BLOCK = (TD_STRUCT_PTR)_mem_alloc(stack_size);
new_td_ptr->STACK_ALLOC_BLOCK = (TD_STRUCT_PTR)_mem_alloc_align(stack_size, 4);‍‍

regards,

  Martin

[RadekS]

Yes, you are right.

This is potential issue with lwmem allocator. It is known issue and we currently working on that. We plan some more complex rework of lwmem which will include also fix of this potential issue. We would like to focus on solution for avoiding this situation instead of specific workaround.

Anyway, thank you for your bug report and offered solution.

[RadekS]

I would like to confirm, that (after triple checking) this bug isn’t presented on current developing version and next MQX release will fix it directly.

Yes, we can confirm that your fix is correct for lwmem.c in MQX version up to 4.1.0 (include 4.1.0.1 patch).

Line “block_ptr->BLOCKSIZE = requested_size;” has to be executed in both cases (if (next_block_size >= LWMEM_MIN_MEMORY_STORAGE_SIZE)).

So, you can move this line behind if command or copy this line also to else branch:

if (next_block_size >= LWMEM_MIN_MEMORY_STORAGE_SIZE)

{

….

}

else

{

requested_size = block_size - shift; 

next_block_ptr = block_ptr->U.NEXTBLOCK;

} /* Endif */

block_ptr->BLOCKSIZE = requested_size;

or

if (next_block_size >= LWMEM_MIN_MEMORY_STORAGE_SIZE)

{

….

block_ptr->BLOCKSIZE = requested_size;

}

else

{

requested_size = block_size - shift; 

next_block_ptr = block_ptr->U.NEXTBLOCK;

block_ptr->BLOCKSIZE = requested_size;

} /* Endif */

Have a great day,
RadekS

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------

[chrissolomon]

Thanks for checking on that.

By the way, the line

requested_size = block_size - shift;

when the whole block is used is needed as well, or the blocksize will still be set incorrectly.

Thanks

Chris

[RadekS]

Yes, you are right. I agree.

I edited my last reply according your note.