From 8f77de645367a27291e351005a586313fce1d25f Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Wed, 20 Nov 2019 17:02:00 +0200 Subject: [PATCH 1/4] TDBStore safety check: Erase if there is valid keys on the free space. In case our are contains data from previous reset() or reset_area(), we might end up in the situation where free space contains valid key headers, but we have not erased that area yet. This can cause failures if the deinit() and init() because new scan of that area would continue as long as keys are found. This causes keys on the not-yet-erased area to be included in the new instance of TDBStore. To prevent this failure, check after each key-write that our free space does not contain valid key headers. Also make sure that we erase one program unit sector over the master record. If we erased just the master record,first key might is still there, causing next init() to find it. Extend erase area by one program unit, so that build_ram_table() won't find any keys. --- features/storage/kvstore/tdbstore/TDBStore.cpp | 17 ++++++++++++++++- features/storage/kvstore/tdbstore/TDBStore.h | 4 ++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/features/storage/kvstore/tdbstore/TDBStore.cpp b/features/storage/kvstore/tdbstore/TDBStore.cpp index 01c9a942bb..7de5371a50 100644 --- a/features/storage/kvstore/tdbstore/TDBStore.cpp +++ b/features/storage/kvstore/tdbstore/TDBStore.cpp @@ -148,6 +148,10 @@ TDBStore::~TDBStore() int TDBStore::read_area(uint8_t area, uint32_t offset, uint32_t size, void *buf) { + //Check that we are not crossing area boundary + if (offset + size > _size) { + return MBED_ERROR_READ_FAILED; + } int os_ret = _buff_bd->read(buf, _area_params[area].address + offset, size); if (os_ret) { @@ -649,6 +653,15 @@ int TDBStore::set_finalize(set_handle_t handle) _free_space_offset = align_up(ih->bd_curr_offset, _prog_size); + // Safety check: If there seems to be valid keys on the free space + // we should erase one sector more, just to ensure that in case of power failure + // next init() would not extend the scan phase to that section as well. + os_ret = read_record(_active_area, _free_space_offset, 0, 0, 0, actual_data_size, 0, + false, false, false, false, hash, flags, next_offset); + if (os_ret == MBED_SUCCESS) { + check_erase_before_write(_active_area, _free_space_offset, sizeof(record_header_t)); + } + end: if ((need_gc) && (ih->bd_base_offset != _master_record_offset)) { garbage_collection(); @@ -972,6 +985,7 @@ int TDBStore::increment_max_keys(void **ram_table) // Reallocate ram table with new size ram_table_entry_t *old_ram_table = (ram_table_entry_t *) _ram_table; ram_table_entry_t *new_ram_table = new ram_table_entry_t[_max_keys + 1]; + memset(new_ram_table, 0, sizeof(ram_table_entry_t)*(_max_keys + 1)); // Copy old content to new table memcpy(new_ram_table, old_ram_table, sizeof(ram_table_entry_t) * _max_keys); @@ -1018,6 +1032,7 @@ int TDBStore::init() _max_keys = initial_max_keys; ram_table = new ram_table_entry_t[_max_keys]; + memset(ram_table, 0, sizeof(ram_table_entry_t) * _max_keys); _ram_table = ram_table; _num_keys = 0; @@ -1211,7 +1226,7 @@ int TDBStore::reset() _num_keys = 0; _free_space_offset = _master_record_offset; _active_area_version = 1; - + memset(_ram_table, 0, sizeof(ram_table_entry_t) * _max_keys); // Write an initial master record on active area ret = write_master_record(_active_area, _active_area_version, _free_space_offset); diff --git a/features/storage/kvstore/tdbstore/TDBStore.h b/features/storage/kvstore/tdbstore/TDBStore.h index a31dd0b501..ad7d3f9779 100644 --- a/features/storage/kvstore/tdbstore/TDBStore.h +++ b/features/storage/kvstore/tdbstore/TDBStore.h @@ -365,8 +365,8 @@ private: * * @param[in] area Area. * @param[in] offset Offset of record in area. - * @param[in] key Key - must not include '*' '/' '?' ':' ';' '\' '"' '|' ' ' '<' '>' '\'. - * @param[in] data_buf Data buffer. + * @param[out] key Key - must not include '*' '/' '?' ':' ';' '\' '"' '|' ' ' '<' '>' '\'. + * @param[out] data_buf Data buffer. * @param[in] data_buf_size Data buffer size. * @param[out] actual_data_size Actual data size. * @param[in] data_offset Offset in data. From 065ff2645e655b8d5968b84c6fe23205145d35a3 Mon Sep 17 00:00:00 2001 From: Veijo Pesonen Date: Tue, 26 Nov 2019 16:36:22 +0200 Subject: [PATCH 2/4] Fixes features-storage-tests-kvstore-static_tests test case bugs At least with LPC55S69's default TDBStore configuration it's impossible to run storage Greentea tests without exhausting the memory reserved for storing keys. Fixes an issue where number of keys were removed based on number of threads which didn't have anything to do with the test case. Fixes an issue where number of keys were assumed to be constant but variable number was used for configuration. --- .../storage/TESTS/kvstore/static_tests/main.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/features/storage/TESTS/kvstore/static_tests/main.cpp b/features/storage/TESTS/kvstore/static_tests/main.cpp index b3bfce8346..673e679c49 100644 --- a/features/storage/TESTS/kvstore/static_tests/main.cpp +++ b/features/storage/TESTS/kvstore/static_tests/main.cpp @@ -34,7 +34,7 @@ static const size_t data_size = 5; static size_t actual_size = 0; static const size_t buffer_size = 20; static const int num_of_threads = 3; -static const char num_of_keys = 3; +static const char num_of_keys = 11; #if defined(MBED_CONF_RTOS_PRESENT) /* Forked 3 threads plus misc, so minimum (4 * OS_STACK_SIZE) heap are required. */ static const int heap_alloc_threshold_size = 4 * OS_STACK_SIZE; @@ -42,7 +42,9 @@ static const int heap_alloc_threshold_size = 4 * OS_STACK_SIZE; /* Bare metal does not require memory for threads, so use just minimum for test */ static const int heap_alloc_threshold_size = MBED_CONF_TARGET_BOOT_STACK_SIZE; #endif -static const char *keys[] = {"key1", "key2", "key3"}; +static const char *keys[num_of_keys] = { "key1", "key2", "key3", "key4", "key5", "key6", "key7", "key8", "key9", + "key10", "key11" + }; static int init_res = MBED_ERROR_NOT_READY; @@ -298,14 +300,14 @@ static void set_several_key_value_sizes() name[6] = 0; - for (i = 0; i < 26; i++) { + for (i = 0; i < num_of_keys; i++) { c = i + 'a'; name[5] = c; res = kv_set(name, name, sizeof(name), 0); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); } - for (i = 0; i < 26; i++) { + for (i = 0; i < num_of_keys; i++) { c = i + 'a'; name[5] = c; res = kv_get(name, buffer, sizeof(buffer), &actual_size); @@ -328,7 +330,7 @@ static void set_several_unvalid_key_names() name[6] = 0; - for (i = 0; i < 11; i++) { + for (i = 0; i < num_of_keys; i++) { name[5] = unvalid[i]; res = kv_set(name, name, sizeof(name), 0); if (unvalid[i] != '/') { @@ -817,7 +819,7 @@ static void iterator_next_full_list() res = kv_iterator_close(kvstore_it); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); - for (i = 0; i < num_of_threads; i++) { + for (i = 0; i < num_of_keys; i++) { res = kv_remove(keys[i]); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); } @@ -855,7 +857,7 @@ static void iterator_next_remove_while_iterating() int i = 0, res = 0; - for (i = 0; i < num_of_keys; i++) { + for (i = 0; i < 3; i++) { int res = kv_set(keys[i], data, data_size, 0); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); } From abbb2485d222f0e2d8d1140b9697db8703fdf25b Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Wed, 27 Nov 2019 18:42:44 +0200 Subject: [PATCH 3/4] Greentea: Fix storage sizes for SecureStore tests. Previously Greentea tests was not initialising its storage before asking for bd->get_program_size(), causing FlashBlockDevice to return zero. This caused both TDBStorage's to use zero for both parameter to SlicingBlockDevice(bd, 0, 0), effetivaly both then used same addresses for slice. This caused SecureStore tests to fail, because writes to internal RBP storage overwrote keys from external storage. Fine-tune TDBStore sizes, so that all tests can fit into storage. --- .../kvstore/general_tests_phase_1/main.cpp | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp b/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp index 5e1419f518..8bef27de2e 100644 --- a/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp +++ b/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp @@ -68,13 +68,18 @@ static int kv_setup = TDBStoreSet; static const int heap_alloc_threshold_size = 4096; +static inline uint32_t align_up(uint32_t val, uint32_t size) +{ + return (((val - 1) / size) + 1) * size; +} + /*----------------initialization------------------*/ //init the blockdevice static void kvstore_init() { int res; - size_t erase_size, ul_bd_size, rbp_bd_size; + size_t program_size, erase_size, ul_bd_size, rbp_bd_size; BlockDevice *sec_bd; res = bd->init(); @@ -109,10 +114,17 @@ static void kvstore_init() flash_bd = new FlashSimBlockDevice(bd); sec_bd = flash_bd; } + res = sec_bd->init(); + TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); - erase_size = sec_bd->get_erase_size(); - ul_bd_size = erase_size * 4; - rbp_bd_size = erase_size * 2; + program_size = sec_bd->get_program_size(); + erase_size = sec_bd->get_erase_size(); + // We must be able to hold at least 10 small keys (20 program sectors) and master record + internal data + ul_bd_size = align_up(program_size * 40, erase_size); + rbp_bd_size = align_up(program_size * 40, erase_size); + + res = sec_bd->deinit(); + TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); ul_bd = new SlicingBlockDevice(sec_bd, 0, ul_bd_size); rbp_bd = new SlicingBlockDevice(sec_bd, ul_bd_size, ul_bd_size + rbp_bd_size); @@ -407,14 +419,14 @@ static void set_several_key_value_sizes() name[6] = 0; - for (i = 0; i < 26; i++) { + for (i = 0; i < 5; i++) { c = i + 'a'; name[5] = c; res = kvstore->set(name, name, sizeof(name), 0); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); } - for (i = 0; i < 26; i++) { + for (i = 0; i < 5; i++) { c = i + 'a'; name[5] = c; res = kvstore->get(name, buffer, sizeof(buffer), &actual_size, 0); From b82e106a430c6bc8095365a07d7df3a0c371ee38 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Thu, 28 Nov 2019 10:59:48 +0200 Subject: [PATCH 4/4] Astyle fixes --- features/storage/kvstore/tdbstore/TDBStore.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/features/storage/kvstore/tdbstore/TDBStore.cpp b/features/storage/kvstore/tdbstore/TDBStore.cpp index 7de5371a50..417a04deb0 100644 --- a/features/storage/kvstore/tdbstore/TDBStore.cpp +++ b/features/storage/kvstore/tdbstore/TDBStore.cpp @@ -657,7 +657,7 @@ int TDBStore::set_finalize(set_handle_t handle) // we should erase one sector more, just to ensure that in case of power failure // next init() would not extend the scan phase to that section as well. os_ret = read_record(_active_area, _free_space_offset, 0, 0, 0, actual_data_size, 0, - false, false, false, false, hash, flags, next_offset); + false, false, false, false, hash, flags, next_offset); if (os_ret == MBED_SUCCESS) { check_erase_before_write(_active_area, _free_space_offset, sizeof(record_header_t)); } @@ -985,7 +985,7 @@ int TDBStore::increment_max_keys(void **ram_table) // Reallocate ram table with new size ram_table_entry_t *old_ram_table = (ram_table_entry_t *) _ram_table; ram_table_entry_t *new_ram_table = new ram_table_entry_t[_max_keys + 1]; - memset(new_ram_table, 0, sizeof(ram_table_entry_t)*(_max_keys + 1)); + memset(new_ram_table, 0, sizeof(ram_table_entry_t) * (_max_keys + 1)); // Copy old content to new table memcpy(new_ram_table, old_ram_table, sizeof(ram_table_entry_t) * _max_keys);