nvs: don’t expect items with bad CRC to be in cache

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.

diff --git a/components/nvs_flash/src/nvs_item_hash_list.cpp b/components/nvs_flash/src/nvs_item_hash_list.cpp
index e483c5969789c93f8f2d96a0ead5bc6b2988de53..845dd091065b53abe4fc1476f0590ee2ed8bedf5 100644 (file)
--- 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 3f8dcc850a6dd20fd4018a6eb2e899d12e26dbed..e759cd818a8acaa304e8b491c1f4d16aa742231e 100644 (file)
--- 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 6b8d1fdacba20fa478c9907f7ee63e8519632279..d05f97edc2b612e963f460d82893f5fb5c2d67c5 100644 (file)
--- 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<ptrdiff_t>(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 19570e237e61b412cc5027f3466a5960b025106a..016110313865bae371cd08ea88d62fa07432841d 100644 (file)
--- 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);