diff --git a/components/nvs_flash/host_test/fixtures/test_fixtures.hpp b/components/nvs_flash/host_test/fixtures/test_fixtures.hpp index 06b12ce176..b0915682f1 100644 --- a/components/nvs_flash/host_test/fixtures/test_fixtures.hpp +++ b/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; diff --git a/components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp b/components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp index ec63aeac76..e5ca9ed089 100644 --- a/components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp +++ b/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); diff --git a/components/nvs_flash/src/nvs_item_hash_list.cpp b/components/nvs_flash/src/nvs_item_hash_list.cpp index 7e1c1241a6..21bf8b3153 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.cpp +++ b/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) diff --git a/components/nvs_flash/src/nvs_item_hash_list.hpp b/components/nvs_flash/src/nvs_item_hash_list.hpp index ca21c92c18..e724c4f02f 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(); 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(); diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index 2f72a2090d..8cc0e1724a 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -393,8 +393,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) { @@ -404,7 +405,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; @@ -601,6 +602,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); diff --git a/components/nvs_flash/src/nvs_page.hpp b/components/nvs_flash/src/nvs_page.hpp index 5857f1ffe4..f4c00e84ca 100644 --- a/components/nvs_flash/src/nvs_page.hpp +++ b/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) }; diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp index bf2712e796..76affc310d 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/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); } diff --git a/components/spi_flash/mock/mock_config.yaml b/components/spi_flash/mock/mock_config.yaml index 60dd3b5267..392ecceea7 100644 --- a/components/spi_flash/mock/mock_config.yaml +++ b/components/spi_flash/mock/mock_config.yaml @@ -5,3 +5,4 @@ - return_thru_ptr - array - callback + - ignore_arg