From de8ce0e43eca6da6ddc0f0d40f6062ada2cf46c8 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Sat, 15 Oct 2016 21:53:12 -0500 Subject: [PATCH 1/4] CFSTORE - Fix crashed due to uninit data When the config store is powered down area_0_head is freed, but area_0_len is not set to 0. This causes when cfstore_realloc_ex is called, since on the first allocation it appears that the config store size is decreasing, and therefore the data is not initialized. Since the data is uninitiated various fields such as the reference can have invalid values. On GCC_ARM built with heap stats enabled this manifests as a crash due to an invalid reference count. This patch fixes this problem by setting area_0_len to 0 when the data is freed. --- .../storage/FEATURE_STORAGE/cfstore/source/configuration_store.c | 1 + 1 file changed, 1 insertion(+) diff --git a/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c b/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c index 34150c48ae..5d71576452 100644 --- a/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c +++ b/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c @@ -4045,6 +4045,7 @@ static int32_t cfstore_uninitialise(void) CFSTORE_FREE(ctx->area_0_head); ctx->area_0_head = NULL; ctx->area_0_tail = NULL; + ctx->area_0_len = 0; } } out: From c908666d63dd92bf3bebbb36ad5721891444f5e5 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Sun, 16 Oct 2016 18:39:31 -0500 Subject: [PATCH 2/4] CFSTORE - fix handling of realloc fail on delete The function cfstore_delete_ex is written under the assumption that CFSTORE_REALLOC will never fail if the size is decreasing. Regardless of the status of CFSTORE_REALLOC the entry is removed from the config store and zeroed. This works correctly if CFSTORE_REALLOC correctly updates area_0_tail, but can lead to crashes in the case area_0_tail is left unchanged. The crash is because when iterating over the config store data, cfstore_get_next_hkvt is unable to determine the end of valid data. This patch fixes this problem by handling the realloc failure case by updating area_0_tail even if CFSTORE_REALLOC returns NULL. This patch also adds an assert to check for out of bound entries in when calling cfstore_get_next_hkvt. This allows an assert to be triggered if this bug is re-introduced, rather than a crash. --- .../FEATURE_STORAGE/cfstore/source/configuration_store.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c b/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c index 5d71576452..d631f4c6fe 100644 --- a/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c +++ b/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c @@ -1294,6 +1294,7 @@ static int32_t cfstore_get_next_hkvt(cfstore_area_hkvt_t* prev, cfstore_area_hkv CFSTORE_ASSERT(prev != NULL); CFSTORE_ASSERT(next != NULL); + CFSTORE_ASSERT(prev->tail <= ctx->area_0_tail); if(prev->tail == ctx->area_0_tail){ CFSTORE_TP(CFSTORE_TP_VERBOSE1, "%s:reached the end of the list. return NULL entry\n", __func__); @@ -1433,6 +1434,14 @@ static int32_t cfstore_realloc_ex(ARM_CFSTORE_SIZE size, uint64_t *allocated_siz } ptr = (uint8_t*) CFSTORE_REALLOC((void*) ctx->area_0_head, size); + if (ptr == NULL) { + if (total_kv_size <= ctx->area_0_len) { + /* Size is shrinking so a realloc failure is recoverable. + * Update ptr so it matches the previous head. + */ + ptr = ctx->area_0_head; + } + } if(ptr == NULL){ CFSTORE_ERRLOG("%s:Error: unable to allocate memory (size=%d)\n", __func__, (int) size); /* realloc() has failed to allocate the required memory object. If previously From 9f0e756e283a46e9a50e788950609738ebecbe8e Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Sun, 16 Oct 2016 20:19:49 -0500 Subject: [PATCH 3/4] CFSTORE - Delete handle even if key ref non zero When closing a file handle remove the handle from the handle list regardless of what the reference count of the key it is pointing to is. This prevents config store from keeping a handle to file handles that have gone out of scope. --- .../FEATURE_STORAGE/cfstore/source/configuration_store.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c b/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c index d631f4c6fe..edeb5d7123 100644 --- a/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c +++ b/features/storage/FEATURE_STORAGE/cfstore/source/configuration_store.c @@ -2458,11 +2458,11 @@ static int32_t cfstore_file_destroy(cfstore_file_t* file) if(cfstore_hkvt_get_flags_delete(&hkvt)){ ret = cfstore_delete_ex(&hkvt); } - /* reset client buffer to empty ready for reuse */ - /* delete the file even if not deleting the KV*/ - cfstore_listDel(&file->node); - memset(file, 0, sizeof(cfstore_file_t)); } + /* reset client buffer to empty ready for reuse */ + /* delete the file even if not deleting the KV*/ + cfstore_listDel(&file->node); + memset(file, 0, sizeof(cfstore_file_t)); } return ret; } From 3601b5ebb3a4ae5430e94466bfb79a0144a0c522 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Sun, 16 Oct 2016 20:46:10 -0500 Subject: [PATCH 4/4] CFSTORE - Fix test failures due to fragmentation In the config store create test in test case #5 the amount of available memory is determined by fully allocating the heap. This is done multiple times to determine if there is a memory leak. This causes problems when even slight fragmentation occurs in the heap, since the size that can be allocated is decreased slightly, which the test flags as a memory leak. This patch makes memory leak detection more robust by using metrics provided by mbed_stats_heap_get. These metrics are an exact measurement of memory allocated is not changed by fragmentation. This allows the memory leak test to report correct values regardless of fragmentation. --- .../TESTS/cfstore/create/create.cpp | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/features/storage/FEATURE_STORAGE/TESTS/cfstore/create/create.cpp b/features/storage/FEATURE_STORAGE/TESTS/cfstore/create/create.cpp index 7090ac7ae1..d84f448240 100644 --- a/features/storage/FEATURE_STORAGE/TESTS/cfstore/create/create.cpp +++ b/features/storage/FEATURE_STORAGE/TESTS/cfstore/create/create.cpp @@ -25,6 +25,7 @@ */ #include "mbed.h" +#include "mbed_stats.h" #include "cfstore_config.h" #include "cfstore_debug.h" #include "cfstore_test.h" @@ -507,11 +508,10 @@ control_t cfstore_create_test_04_end(const size_t call_count) * * Create enough KV's to consume the whole of available memory */ -int32_t cfstore_create_test_05_core(const size_t call_count, uint32_t* bytes_stored_ex) +int32_t cfstore_create_test_05_core(const size_t call_count) { int32_t ret = ARM_DRIVER_ERROR; uint32_t i = 0; - uint32_t bytes_stored = 0; const uint32_t max_num_kvs_create = 200; const size_t kv_name_tag_len = 3; const size_t kv_name_min_len = 10; @@ -535,9 +535,6 @@ int32_t cfstore_create_test_05_core(const size_t call_count, uint32_t* bytes_sto memset(value_buf, 0, max_value_buf_size); snprintf(kv_name_tag_buf, kv_name_tag_len+1, "%0d", (int) i); ret = cfstore_create_kv_create(kv_name_min_len, kv_name_tag_buf, value_buf, kv_value_min_len/64 * (i+1)); - bytes_stored += kv_name_min_len + i + strlen(kv_name_tag_buf); /* kv_name */ - bytes_stored += kv_value_min_len/64 * (i+1); /* kv value blob */ - bytes_stored += 8; /* kv overhead */ if(ret == ARM_CFSTORE_DRIVER_ERROR_OUT_OF_MEMORY){ CFSTORE_ERRLOG("Out of memory on %d-th KV, trying to allocate memory totalling %d.\n", (int) i, (int) bytes_stored); break; @@ -551,9 +548,6 @@ int32_t cfstore_create_test_05_core(const size_t call_count, uint32_t* bytes_sto free(value_buf); CFSTORE_TEST_UTEST_MESSAGE(cfstore_create_utest_msg_g, CFSTORE_UTEST_MSG_BUF_SIZE, "%s:Error: Uninitialize() call failed.\n", __func__); TEST_ASSERT_MESSAGE(drv->Uninitialize() >= ARM_DRIVER_OK, cfstore_create_utest_msg_g); - if(bytes_stored_ex){ - *bytes_stored_ex = bytes_stored; - } return ret; } @@ -576,22 +570,25 @@ control_t cfstore_create_test_05(const size_t call_count) { uint32_t i = 0; int32_t ret = ARM_DRIVER_ERROR; - uint32_t bytes_stored = 0; - uint32_t bytes_stored_prev = 0; const uint32_t max_loops = 50; + mbed_stats_heap_t stats_before; + mbed_stats_heap_t stats_after; + + mbed_stats_heap_get(&stats_before); CFSTORE_FENTRYLOG("%s:entered\n", __func__); for(i = 0; i < max_loops; i++) { - ret = cfstore_create_test_05_core(call_count, &bytes_stored); + ret = cfstore_create_test_05_core(call_count); CFSTORE_TEST_UTEST_MESSAGE(cfstore_create_utest_msg_g, CFSTORE_UTEST_MSG_BUF_SIZE, "%s:Error: cfstore_create_test_05_core() failed (ret = %d.\n", __func__, (int) ret); TEST_ASSERT_MESSAGE(ret >= ARM_DRIVER_OK, cfstore_create_utest_msg_g); + mbed_stats_heap_get(&stats_after); if(i > 1) { - CFSTORE_TEST_UTEST_MESSAGE(cfstore_create_utest_msg_g, CFSTORE_UTEST_MSG_BUF_SIZE, "%s:Error: memory leak: stored %d bytes on loop %d, but %d bytes on loop %d .\n", __func__, (int) bytes_stored, (int) i, (int) bytes_stored_prev, (int) i-1); - TEST_ASSERT_MESSAGE(bytes_stored == bytes_stored_prev, cfstore_create_utest_msg_g); - + CFSTORE_TEST_UTEST_MESSAGE(cfstore_create_utest_msg_g, CFSTORE_UTEST_MSG_BUF_SIZE, "%s:Error: memory leak: stored %d bytes on loop %d, but %d bytes on loop %d .\n", __func__, (int) stats_after.current_size, (int) i, (int) stats_before.current_size, (int) i-1); + TEST_ASSERT_MESSAGE(stats_after.current_size == stats_before.current_size, cfstore_create_utest_msg_g); + TEST_ASSERT(stats_after.alloc_fail_cnt > stats_before.alloc_fail_cnt); } - bytes_stored_prev = bytes_stored; + stats_before = stats_after; } return CaseNext; } @@ -828,7 +825,9 @@ Case cases[] = { Case("CREATE_test_03_end", cfstore_create_test_03_end), Case("CREATE_test_04_start", cfstore_utest_default_start), Case("CREATE_test_04_end", cfstore_create_test_04_end), +#if defined(MBED_HEAP_STATS_ENABLED) && MBED_HEAP_STATS_ENABLED && !defined(__ICCARM__) Case("CREATE_test_05", cfstore_create_test_05), +#endif Case("CREATE_test_06_start", cfstore_utest_default_start), Case("CREATE_test_06_end", cfstore_create_test_06_end), Case("CREATE_test_07_start", cfstore_utest_default_start),