From ece756c0c4c49c0c16b9f617a8eb95915ae2db66 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 2 Dec 2020 10:22:46 +0000 Subject: [PATCH 1/4] TDBStore: pad record header to whole program size before storing it Previously, when writing a record header into a TDBStore, we took the pointer to the record_header_t variable as the input, but used a program-aligned chunk size which is larger. As a result, garbage from the stack memory gets written. This commit fixes that by buffering the record header. Co-authored-by: Seppo Takalo --- storage/kvstore/source/TDBStore.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/storage/kvstore/source/TDBStore.cpp b/storage/kvstore/source/TDBStore.cpp index b8f2e530dc..b0be309ee8 100644 --- a/storage/kvstore/source/TDBStore.cpp +++ b/storage/kvstore/source/TDBStore.cpp @@ -847,7 +847,10 @@ int TDBStore::copy_record(uint8_t from_area, uint32_t from_offset, uint32_t to_o } chunk_size = align_up(sizeof(record_header_t), _prog_size); - ret = write_area(1 - from_area, to_offset, chunk_size, &header); + // The record header takes up whole program units + memset(_work_buf, 0, chunk_size); + memcpy(_work_buf, &header, sizeof(record_header_t)); + ret = write_area(1 - from_area, to_offset, chunk_size, _work_buf); if (ret) { return ret; } From cf977b7aae38503a92102fbfaf007be7ab3bb080 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 2 Dec 2020 14:36:30 +0000 Subject: [PATCH 2/4] TDBStore: the work buffer is at least the program size or 64 bytes Previously, we always set the work buffer to 64 bytes, without checking it was no less the actual program size. But we can't simply switch to use the program size for the work buffer, because if it's too small (e.g. 1 byte in some cases), we will not be able to read the status header (24 bytes), and small buffers means more underlying write operations and lower efficiency. This PR changes the work buffer size to be the program size, or 64 bytes as an absolute minimum like before. --- storage/kvstore/include/kvstore/TDBStore.h | 1 + storage/kvstore/source/TDBStore.cpp | 15 ++++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/storage/kvstore/include/kvstore/TDBStore.h b/storage/kvstore/include/kvstore/TDBStore.h index 8a823a1c66..5115d12675 100644 --- a/storage/kvstore/include/kvstore/TDBStore.h +++ b/storage/kvstore/include/kvstore/TDBStore.h @@ -308,6 +308,7 @@ private: tdbstore_area_data_t _area_params[_num_areas]; uint32_t _prog_size; uint8_t *_work_buf; + size_t _work_buf_size; char *_key_buf; void *_inc_set_handle; void *_iterator_table[_max_open_iterators]; diff --git a/storage/kvstore/source/TDBStore.cpp b/storage/kvstore/source/TDBStore.cpp index b0be309ee8..787e7349c4 100644 --- a/storage/kvstore/source/TDBStore.cpp +++ b/storage/kvstore/source/TDBStore.cpp @@ -77,7 +77,7 @@ typedef struct { uint32_t crc; } reserved_trailer_t; -static const uint32_t work_buf_size = 64; +static const size_t min_work_buf_size = 64; static const uint32_t initial_crc = 0xFFFFFFFF; static const uint32_t initial_max_keys = 16; @@ -130,7 +130,7 @@ static uint32_t calc_crc(uint32_t init_crc, uint32_t data_size, const void *data TDBStore::TDBStore(BlockDevice *bd) : _ram_table(0), _max_keys(0), _num_keys(0), _bd(bd), _buff_bd(0), _free_space_offset(0), _master_record_offset(0), _master_record_size(0), _is_initialized(false), _active_area(0), _active_area_version(0), _size(0), - _area_params{}, _prog_size(0), _work_buf(0), _key_buf(0), _inc_set_handle(0) + _area_params{}, _prog_size(0), _work_buf(0), _work_buf_size(0), _key_buf(0), _inc_set_handle(0) { for (int i = 0; i < _num_areas; i++) { _area_params[i] = { 0 }; @@ -327,7 +327,7 @@ int TDBStore::read_record(uint8_t area, uint32_t offset, char *key, user_key_ptr[key_size] = '\0'; } else { dest_buf = _work_buf; - chunk_size = std::min(key_size, work_buf_size); + chunk_size = std::min(key_size, _work_buf_size); } } else { // This means that we're on the data part @@ -337,13 +337,13 @@ int TDBStore::read_record(uint8_t area, uint32_t offset, char *key, // 3. After actual part is finished - read to work buffer // 4. Copy data flag not set - read to work buffer if (curr_data_offset < data_offset) { - chunk_size = std::min((size_t)work_buf_size, (size_t)(data_offset - curr_data_offset)); + chunk_size = std::min(_work_buf_size, (data_offset - curr_data_offset)); dest_buf = _work_buf; } else if (copy_data && (curr_data_offset < data_offset + actual_data_size)) { chunk_size = actual_data_size; dest_buf = static_cast(data_buf); } else { - chunk_size = std::min(work_buf_size, total_size); + chunk_size = std::min(_work_buf_size, total_size); dest_buf = _work_buf; } } @@ -860,7 +860,7 @@ int TDBStore::copy_record(uint8_t from_area, uint32_t from_offset, uint32_t to_o total_size -= chunk_size; while (total_size) { - chunk_size = std::min(total_size, work_buf_size); + chunk_size = std::min(total_size, _work_buf_size); ret = read_area(from_area, from_offset, chunk_size, _work_buf); if (ret) { return ret; @@ -1044,7 +1044,8 @@ int TDBStore::init() } _prog_size = _bd->get_program_size(); - _work_buf = new uint8_t[work_buf_size]; + _work_buf_size = std::max(_prog_size, min_work_buf_size); + _work_buf = new uint8_t[_work_buf_size]; _key_buf = new char[MAX_KEY_SIZE]; _inc_set_handle = new inc_set_handle_t; memset(_inc_set_handle, 0, sizeof(inc_set_handle_t)); From fd5b2c569dd922f709074ffb44c86da93788b28e Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 2 Dec 2020 14:51:56 +0000 Subject: [PATCH 3/4] Avoid force-casting inputs when calling std::min() --- storage/kvstore/source/TDBStore.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/storage/kvstore/source/TDBStore.cpp b/storage/kvstore/source/TDBStore.cpp index 787e7349c4..6c74ac0a1e 100644 --- a/storage/kvstore/source/TDBStore.cpp +++ b/storage/kvstore/source/TDBStore.cpp @@ -196,7 +196,7 @@ int TDBStore::erase_erase_unit(uint8_t area, uint32_t offset) void TDBStore::calc_area_params() { // TDBStore can't exceed 32 bits - bd_size_t bd_size = std::min(_bd->size(), (bd_size_t) 0x80000000L); + bd_size_t bd_size = std::min(_bd->size(), 0x80000000L); memset(_area_params, 0, sizeof(_area_params)); size_t area_0_size = 0; @@ -293,7 +293,7 @@ int TDBStore::read_record(uint8_t area, uint32_t offset, char *key, return MBED_ERROR_INVALID_SIZE; } - actual_data_size = std::min((size_t)data_buf_size, (size_t)data_size - data_offset); + actual_data_size = std::min(data_buf_size, data_size - data_offset); if (copy_data && actual_data_size && !data_buf) { return MBED_ERROR_INVALID_ARGUMENT; @@ -327,7 +327,7 @@ int TDBStore::read_record(uint8_t area, uint32_t offset, char *key, user_key_ptr[key_size] = '\0'; } else { dest_buf = _work_buf; - chunk_size = std::min(key_size, _work_buf_size); + chunk_size = std::min(key_size, _work_buf_size); } } else { // This means that we're on the data part @@ -860,7 +860,7 @@ int TDBStore::copy_record(uint8_t from_area, uint32_t from_offset, uint32_t to_o total_size -= chunk_size; while (total_size) { - chunk_size = std::min(total_size, _work_buf_size); + chunk_size = std::min(total_size, _work_buf_size); ret = read_area(from_area, from_offset, chunk_size, _work_buf); if (ret) { return ret; From a4907e89dfaec029e81011c787346e47cda6b8b0 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 2 Dec 2020 14:56:08 +0000 Subject: [PATCH 4/4] Bring in TDBStore UNITTESTS improvements from PR #13731 Co-authored-by: Seppo Takalo --- .../tests/UNITTESTS/TDBStore/moduletest.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/storage/kvstore/tests/UNITTESTS/TDBStore/moduletest.cpp b/storage/kvstore/tests/UNITTESTS/TDBStore/moduletest.cpp index bab1422192..c51680e5f9 100644 --- a/storage/kvstore/tests/UNITTESTS/TDBStore/moduletest.cpp +++ b/storage/kvstore/tests/UNITTESTS/TDBStore/moduletest.cpp @@ -20,14 +20,14 @@ #include "kvstore/TDBStore.h" #include -#define BLOCK_SIZE (512) +#define BLOCK_SIZE (256) #define DEVICE_SIZE (BLOCK_SIZE*200) using namespace mbed; class TDBStoreModuleTest : public testing::Test { protected: - HeapBlockDevice heap{DEVICE_SIZE}; + HeapBlockDevice heap{DEVICE_SIZE, BLOCK_SIZE}; FlashSimBlockDevice flash{&heap}; TDBStore tdb{&flash}; @@ -77,15 +77,17 @@ TEST_F(TDBStoreModuleTest, erased_set_get) TEST_F(TDBStoreModuleTest, set_deinit_init_get) { - char buf[100]; size_t size; for (int i = 0; i < 100; ++i) { - EXPECT_EQ(tdb.set("key", "data", 5, 0), MBED_SUCCESS); + char str[11] = {0}; + char buf[100] = {0}; + int len = snprintf(str, 11, "data%d", i) + 1; + EXPECT_EQ(tdb.set("key", str, len, 0), MBED_SUCCESS); EXPECT_EQ(tdb.deinit(), MBED_SUCCESS); EXPECT_EQ(tdb.init(), MBED_SUCCESS); EXPECT_EQ(tdb.get("key", buf, 100, &size), MBED_SUCCESS); - EXPECT_EQ(size, 5); - EXPECT_STREQ("data", buf); + EXPECT_EQ(size, len); + EXPECT_STREQ(str, buf); EXPECT_EQ(tdb.remove("key"), MBED_SUCCESS); } }