Merge branch 'bugfix/nvs_failed_crc_assert' into 'master'

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

See merge request idf/esp-idf!2239

(cherry picked from commit 17ab60d642)

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

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