fix(nvs): fixed erasing of old values if new data type is not the same

Closes https://github.com/espressif/esp-idf/issues/15559

components/nvs_flash/host_test/nvs_host_test/main/test_nvs.cpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -3806,6 +3806,82 @@ TEST_CASE("nvs multiple write with same key but different types", "[nvs]")
     TEST_ESP_OK(nvs_flash_deinit_partition(NVS_DEFAULT_PART_NAME));
 }
 
+TEST_CASE("nvs multiple write with same key blob and string involved", "[nvs]")
+{
+    PartitionEmulationFixture f(0, 10);
+
+    nvs_handle_t handle_1;
+    const uint32_t NVS_FLASH_SECTOR = 6;
+    const uint32_t NVS_FLASH_SECTOR_COUNT_MIN = 3;
+    TEMPORARILY_DISABLED(f.emu.setBounds(NVS_FLASH_SECTOR, NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN);)
+
+    for (uint16_t j = NVS_FLASH_SECTOR; j < NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN; ++j) {
+        f.erase(j);
+    }
+    TEST_ESP_OK(nvs::NVSPartitionManager::get_instance()->init_custom(f.part(),
+                                                                      NVS_FLASH_SECTOR,
+                                                                      NVS_FLASH_SECTOR_COUNT_MIN));
+
+    TEST_ESP_OK(nvs_open("namespace1", NVS_READWRITE, &handle_1));
+
+    nvs_erase_all(handle_1);
+
+    const char key_name[] = "foo";
+
+    // integer variables
+    int32_t v32;
+    int8_t v8;
+
+    // string
+    #define str_data_len 64
+    const char str_data[] = "string data";
+    char str_buf[str_data_len] = {0};
+    size_t str_len = str_data_len;
+
+    // blob
+    #define blob_data_len 64
+    uint8_t blob_data[blob_data_len] = {0};
+    uint8_t blob_buf[blob_data_len] = {0};
+    size_t blob_read_size;
+
+    // first write is i32
+    TEST_ESP_OK(nvs_set_i32(handle_1, key_name, (int32_t)12345678));
+
+    TEST_ESP_ERR(nvs_get_i8(handle_1, key_name, &v8), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_OK(nvs_get_i32(handle_1, key_name, &v32));
+    TEST_ESP_ERR(nvs_get_str(handle_1, key_name, str_buf, &str_len), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_get_blob(handle_1, key_name, blob_buf, &blob_read_size), ESP_ERR_NVS_NOT_FOUND);
+
+
+    // second write is string
+    TEST_ESP_OK(nvs_set_str(handle_1, key_name, str_data));
+
+    TEST_ESP_ERR(nvs_get_i8(handle_1, key_name, &v8), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_get_i32(handle_1, key_name, &v32), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_OK(nvs_get_str(handle_1, key_name, str_buf, &str_len));
+    TEST_ESP_ERR(nvs_get_blob(handle_1, key_name, blob_buf, &blob_read_size), ESP_ERR_NVS_NOT_FOUND);
+
+    // third write is blob
+    TEST_ESP_OK(nvs_set_blob(handle_1, key_name, blob_data, blob_data_len));
+
+    TEST_ESP_ERR(nvs_get_i8(handle_1, key_name, &v8), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_get_i32(handle_1, key_name, &v32), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_get_str(handle_1, key_name, str_buf, &str_len), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_OK(nvs_get_blob(handle_1, key_name, blob_buf, &blob_read_size));
+
+    // fourth write is i8
+    TEST_ESP_OK(nvs_set_i8(handle_1, key_name, (int8_t)12));
+
+    TEST_ESP_OK(nvs_get_i8(handle_1, key_name, &v8));
+    TEST_ESP_ERR(nvs_get_i32(handle_1, key_name, &v32), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_get_str(handle_1, key_name, str_buf, &str_len), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_get_blob(handle_1, key_name, blob_buf, &blob_read_size), ESP_ERR_NVS_NOT_FOUND);
+
+    nvs_close(handle_1);
+
+    TEST_ESP_OK(nvs_flash_deinit_partition(NVS_DEFAULT_PART_NAME));
+}
+
 TEST_CASE("nvs find key tests", "[nvs]")
 {
     const size_t buff_len = 4096;

components/nvs_flash/src/nvs_storage.cpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -372,98 +372,130 @@ esp_err_t Storage::writeMultiPageBlob(uint8_t nsIndex, const char* key, const vo
     return err;
 }
 
+// datatype BLOB is written as BLOB_INDEX and BLOB_DATA and is searched for previous value as BLOB_INDEX and/or BLOB
+// datatype BLOB_IDX and BLOB_DATA are not supported as input parameters, the layer above should always use BLOB
 esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key, const void* data, size_t dataSize)
 {
     if(mState != StorageState::ACTIVE) {
         return ESP_ERR_NVS_NOT_INITIALIZED;
     }
 
+    // pointer to the page where the existing item was found
     Page* findPage = nullptr;
+    // page sequence number helping to detect whether the page with old value was relocated during the new write
+    uint32_t findPageSeqNumber = UINT32_MAX;
+
+    // indicates the datatype representation match between the old value and the new one
     bool matchedTypePageFound = false;
+
+    // contains the item with the old value, if found
     Item item;
 
-    esp_err_t err;
+    esp_err_t err = ESP_OK;
+
+    // Try to find existing item with the same key and namespace index
+    // We are performing the findItem with datatype specified (it is not ANY) to ensure the hash list lookup is done.
     if(datatype == ItemType::BLOB) {
+        // Specific lookup if performed for input datatype BLOB. The searched datatype for exact match is BLOB_INDEX.
+        // The BLOB_INDEX is used to store the metadata of the (originally typed) BLOB data in current V2 implementation.
         err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item);
-        if(err == ESP_OK) {
-            matchedTypePageFound = true;
-        }
-    } else {
-#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
-        err = findItem(nsIndex, datatype, key, findPage, item);
         if(err == ESP_OK && findPage != nullptr) {
             matchedTypePageFound = true;
         }
-#else
+    } else {
-        err = findItem(nsIndex, ItemType::ANY, key, findPage, item);
+        // Handle all other data types than BLOB
-        if(err == ESP_OK && datatype == item.datatype) {
+        err = findItem(nsIndex, datatype, key, findPage, item);
+        if(err == ESP_OK && findPage != nullptr) {
             matchedTypePageFound = true;
+
+            // keep the sequence number of the page where the item was found for later check of relocation
+            err = findPage->getSeqNumber(findPageSeqNumber);
+            if(err != ESP_OK) {
+                return err;
+            }
         }
-#endif
     }
 
+#ifndef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
+    // If the item was not found, try to find it with any other datatype. Omit the BLOB_DATA and handle BLOB with respect
+    // to the V1 and V2 representation.
+    const ItemType dataTypes[] = {
+        ItemType::U8,
+        ItemType::U16,
+        ItemType::U32,
+        ItemType::U64,
+        ItemType::I8,
+        ItemType::I16,
+        ItemType::I32,
+        ItemType::I64,
+        ItemType::SZ,
+        ItemType::BLOB_IDX,
+        ItemType::BLOB
+    };
+
+    if(findPage == nullptr) {
+        // Iterate over potential data types to allow findItem() search using the hash list instead of bruteforce search.
+        for(const auto& currType : dataTypes) {
+            // Skip search for BLOB_IDX if the requested datatype is BLOB. BLOB_IDX was already searched above.
+            if(datatype == ItemType::BLOB && currType == ItemType::BLOB_IDX) continue;
+
+            // Skip search if requested datatype is not BLOB and the current datatype is equal to the requested one. This was already searched above.
+            if(datatype != ItemType::BLOB && currType == datatype) continue;
+
+            err = findItem(nsIndex, currType, key, findPage, item);
+            if(err == ESP_OK && findPage != nullptr) {
+                // keep the sequence number of the page where the item was found for later check of relocation
+                err = findPage->getSeqNumber(findPageSeqNumber);
+                if(err != ESP_OK) {
+                    return err;
+                }
+                // item was found with the same key and namespace index but data type is different
+                matchedTypePageFound = false;
+                break;
+            }
+        }
+    }
+#endif
+
+    // Here the findPage is either nullptr or points to the page where the item was found.
+    // The matchedTypePageFound is true if the old value item was found and its datatype representation matches the new one.
+    // This flag is used to determine if the item should be checked for same value.
     if(err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
         return err;
     }
 
+    // Handle value update
     if(datatype == ItemType::BLOB) {
         VerOffset prevStart,  nextStart;
         prevStart = nextStart = VerOffset::VER_0_OFFSET;
         if(matchedTypePageFound) {
-            // Do a sanity check that the item in question is actually being modified.
+            // Do a check that the item in question is actually being modified.
             // If it isn't, it is cheaper to purposefully not write out new data.
             // since it may invoke an erasure of flash.
             if(cmpMultiPageBlob(nsIndex, key, data, dataSize) == ESP_OK) {
                 return ESP_OK;
             }
 
-            if (findPage->state() == Page::PageState::UNINITIALIZED ||
+            // Get the version of the previous index with same <ns,key>
-                    findPage->state() == Page::PageState::INVALID) {
-                err = findItem(nsIndex, datatype, key, findPage, item);
-                if (err != ESP_OK) {
-                    return err;
-                }
-            }
-            /* Get the version of the previous index with same <ns,key> */
             prevStart = item.blobIndex.chunkStart;
             NVS_ASSERT_OR_RETURN(prevStart == VerOffset::VER_0_OFFSET || prevStart == VerOffset::VER_1_OFFSET, ESP_FAIL);
 
-
+            // Toggle the version by changing the offset
-            /* Toggle the version by changing the offset */
             nextStart
                 = (prevStart == VerOffset::VER_1_OFFSET) ? VerOffset::VER_0_OFFSET : VerOffset::VER_1_OFFSET;
         }
-        /* Write the blob with new version*/
+        // Write the blob with new version
         err = writeMultiPageBlob(nsIndex, key, data, dataSize, nextStart);
 
         if(err == ESP_ERR_NVS_PAGE_FULL) {
             return ESP_ERR_NVS_NOT_ENOUGH_SPACE;
         }
+
         if(err != ESP_OK) {
             return err;
         }
-
-        if (matchedTypePageFound) {
-            /* Erase the blob with earlier version*/
-            err = eraseMultiPageBlob(nsIndex, key, prevStart);
-
-            if (err == ESP_ERR_FLASH_OP_FAIL) {
-                return ESP_ERR_NVS_REMOVE_FAILED;
-            }
-            if (err != ESP_OK) {
-                return err;
-            }
-
-            findPage = nullptr;
     } else {
-            /* Support for earlier versions where BLOBS were stored without index */
+        // Do a check that the item in question is actually being modified.
-            err = findItem(nsIndex, datatype, key, findPage, item);
-            if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
-                return err;
-            }
-        }
-    } else {
-        // Do a sanity check that the item in question is actually being modified.
         // If it isn't, it is cheaper to purposefully not write out new data.
         // since it may invoke an erasure of flash.
         if(matchedTypePageFound &&
@@ -489,42 +521,74 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
             if(err == ESP_ERR_NVS_PAGE_FULL) {
                 return ESP_ERR_NVS_NOT_ENOUGH_SPACE;
             }
-            if (err != ESP_OK) {
-                return err;
         }
-        } else if (err != ESP_OK) {
+
+        if(err != ESP_OK) {
             return err;
         }
     }
 
-    if (findPage) {
+    // Delete previous value
-        if (findPage->state() == Page::PageState::UNINITIALIZED ||
+    // Note: The old entry won't be deleted if the new value is the same as the old value - code won't reach here in that case.
-                findPage->state() == Page::PageState::INVALID) {
+
-#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
+    // If findPage is null then previous value was not present in NVS and nothig is to be deleted.
-            err = findItem(nsIndex, datatype, key, findPage, item);
+    if(findPage == nullptr) {
-#else
-            err = findItem(nsIndex, ItemType::ANY, key, findPage, item);
-#endif
-            if (err != ESP_OK) {
         return err;
     }
-        }
+
-#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
+    if(item.datatype == ItemType::BLOB_IDX) {
-        err = findPage->eraseItem(nsIndex, datatype, key);
+        // If the item found was BLOB_INDEX, the eraseMultiPageBlob is used to erase the whole multi-page blob.
-#else
+        // It is not necessary to check the potential page relocation as the function will find the blob again anyway.
-        err = findPage->eraseItem(nsIndex, ItemType::ANY, key);
+        VerOffset prevStart = item.blobIndex.chunkStart;
-#endif
+        err = eraseMultiPageBlob(nsIndex, key, prevStart);
+
         if(err == ESP_ERR_FLASH_OP_FAIL) {
             return ESP_ERR_NVS_REMOVE_FAILED;
         }
+    } else {
+        // For all other data types, we have to check the potential page relocation.
+
+        // The findPage might have been relocated as a part of space reclaiming.
+        // First indication is the page state. It might become the "spare" page thus changing the state from FULL or ACTIVE.
+        bool wasRelocated = false;
+
+        if( findPage->state() != Page::PageState::ACTIVE &&
+            findPage->state() != Page::PageState::FULL) {
+                wasRelocated = true;
+        }
+
+        // Other indication of the multi step relocation is the page sequence number. If the sequence number is different than page
+        // sequence number at the moment initial item was found, the page was relocated.
+        if(!wasRelocated) {
+            uint32_t newPageSeqNumber;
+            err = findPage->getSeqNumber(newPageSeqNumber);
+            if(err != ESP_OK) {
+                return err;
+            }
+            if(newPageSeqNumber != findPageSeqNumber) {
+                wasRelocated = true;
+            }
+        }
+
+        if(wasRelocated) {
+            // The page was relocated. We have to find the old value again from the beginning.
+            // As the item was already found before relocation, we can use the exact datatype from item.
+            err = findItem(nsIndex, item.datatype, key, findPage, item);
             if(err != ESP_OK) {
                 return err;
             }
         }
+        // Page containing the old value is now refreshed. We can erase the old value.
+        err = findPage->eraseItem(nsIndex, item.datatype, key);
+        if(err == ESP_ERR_FLASH_OP_FAIL) {
+            return ESP_ERR_NVS_REMOVE_FAILED;
+        }
+    }
+
 #ifdef DEBUG_STORAGE
     debugCheck();
 #endif
-    return ESP_OK;
+    return err;
 }
 
 esp_err_t Storage::createOrOpenNamespace(const char* nsName, bool canCreate, uint8_t& nsIndex)