From d6d02c67d2bcacaa02499930aabd76365797b108 Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Mon, 28 Sep 2020 22:34:02 +0200 Subject: [PATCH 1/3] Fix inconsistent supervisor heap. When allocations were freed in a different order from the reverse of how they were allocated (leaving holes), the heap would get into an inconsistent state, eventually resulting in crashes. free_memory() relies on having allocations in order, but allocate_memory() did not guarantee that: It reused the first allocation with a NULL ptr without ensuring that it was between low_address and high_address. When it belongs to a hole in the allocated memory, such an allocation is not really free for reuse, because free_memory() still needs its length. Instead, explicitly mark allocations available for reuse with a special (invalid) value in the length field. Only allocations that lie between low_address and high_address are marked that way. --- supervisor/shared/memory.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/supervisor/shared/memory.c b/supervisor/shared/memory.c index 8ae8a16997..56c617b9cf 100755 --- a/supervisor/shared/memory.c +++ b/supervisor/shared/memory.c @@ -33,6 +33,10 @@ #define CIRCUITPY_SUPERVISOR_ALLOC_COUNT (12) +// Using a zero length to mark an unused allocation makes the code a bit shorter (but makes it +// impossible to support zero-length allocations). +#define FREE 0 + static supervisor_allocation allocations[CIRCUITPY_SUPERVISOR_ALLOC_COUNT]; // We use uint32_t* to ensure word (4 byte) alignment. uint32_t* low_address; @@ -61,19 +65,23 @@ void free_memory(supervisor_allocation* allocation) { } if (allocation->ptr == high_address) { high_address += allocation->length / 4; + allocation->length = FREE; for (index++; index < CIRCUITPY_SUPERVISOR_ALLOC_COUNT; index++) { if (allocations[index].ptr != NULL) { break; } high_address += allocations[index].length / 4; + allocations[index].length = FREE; } } else if (allocation->ptr + allocation->length / 4 == low_address) { low_address = allocation->ptr; + allocation->length = FREE; for (index--; index >= 0; index--) { if (allocations[index].ptr != NULL) { break; } low_address -= allocations[index].length / 4; + allocations[index].length = FREE; } } else { // Freed memory isn't in the middle so skip updating bounds. The memory will be added to the @@ -99,7 +107,7 @@ supervisor_allocation* allocate_remaining_memory(void) { } supervisor_allocation* allocate_memory(uint32_t length, bool high) { - if ((high_address - low_address) * 4 < (int32_t) length || length % 4 != 0) { + if (length == 0 || (high_address - low_address) * 4 < (int32_t) length || length % 4 != 0) { return NULL; } uint8_t index = 0; @@ -109,7 +117,7 @@ supervisor_allocation* allocate_memory(uint32_t length, bool high) { direction = -1; } for (; index < CIRCUITPY_SUPERVISOR_ALLOC_COUNT; index += direction) { - if (allocations[index].ptr == NULL) { + if (allocations[index].length == FREE) { break; } } From 5bdb8c45dd2700941ab75661b773680bdd46aaf1 Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Tue, 29 Sep 2020 22:21:29 +0200 Subject: [PATCH 2/3] Allow allocate_memory() to reuse holes when matching exactly. This requires recovering the pointer of the allocation, which could be done by adding up neighbor lengths, but the simpler way is to stop NULLing it out in the first place and instead mark an allocation as freed by the client by setting the lowest bit of the length (which is always zero in a valid length). --- supervisor/shared/memory.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/supervisor/shared/memory.c b/supervisor/shared/memory.c index 56c617b9cf..fdb4e06fbb 100755 --- a/supervisor/shared/memory.c +++ b/supervisor/shared/memory.c @@ -37,6 +37,10 @@ // impossible to support zero-length allocations). #define FREE 0 +// The lowest two bits of a valid length are always zero, so we can use them to mark an allocation +// as freed by the client but not yet reclaimed into the FREE middle. +#define HOLE 1 + static supervisor_allocation allocations[CIRCUITPY_SUPERVISOR_ALLOC_COUNT]; // We use uint32_t* to ensure word (4 byte) alignment. uint32_t* low_address; @@ -67,9 +71,10 @@ void free_memory(supervisor_allocation* allocation) { high_address += allocation->length / 4; allocation->length = FREE; for (index++; index < CIRCUITPY_SUPERVISOR_ALLOC_COUNT; index++) { - if (allocations[index].ptr != NULL) { + if (!(allocations[index].length & HOLE)) { break; } + // Division automatically shifts out the HOLE bit. high_address += allocations[index].length / 4; allocations[index].length = FREE; } @@ -77,7 +82,7 @@ void free_memory(supervisor_allocation* allocation) { low_address = allocation->ptr; allocation->length = FREE; for (index--; index >= 0; index--) { - if (allocations[index].ptr != NULL) { + if (!(allocations[index].length & HOLE)) { break; } low_address -= allocations[index].length / 4; @@ -85,9 +90,10 @@ void free_memory(supervisor_allocation* allocation) { } } else { // Freed memory isn't in the middle so skip updating bounds. The memory will be added to the - // middle when the memory to the inside is freed. + // middle when the memory to the inside is freed. We still need its length, but setting + // only the lowest bit is nondestructive. + allocation->length |= HOLE; } - allocation->ptr = NULL; } supervisor_allocation* allocation_from_ptr(void *ptr) { @@ -107,7 +113,7 @@ supervisor_allocation* allocate_remaining_memory(void) { } supervisor_allocation* allocate_memory(uint32_t length, bool high) { - if (length == 0 || (high_address - low_address) * 4 < (int32_t) length || length % 4 != 0) { + if (length == 0 || length % 4 != 0) { return NULL; } uint8_t index = 0; @@ -116,15 +122,21 @@ supervisor_allocation* allocate_memory(uint32_t length, bool high) { index = CIRCUITPY_SUPERVISOR_ALLOC_COUNT - 1; direction = -1; } + supervisor_allocation* alloc; for (; index < CIRCUITPY_SUPERVISOR_ALLOC_COUNT; index += direction) { - if (allocations[index].length == FREE) { + alloc = &allocations[index]; + if (alloc->length == FREE) { break; } + // If a hole matches in length exactly, we can reuse it. + if (alloc->length == (length | HOLE)) { + alloc->length = length; + return alloc; + } } - if (index >= CIRCUITPY_SUPERVISOR_ALLOC_COUNT) { + if (index >= CIRCUITPY_SUPERVISOR_ALLOC_COUNT || (high_address - low_address) * 4 < (int32_t) length) { return NULL; } - supervisor_allocation* alloc = &allocations[index]; if (high) { high_address -= length / 4; alloc->ptr = high_address; From be8092f4d32d06488df88294a9dc4bb809411c5e Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Fri, 2 Oct 2020 23:07:07 +0200 Subject: [PATCH 3/3] When there is not enough free space, but a matching hole on the other side, use it. --- supervisor/shared/memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/supervisor/shared/memory.c b/supervisor/shared/memory.c index fdb4e06fbb..0f96ae2734 100755 --- a/supervisor/shared/memory.c +++ b/supervisor/shared/memory.c @@ -125,7 +125,7 @@ supervisor_allocation* allocate_memory(uint32_t length, bool high) { supervisor_allocation* alloc; for (; index < CIRCUITPY_SUPERVISOR_ALLOC_COUNT; index += direction) { alloc = &allocations[index]; - if (alloc->length == FREE) { + if (alloc->length == FREE && (high_address - low_address) * 4 >= (int32_t) length) { break; } // If a hole matches in length exactly, we can reuse it. @@ -134,7 +134,7 @@ supervisor_allocation* allocate_memory(uint32_t length, bool high) { return alloc; } } - if (index >= CIRCUITPY_SUPERVISOR_ALLOC_COUNT || (high_address - low_address) * 4 < (int32_t) length) { + if (index >= CIRCUITPY_SUPERVISOR_ALLOC_COUNT) { return NULL; } if (high) {