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.

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)

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();
     

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;
         }
 

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);