Merge branch 'bugfix/nvs_corrupted_storage_crashes_app' into 'master'

[NVS]: fix crashes from entry state 1

See merge request espressif/esp-idf!14548

components/nvs_flash/host_test/fixtures/test_fixtures.hpp

@@ -122,6 +122,9 @@ struct PartitionMockFixture {
             const char *partition_name = NVS_DEFAULT_PART_NAME)
         : part_mock(start_sector * SPI_FLASH_SEC_SIZE, sector_size * SPI_FLASH_SEC_SIZE) {
         std::fill_n(raw_header, sizeof(raw_header)/sizeof(raw_header[0]), UINT8_MAX);
+
+        // This resets the mocks and prevents meeting accidental expectations from previous tests.
+        Mockesp_partition_Init();
     }
 
     ~PartitionMockFixture() { }
@@ -151,7 +154,7 @@ struct NVSPageFixture : public PartitionMockFixture {
     nvs::Page page;
 };
 
-struct NVSValidPageFixture : public PartitionMockFixture {
+struct NVSValidPageFlashFixture : public PartitionMockFixture {
     const static uint8_t NS_INDEX = 1;
 
     // valid header
@@ -164,7 +167,7 @@ struct NVSValidPageFixture : public PartitionMockFixture {
 
     uint8_t value_entry [32];
 
-    NVSValidPageFixture(uint32_t start_sector = 0,
+    NVSValidPageFlashFixture(uint32_t start_sector = 0,
             uint32_t sector_size = 1,
             const char *partition_name = NVS_DEFAULT_PART_NAME)
         : PartitionMockFixture(start_sector, sector_size, partition_name),
@@ -173,8 +176,7 @@ struct NVSValidPageFixture : public PartitionMockFixture {
         ns_entry {0x00, 0x01, 0x01, 0xff, 0x68, 0xc5, 0x3f, 0x0b, 't', 'e', 's', 't', '_', 'n', 's', '\0',
                 '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', 1, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
         value_entry {0x01, 0x01, 0x01, 0xff, 0x3d, 0xf3, 0x99, 0xe5, 't', 'e', 's', 't', '_', 'v', 'a', 'l',
-                'u', 'e', '\0', '\0', '\0', '\0', '\0', '\0', 47, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
-        page()
+                'u', 'e', '\0', '\0', '\0', '\0', '\0', '\0', 47, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}
     {
         std::fill_n(raw_entry_table, sizeof(raw_entry_table)/sizeof(raw_entry_table[0]), 0);
         raw_entry_table[0] = 0xfa;
@@ -202,7 +204,15 @@ struct NVSValidPageFixture : public PartitionMockFixture {
         // read normal entry second time during duplicated entry check
         esp_partition_read_ExpectAnyArgsAndReturn(ESP_OK);
         esp_partition_read_ReturnArrayThruPtr_dst(value_entry, 32);
+    }
+};
 
+struct NVSValidPageFixture : public NVSValidPageFlashFixture {
+    NVSValidPageFixture(uint32_t start_sector = 0,
+            uint32_t sector_size = 1,
+            const char *partition_name = NVS_DEFAULT_PART_NAME)
+        : NVSValidPageFlashFixture(start_sector, sector_size, partition_name), page()
+    {
         if (page.load(&part_mock, start_sector) != ESP_OK) throw FixtureException("couldn't setup page");
     }
 
@@ -392,9 +402,6 @@ struct NVSFullPageFixture : public PartitionMockFixture {
                 'u', 'e', '\0', '\0', '\0', '\0', '\0', '\0', 47, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
         page()
     {
-        std::fill_n(raw_entry_table, sizeof(raw_entry_table)/sizeof(raw_entry_table[0]), 0);
-        raw_entry_table[0] = 0xfa;
-
         // entry_table with all elements deleted except the namespace entry written and the last entry free
         std::fill_n(raw_entry_table, sizeof(raw_entry_table)/sizeof(raw_entry_table[0]), 0);
         raw_entry_table[0] = 0x0a;

components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp

@@ -30,6 +30,8 @@ void test_Page_load_reading_header_fails()
 
     TEST_ASSERT_EQUAL(Page::PageState::INVALID, page.state());
     TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG, page.load(&mock, 0));
+
+    Mockesp_partition_Verify();
 }
 
 void test_Page_load_reading_data_fails()
@@ -44,6 +46,8 @@ void test_Page_load_reading_data_fails()
 
     TEST_ASSERT_EQUAL(Page::PageState::INVALID, page.state());
     TEST_ASSERT_EQUAL(ESP_FAIL, page.load(&mock, 0));
+
+    Mockesp_partition_Verify();
 }
 
 void test_Page_load__uninitialized_page_has_0xfe()
@@ -62,6 +66,8 @@ void test_Page_load__uninitialized_page_has_0xfe()
     TEST_ASSERT_EQUAL(ESP_OK, page.load(&fix.part_mock, 0));
 
     TEST_ASSERT_EQUAL(Page::PageState::CORRUPT, page.state());
+
+    Mockesp_partition_Verify();
 }
 
 void test_Page_load__initialized_corrupt_header()
@@ -79,6 +85,60 @@ void test_Page_load__initialized_corrupt_header()
     TEST_ASSERT_EQUAL(ESP_OK, page.load(&fix.part_mock, 0));
 
     TEST_ASSERT_EQUAL(Page::PageState::CORRUPT, page.state());
+
+    Mockesp_partition_Verify();
+}
+
+void test_Page_load__corrupt_entry_table()
+{
+    PartitionMockFixture fix;
+
+    // valid header
+    uint8_t raw_header_valid [32] = {0xfe, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+                0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc2, 0x16, 0xdd, 0xdc};
+
+    // entry table with one entry
+    uint8_t raw_entry_table [32];
+
+    uint8_t ns_entry [32] = {0x00, 0x01, 0x01, 0xff, 0x68, 0xc5, 0x3f, 0x0b, 't', 'e', 's', 't', '_', 'n', 's', '\0',
+                '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', 1, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+
+    uint8_t raw_header[4] = {0xff, 0xff, 0xff, 0xff};
+    std::fill_n(raw_entry_table, sizeof(raw_entry_table)/sizeof(raw_entry_table[0]), 0);
+    raw_entry_table[0] = 0xfa;
+
+    // read page header
+    esp_partition_read_raw_ExpectAnyArgsAndReturn(ESP_OK);
+    esp_partition_read_raw_ReturnArrayThruPtr_dst(raw_header_valid, 32);
+
+    // read entry table
+    esp_partition_read_raw_ExpectAnyArgsAndReturn(ESP_OK);
+    esp_partition_read_raw_ReturnArrayThruPtr_dst(raw_entry_table, 32);
+
+    // read next free entry's header
+    esp_partition_read_raw_ExpectAnyArgsAndReturn(ESP_OK);
+    esp_partition_read_raw_ReturnArrayThruPtr_dst(raw_header, 4);
+
+    // read namespace entry
+    esp_partition_read_ExpectAnyArgsAndReturn(ESP_OK);
+    esp_partition_read_ReturnArrayThruPtr_dst(ns_entry, 32);
+
+    // we expect a raw word write from the partition in order to change the entry bits to erased (0)
+    esp_partition_write_raw_ExpectAndReturn(&fix.part_mock.partition, 32, nullptr, 4, ESP_OK);
+    esp_partition_write_raw_IgnoreArg_src();
+
+    // corrupt entry table as well as crc of corresponding item
+    raw_entry_table[0] = 0xf6;
+
+    Page page;
+
+    // Page::load() should return ESP_OK, but state has to be corrupt
+    TEST_ASSERT_EQUAL(ESP_OK, page.load(&fix.part_mock, 0));
+
+    TEST_ASSERT_EQUAL(Page::PageState::ACTIVE, page.state());
+    TEST_ASSERT_EQUAL(1, page.getUsedEntryCount());
+
+    Mockesp_partition_Verify();
 }
 
 void test_Page_load_success()
@@ -886,6 +946,7 @@ int main(int argc, char **argv)
     RUN_TEST(test_Page_load_reading_data_fails);
     RUN_TEST(test_Page_load__uninitialized_page_has_0xfe);
     RUN_TEST(test_Page_load__initialized_corrupt_header);
+    RUN_TEST(test_Page_load__corrupt_entry_table);
     RUN_TEST(test_Page_load_success);
     RUN_TEST(test_Page_load_full_page);
     RUN_TEST(test_Page_load__seq_number_0);

components/nvs_flash/src/nvs_item_hash_list.cpp

@@ -65,7 +65,7 @@ esp_err_t HashList::insert(const Item& item, size_t index)
     return ESP_OK;
 }
 
-void HashList::erase(size_t index, bool itemShouldExist)
+bool HashList::erase(size_t index)
 {
     for (auto it = mBlockList.begin(); it != mBlockList.end();) {
         bool haveEntries = false;
@@ -81,7 +81,7 @@ void HashList::erase(size_t index, bool itemShouldExist)
             }
             if (haveEntries && foundIndex) {
                 /* item was found, and HashListBlock still has some items */
-                return;
+                return true;
             }
         }
         /* no items left in HashListBlock, can remove */
@@ -95,12 +95,12 @@ void HashList::erase(size_t index, bool itemShouldExist)
         }
         if (foundIndex) {
             /* item was found and empty HashListBlock was removed */
-            return;
+            return true;
         }
     }
-    if (itemShouldExist) {
-        assert(false && "item should have been present in cache");
-    }
+
+    // item hasn't been present in cache");
+    return false;
 }
 
 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();
 
     esp_err_t insert(const Item& item, size_t index);
-    void erase(const size_t index, bool itemShouldExist=true);
+    bool erase(const size_t index);
     size_t find(size_t start, const Item& item);
     void clear();
 

components/nvs_flash/src/nvs_page.cpp

@@ -389,8 +389,9 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, ui
 
 esp_err_t Page::eraseEntryAndSpan(size_t index)
 {
+    uint32_t seq_num;
+    getSeqNumber(seq_num);
     auto state = mEntryTable.get(index);
-    assert(state == EntryState::WRITTEN || state == EntryState::EMPTY);
 
     size_t span = 1;
     if (state == EntryState::WRITTEN) {
@@ -400,7 +401,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index)
             return rc;
         }
         if (item.calculateCrc32() != item.crc32) {
-            mHashList.erase(index, false);
+            mHashList.erase(index);
             rc = alterEntryState(index, EntryState::ERASED);
             --mUsedEntryCount;
             ++mErasedEntryCount;
@@ -597,6 +598,16 @@ esp_err_t Page::mLoadEntryTable()
                 continue;
             }
 
+            if (mEntryTable.get(i) == EntryState::ILLEGAL) {
+                lastItemIndex = INVALID_ENTRY;
+                auto err = eraseEntryAndSpan(i);
+                if (err != ESP_OK) {
+                    mState = PageState::INVALID;
+                    return err;
+                }
+                continue;
+            }
+
             lastItemIndex = i;
 
             auto err = readEntry(i, item);

components/nvs_flash/src/nvs_page.hpp

@@ -175,6 +175,7 @@ protected:
         EMPTY   = 0x3, // 0b11, default state after flash erase
         WRITTEN = EMPTY & ~ESB_WRITTEN, // entry was written
         ERASED  = WRITTEN & ~ESB_ERASED, // entry was written and then erased
+        ILLEGAL = 0x1, // only possible if flash is inconsistent
         INVALID = 0x4 // entry is in inconsistent state (write started but ESB_WRITTEN has not been set yet)
     };
 

components/nvs_flash/test_nvs_host/test_nvs.cpp

@@ -313,7 +313,8 @@ TEST_CASE("HashList is cleaned up as soon as items are erased", "[nvs]")
     INFO("Added " << count << " items, " << hashlist.getBlockCount() << " blocks");
     // Remove them in reverse order
     for (size_t i = count; i > 0; --i) {
-        hashlist.erase(i - 1, true);
+        // Make sure that the element existed before it's erased
+        CHECK(hashlist.erase(i - 1) == true);
     }
     CHECK(hashlist.getBlockCount() == 0);
     // Add again
@@ -326,7 +327,7 @@ TEST_CASE("HashList is cleaned up as soon as items are erased", "[nvs]")
     INFO("Added " << count << " items, " << hashlist.getBlockCount() << " blocks");
     // Remove them in the same order
     for (size_t i = 0; i < count; ++i) {
-        hashlist.erase(i, true);
+        CHECK(hashlist.erase(i) == true);
     }
     CHECK(hashlist.getBlockCount() == 0);
 }

components/spi_flash/mock/mock_config.yaml

@@ -5,3 +5,4 @@
             - return_thru_ptr
             - array
             - callback
+            - ignore_arg