diff --git a/components/nvs_flash/src/nvs_item_hash_list.cpp b/components/nvs_flash/src/nvs_item_hash_list.cpp index e483c5969..845dd0910 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 3f8dcc850..e759cd818 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 11dcb17d1..f72207387 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -314,7 +314,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) { @@ -324,6 +323,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; @@ -331,6 +331,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) { @@ -511,11 +512,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) { @@ -525,6 +521,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; @@ -576,8 +576,6 @@ esp_err_t Page::mLoadEntryTable() return err; } - mHashList.insert(item, i); - if (item.crc32 != item.calculateCrc32()) { err = eraseEntryAndSpan(i); if (err != ESP_OK) { @@ -586,9 +584,11 @@ esp_err_t Page::mLoadEntryTable() } continue; } - + assert(item.span > 0); + mHashList.insert(item, i); + size_t span = item.span; i += span - 1; } @@ -726,7 +726,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 f419256cf..598bd6fab 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -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);