From ea577c8ae0743d02aaaf8ffd89bce4d14c26c73c Mon Sep 17 00:00:00 2001 From: Dong Heng Date: Tue, 22 Jan 2019 11:46:35 +0800 Subject: [PATCH] =?UTF-8?q?fix(nvs=5Fflash):=20don=E2=80=99t=20expect=20it?= =?UTF-8?q?ems=20with=20bad=20CRC=20to=20be=20in=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When erasing a variable length item with an incorrect CRC32, the span value of the item can not be trusted, so the item will be erased with span = 1. Subsequent entries represent the data of the variable length item, and these will be treated as separate items. For each entry CRC32 is checked, the check most likely fails (because the entry contains arbitrary data, and not a proper NVS item), and the entry is erased. Erase function assumed that every item should be present in cache, but it is not the case for the entries which are just parts of item’s payload. This change allows for the item to be not found in the hashlist, if the CRC32 check fails. esp-idf commit ID: 2c3644a0 --- .../nvs_flash/src/nvs_item_hash_list.cpp | 6 +++-- .../nvs_flash/src/nvs_item_hash_list.hpp | 2 +- components/nvs_flash/src/nvs_page.cpp | 22 ++++++++------- .../nvs_flash/test_nvs_host/test_nvs.cpp | 27 ++++++++++++++++--- 4 files changed, 40 insertions(+), 17 deletions(-) diff --git a/components/nvs_flash/src/nvs_item_hash_list.cpp b/components/nvs_flash/src/nvs_item_hash_list.cpp index e483c596..845dd091 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.cpp +++ b/components/nvs_flash/src/nvs_item_hash_list.cpp @@ -60,7 +60,7 @@ void HashList::insert(const Item& item, size_t index) newBlock->mCount++; } -void HashList::erase(size_t index) +void HashList::erase(size_t index, bool itemShouldExist) { for (auto it = mBlockList.begin(); it != mBlockList.end();) { bool haveEntries = false; @@ -82,7 +82,9 @@ void HashList::erase(size_t index) ++it; } } - assert(false && "item should have been present in cache"); + if (itemShouldExist) { + assert(false && "item should have been present in cache"); + } } size_t HashList::find(size_t start, const Item& item) diff --git a/components/nvs_flash/src/nvs_item_hash_list.hpp b/components/nvs_flash/src/nvs_item_hash_list.hpp index 3f8dcc85..e759cd81 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.hpp +++ b/components/nvs_flash/src/nvs_item_hash_list.hpp @@ -29,7 +29,7 @@ public: ~HashList(); void insert(const Item& item, size_t index); - void erase(const size_t index); + void erase(const size_t index, bool itemShouldExist=true); size_t find(size_t start, const Item& item); void clear(); diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index a5ee0b6c..bf9007ae 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -318,7 +318,6 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) { auto state = mEntryTable.get(index); assert(state == EntryState::WRITTEN || state == EntryState::EMPTY); - mHashList.erase(index); size_t span = 1; if (state == EntryState::WRITTEN) { @@ -328,6 +327,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) return rc; } if (item.calculateCrc32() != item.crc32) { + mHashList.erase(index, false); rc = alterEntryState(index, EntryState::ERASED); --mUsedEntryCount; ++mErasedEntryCount; @@ -335,6 +335,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) return rc; } } else { + mHashList.erase(index); span = item.span; for (ptrdiff_t i = index + span - 1; i >= static_cast(index); --i) { if (mEntryTable.get(i) == EntryState::WRITTEN) { @@ -515,11 +516,6 @@ esp_err_t Page::mLoadEntryTable() return err; } - mHashList.insert(item, i); - - // search for potential duplicate item - size_t duplicateIndex = mHashList.find(0, item); - if (item.crc32 != item.calculateCrc32()) { err = eraseEntryAndSpan(i); if (err != ESP_OK) { @@ -529,6 +525,10 @@ esp_err_t Page::mLoadEntryTable() continue; } + mHashList.insert(item, i); + + // search for potential duplicate item + size_t duplicateIndex = mHashList.find(0, item); if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) { span = item.span; @@ -580,8 +580,6 @@ esp_err_t Page::mLoadEntryTable() return err; } - mHashList.insert(item, i); - if (item.crc32 != item.calculateCrc32()) { err = eraseEntryAndSpan(i); if (err != ESP_OK) { @@ -590,9 +588,9 @@ esp_err_t Page::mLoadEntryTable() } continue; } - assert(item.span > 0); + mHashList.insert(item, i); size_t span = item.span; if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) { @@ -740,7 +738,11 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si auto crc32 = item.calculateCrc32(); if (item.crc32 != crc32) { - eraseEntryAndSpan(i); + rc = eraseEntryAndSpan(i); + if (rc != ESP_OK) { + mState = PageState::INVALID; + return rc; + } continue; } diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp index 0979bb40..c23f5d66 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -132,14 +132,14 @@ TEST_CASE("when writing and erasing, used/erased counts are updated correctly", CHECK(page.getErasedEntryCount() == 1); for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.writeItem(1, name, i) == ESP_OK); } CHECK(page.getUsedEntryCount() == Page::ENTRY_COUNT - 1); CHECK(page.getErasedEntryCount() == 1); for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.eraseItem(1, itemTypeOf(), name) == ESP_OK); } CHECK(page.getUsedEntryCount() == 1); @@ -153,7 +153,7 @@ TEST_CASE("when page is full, adding an element fails", "[nvs]") CHECK(page.load(0) == ESP_OK); for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.writeItem(1, name, i) == ESP_OK); } CHECK(page.writeItem(1, "foo", 64UL) == ESP_ERR_NVS_PAGE_FULL); @@ -259,6 +259,25 @@ TEST_CASE("Page validates blob size", "[nvs]") TEST_ESP_OK(page.writeItem(1, ItemType::BLOB, "2", buf, Page::BLOB_MAX_SIZE)); } +TEST_CASE("Page handles invalid CRC of variable length items", "[nvs][cur]") +{ + SpiFlashEmulator emu(4); + { + Page page; + TEST_ESP_OK(page.load(0)); + char buf[128] = {0}; + TEST_ESP_OK(page.writeItem(1, ItemType::BLOB, "1", buf, sizeof(buf))); + } + // corrupt header of the item (64 is the offset of the first item in page) + uint32_t overwrite_buf = 0; + emu.write(64, &overwrite_buf, 4); + // load page again + { + Page page; + TEST_ESP_OK(page.load(0)); + } +} + TEST_CASE("can init PageManager in empty flash", "[nvs]") { SpiFlashEmulator emu(4); @@ -1123,7 +1142,7 @@ TEST_CASE("crc errors in item header are handled", "[nvs]") // add more items to make the page full for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { char item_name[Item::MAX_KEY_LENGTH + 1]; - snprintf(item_name, sizeof(item_name), "item_%ld", i); + snprintf(item_name, sizeof(item_name), "item_%ld", (long int)i); TEST_ESP_OK(storage.writeItem(1, item_name, static_cast(i))); }