From a29627f59d9c88ffdce9d6762078b90c84f9b025 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 5 Oct 2022 12:00:21 +0200 Subject: [PATCH 1/3] heap: fix the size of buffers in ignored tests the ignored tests are updated with the minimum required buffer size to create a new heap using the tlsf heap callocator. --- components/heap/test_apps/main/CMakeLists.txt | 2 +- .../{test_app_main.c => test_heap_main.c} | 2 +- .../test_apps/main/test_runtime_heap_reg.c | 27 ++++++++++--------- components/heap/test_apps/pytest_heap.py | 4 +-- 4 files changed, 18 insertions(+), 17 deletions(-) rename components/heap/test_apps/main/{test_app_main.c => test_heap_main.c} (96%) diff --git a/components/heap/test_apps/main/CMakeLists.txt b/components/heap/test_apps/main/CMakeLists.txt index 25d6596721..d823d12c98 100644 --- a/components/heap/test_apps/main/CMakeLists.txt +++ b/components/heap/test_apps/main/CMakeLists.txt @@ -1,4 +1,4 @@ -set(src_test "test_app_main.c" +set(src_test "test_heap_main.c" "test_aligned_alloc_caps.c" "test_allocator_timings.c" "test_corruption_check.c" diff --git a/components/heap/test_apps/main/test_app_main.c b/components/heap/test_apps/main/test_heap_main.c similarity index 96% rename from components/heap/test_apps/main/test_app_main.c rename to components/heap/test_apps/main/test_heap_main.c index fdae5be5fb..c5f5dce532 100644 --- a/components/heap/test_apps/main/test_app_main.c +++ b/components/heap/test_apps/main/test_heap_main.c @@ -8,7 +8,7 @@ #include "unity_test_runner.h" #include "esp_heap_caps.h" -#define TEST_MEMORY_LEAK_THRESHOLD (-1024) +#define TEST_MEMORY_LEAK_THRESHOLD (-3000) static size_t before_free_8bit; static size_t before_free_32bit; diff --git a/components/heap/test_apps/main/test_runtime_heap_reg.c b/components/heap/test_apps/main/test_runtime_heap_reg.c index c2729708f4..b1bc832a0a 100644 --- a/components/heap/test_apps/main/test_runtime_heap_reg.c +++ b/components/heap/test_apps/main/test_runtime_heap_reg.c @@ -15,13 +15,17 @@ #include "esp_system.h" #include "heap_memory_layout.h" +#include "../tlsf/tlsf.h" /* NOTE: This is not a well-formed unit test, it leaks memory */ TEST_CASE("Allocate new heap at runtime", "[heap][ignore]") { - const size_t BUF_SZ = 1000; - const size_t HEAP_OVERHEAD_MAX = 200; + // 60 bytes of overhead in multi_heap + size of control_t from tlsf + const size_t HEAP_OVERHEAD_MAX = tlsf_size() + 60; + const size_t MIN_HEAP_SIZE = HEAP_OVERHEAD_MAX + tlsf_block_size_min(); + const size_t BUF_SZ = MIN_HEAP_SIZE; void *buffer = malloc(BUF_SZ); + TEST_ASSERT_NOT_NULL(buffer); uint32_t before_free = esp_get_free_heap_size(); TEST_ESP_OK( heap_caps_add_region((intptr_t)buffer, (intptr_t)buffer + BUF_SZ) ); @@ -36,12 +40,12 @@ TEST_CASE("Allocate new heap at runtime", "[heap][ignore]") */ TEST_CASE("Allocate new heap with new capability", "[heap][ignore]") { - const size_t BUF_SZ = 100; -#ifdef CONFIG_ESP_SYSTEM_MEMPROT_FEATURE - const size_t ALLOC_SZ = 32; -#else - const size_t ALLOC_SZ = 64; // More than half of BUF_SZ -#endif + // 60 bytes of multi_heap structures overhead + size of control_t from tlsf + const size_t HEAP_OVERHEAD = tlsf_size() + 60; + const size_t MIN_HEAP_SIZE = HEAP_OVERHEAD + tlsf_block_size_min(); + const size_t BUF_SZ = MIN_HEAP_SIZE; + const size_t ALLOC_SZ = tlsf_block_size_min(); + const uint32_t MALLOC_CAP_INVENTED = (1 << 30); /* this must be unused in esp_heap_caps.h */ /* no memory exists to provide this capability */ @@ -62,8 +66,8 @@ TEST_CASE("Allocate new heap with new capability", "[heap][ignore]") TEST_CASE("Add .bss memory to heap region runtime", "[heap][ignore]") { -#define BUF_SZ 1000 -#define HEAP_OVERHEAD_MAX 200 +#define HEAP_OVERHEAD_MAX 3248 +#define BUF_SZ 3260 static uint8_t s_buffer[BUF_SZ]; printf("s_buffer start %08x end %08x\n", (intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ); @@ -73,9 +77,6 @@ TEST_CASE("Add .bss memory to heap region runtime", "[heap][ignore]") printf("Before %"PRIu32" after %"PRIu32"\n", before_free, after_free); /* allow for some 'heap overhead' from accounting structures */ TEST_ASSERT(after_free >= before_free + BUF_SZ - HEAP_OVERHEAD_MAX); - - /* Twice add must be failed */ - TEST_ASSERT( (heap_caps_add_region((intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ) != ESP_OK) ); } extern esp_err_t heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, intptr_t start, intptr_t end); diff --git a/components/heap/test_apps/pytest_heap.py b/components/heap/test_apps/pytest_heap.py index 8c43daa550..377e595c43 100644 --- a/components/heap/test_apps/pytest_heap.py +++ b/components/heap/test_apps/pytest_heap.py @@ -17,7 +17,7 @@ from pytest_embedded import Dut ) def test_heap_poisoning(dut: Dut) -> None: dut.expect_exact('Press ENTER to see the list of tests') - dut.write('![ignore]') + dut.write('*') dut.expect_unity_test_output(timeout=300) @@ -34,7 +34,7 @@ def test_heap_poisoning(dut: Dut) -> None: ) def test_heap(dut: Dut) -> None: dut.expect_exact('Press ENTER to see the list of tests') - dut.write('![ignore]') + dut.write('*') dut.expect_unity_test_output(timeout=300) From 2cce5e98b1760880e4cbd0d9dcd4f9c96e909f0f Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 5 Oct 2022 15:03:05 +0200 Subject: [PATCH 2/3] heap: add dynamic poisoning threshold in pytest env to allow test with known memory leak to pass --- .../heap/test_apps/main/test_heap_main.c | 11 +++++++-- .../test_apps/main/test_runtime_heap_reg.c | 24 +++++++++++++------ 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/components/heap/test_apps/main/test_heap_main.c b/components/heap/test_apps/main/test_heap_main.c index c5f5dce532..c271df754f 100644 --- a/components/heap/test_apps/main/test_heap_main.c +++ b/components/heap/test_apps/main/test_heap_main.c @@ -8,7 +8,12 @@ #include "unity_test_runner.h" #include "esp_heap_caps.h" -#define TEST_MEMORY_LEAK_THRESHOLD (-3000) +#define TEST_MEMORY_LEAK_THRESHOLD_DEFAULT -100 +static int leak_threshold = TEST_MEMORY_LEAK_THRESHOLD_DEFAULT; +void set_leak_threshold(int threshold) +{ + leak_threshold = threshold; +} static size_t before_free_8bit; static size_t before_free_32bit; @@ -17,7 +22,7 @@ static void check_leak(size_t before_free, size_t after_free, const char *type) { ssize_t delta = after_free - before_free; printf("MALLOC_CAP_%s: Before %u bytes free, After %u bytes free (delta %d)\n", type, before_free, after_free, delta); - TEST_ASSERT_MESSAGE(delta >= TEST_MEMORY_LEAK_THRESHOLD, "memory leak"); + TEST_ASSERT_MESSAGE(delta >= leak_threshold, "memory leak"); } void setUp(void) @@ -32,6 +37,8 @@ void tearDown(void) size_t after_free_32bit = heap_caps_get_free_size(MALLOC_CAP_32BIT); check_leak(before_free_8bit, after_free_8bit, "8BIT"); check_leak(before_free_32bit, after_free_32bit, "32BIT"); + + leak_threshold = TEST_MEMORY_LEAK_THRESHOLD_DEFAULT; } void app_main(void) diff --git a/components/heap/test_apps/main/test_runtime_heap_reg.c b/components/heap/test_apps/main/test_runtime_heap_reg.c index b1bc832a0a..2dd56a1f93 100644 --- a/components/heap/test_apps/main/test_runtime_heap_reg.c +++ b/components/heap/test_apps/main/test_runtime_heap_reg.c @@ -17,11 +17,14 @@ #include "../tlsf/tlsf.h" +extern void set_leak_threshold(int threshold); + /* NOTE: This is not a well-formed unit test, it leaks memory */ -TEST_CASE("Allocate new heap at runtime", "[heap][ignore]") +TEST_CASE("Allocate new heap at runtime", "[heap]") { - // 60 bytes of overhead in multi_heap + size of control_t from tlsf - const size_t HEAP_OVERHEAD_MAX = tlsf_size() + 60; + // 84 bytes of overhead to account for multi_heap structs and eventual + // poisoning bytes + size of control_t from tlsf + const size_t HEAP_OVERHEAD_MAX = tlsf_size() + 84; const size_t MIN_HEAP_SIZE = HEAP_OVERHEAD_MAX + tlsf_block_size_min(); const size_t BUF_SZ = MIN_HEAP_SIZE; void *buffer = malloc(BUF_SZ); @@ -33,15 +36,19 @@ TEST_CASE("Allocate new heap at runtime", "[heap][ignore]") printf("Before %"PRIu32" after %"PRIu32"\n", before_free, after_free); /* allow for some 'heap overhead' from accounting structures */ TEST_ASSERT(after_free >= before_free + BUF_SZ - HEAP_OVERHEAD_MAX); + + // set the leak threshold to a bigger value as this test leaks memory + set_leak_threshold(-3000); } /* NOTE: This is not a well-formed unit test, it leaks memory and may fail if run twice in a row without a reset. */ -TEST_CASE("Allocate new heap with new capability", "[heap][ignore]") +TEST_CASE("Allocate new heap with new capability", "[heap]") { - // 60 bytes of multi_heap structures overhead + size of control_t from tlsf - const size_t HEAP_OVERHEAD = tlsf_size() + 60; + // 84 bytes of overhead to account for multi_heap structs and eventual + // poisoning bytes + size of control_t from tlsf + const size_t HEAP_OVERHEAD = tlsf_size() + 84; const size_t MIN_HEAP_SIZE = HEAP_OVERHEAD + tlsf_block_size_min(); const size_t BUF_SZ = MIN_HEAP_SIZE; const size_t ALLOC_SZ = tlsf_block_size_min(); @@ -58,13 +65,16 @@ TEST_CASE("Allocate new heap with new capability", "[heap][ignore]") /* ta-da, it's now possible! */ TEST_ASSERT_NOT_NULL( heap_caps_malloc(ALLOC_SZ, MALLOC_CAP_INVENTED) ); + + // set the leak threshold to a bigger value as this test leaks memory + set_leak_threshold(-3000); } /* NOTE: This is not a well-formed unit test. * If run twice without a reset, it will failed. */ -TEST_CASE("Add .bss memory to heap region runtime", "[heap][ignore]") +TEST_CASE("Add .bss memory to heap region runtime", "[heap]") { #define HEAP_OVERHEAD_MAX 3248 #define BUF_SZ 3260 From 2b5a844640816971d3d0d4f73b2f110d967aed8c Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 5 Oct 2022 15:05:08 +0200 Subject: [PATCH 3/3] heap: fix the boundary checks when adding a new region --- components/heap/heap_caps_init.c | 4 ++-- components/heap/test_apps/main/test_runtime_heap_reg.c | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c index 75556dd155..5a7beb3a7c 100644 --- a/components/heap/heap_caps_init.c +++ b/components/heap/heap_caps_init.c @@ -189,14 +189,14 @@ bool heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, * 3.add region (s3>=s && e3= heap_start && end < heap_end; * |--------------| correct * - * 4.add region (s4e) |------------------------| wrong: bool condition_4 = start < heap_end && end > heap_end; + * 4.add region (s4=e) |------------------------| wrong: bool condition_4 = start < heap_end && end >= heap_end; * |---------------------| wrong * * 5.add region (s5>=e) |----| correct: bool condition_5 = start >= heap_end; */ bool condition_2 = start < heap_start && end > heap_start; // if true then region not allowed - bool condition_4 = start < heap_end && end > heap_end; // if true then region not allowed + bool condition_4 = start < heap_end && end >= heap_end; // if true then region not allowed return (condition_2 || condition_4) ? false: true; } diff --git a/components/heap/test_apps/main/test_runtime_heap_reg.c b/components/heap/test_apps/main/test_runtime_heap_reg.c index 2dd56a1f93..8e54973ede 100644 --- a/components/heap/test_apps/main/test_runtime_heap_reg.c +++ b/components/heap/test_apps/main/test_runtime_heap_reg.c @@ -87,6 +87,9 @@ TEST_CASE("Add .bss memory to heap region runtime", "[heap]") printf("Before %"PRIu32" after %"PRIu32"\n", before_free, after_free); /* allow for some 'heap overhead' from accounting structures */ TEST_ASSERT(after_free >= before_free + BUF_SZ - HEAP_OVERHEAD_MAX); + + /* Twice add must be failed */ + TEST_ASSERT( (heap_caps_add_region((intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ) != ESP_OK) ); } extern esp_err_t heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, intptr_t start, intptr_t end); @@ -98,10 +101,10 @@ TEST_CASE("Add heap region address range checks", "[heap]") TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x0, 0x1000)); TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x1000, 0x2000)); - TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x1000, 0x3000)); TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x3000, 0x4000)); TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x0, 0x2000)); TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x0, 0x4000)); TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x1000, 0x4000)); TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x2000, 0x4000)); + TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x1000, 0x3000)); }