From 2c3644a09f886ba619df6c227e32508b03d337f5 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 12 Mar 2018 06:35:21 +0800 Subject: [PATCH] =?UTF-8?q?nvs:=20don=E2=80=99t=20expect=20items=20with=20?= =?UTF-8?q?bad=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. --- .../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 | 24 +++++++++++-------- .../nvs_flash/test_nvs_host/test_nvs.cpp | 19 +++++++++++++++ 4 files changed, 38 insertions(+), 13 deletions(-) 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 6b8d1fdac..d05f97edc 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 19570e237..016110313 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);