Merge branch 'bugfix/nvs_blob_overwrite' into 'master'

Bugfix of old NVS Blobs handling when legacy compatibility option is set

See merge request espressif/esp-idf!41741

Author: rrtandler

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

@@ -608,7 +608,7 @@ TEST_CASE("nvs api tests", "[nvs]")
     nvs_handle_t handle_2;
     TEST_ESP_OK(nvs_open("namespace2", NVS_READWRITE, &handle_2));
     TEST_ESP_OK(nvs_set_i32(handle_2, "foo", 0x3456789a));
-    const char *str = "value 0123456789abcdef0123456789abcdef";
+    char str[] = "value 0123456789abcdef0123456789abcdef";
     TEST_ESP_OK(nvs_set_str(handle_2, "key", str));
 
     int32_t v1;
@@ -619,7 +619,7 @@ TEST_CASE("nvs api tests", "[nvs]")
     TEST_ESP_OK(nvs_get_i32(handle_2, "foo", &v2));
     CHECK(0x3456789a == v2);
 
-    char buf[strlen(str) + 1];
+    char buf[sizeof(str)];
     size_t buf_len = sizeof(buf);
 
     size_t buf_len_needed;
@@ -1270,13 +1270,15 @@ class RandomTest {
                 } else {
                     blobBufLen = largeBlobLen ;
                 }
-                uint8_t buf[blobBufLen];
+                uint8_t* buf = new uint8_t[blobBufLen];
                 memset(buf, 0, blobBufLen);
 
                 size_t len = blobBufLen;
                 auto err = nvs_get_blob(handle, keys[index], buf, &len);
+                auto eval_result = evaluate(delayCount, err, types[index], written[index], potentially_written[index], buf, values[index], future_values[index], blobBufLen);
+                delete [] buf;
 
-                REQUIRE(evaluate(delayCount, err, types[index], written[index], potentially_written[index], buf, values[index], future_values[index], blobBufLen) == true);
+                REQUIRE(eval_result == true);
                 break;
             }
 
@@ -1371,7 +1373,7 @@ class RandomTest {
                 } else {
                     blobBufLen = largeBlobLen ;
                 }
-                uint8_t buf[blobBufLen];
+                uint8_t* buf = new uint8_t[blobBufLen];
                 memset(buf, 0, blobBufLen);
                 size_t blobLen = gen() % blobBufLen;
                 std::generate_n(buf, blobLen, [&]() -> uint8_t {
@@ -1386,16 +1388,19 @@ class RandomTest {
                 if (err == ESP_ERR_FLASH_OP_FAIL) {
                     // mark potentially written
                     potentially_written[index] = true;
+                    delete [] buf;
                     return err;
                 }
                 if (err == ESP_ERR_NVS_REMOVE_FAILED) {
                     written[index] = true;
                     memcpy(reinterpret_cast<uint8_t *>(values[index]), buf, blobBufLen);
+                    delete [] buf;
                     return ESP_ERR_FLASH_OP_FAIL;
                 }
                 REQUIRE(err == ESP_OK);
                 written[index] = true;
                 memcpy(reinterpret_cast<char *>(values[index]), buf, blobBufLen);
+                delete [] buf;
                 break;
             }
 
@@ -2889,16 +2894,20 @@ TEST_CASE("inconsistent fields in item header with correct crc are handled for m
     // initial values
     uint32_t uval1 = 1;
     uint32_t uval2 = 2;
-    uint8_t blob_data1[blob_key1_chunk_0_len + blob_key1_chunk_1_len];
-    uint8_t blob_data2[blob_key2_chunk_len];
+    size_t blob_data1_len = blob_key1_chunk_0_len + blob_key1_chunk_1_len;
+    size_t blob_data2_len = blob_key2_chunk_len;
+    uint8_t* blob_data1 = new uint8_t[blob_data1_len];
+    uint8_t* blob_data2 = new uint8_t[blob_data2_len];
 
     // value buffers
     uint32_t read_u32_1;
     uint32_t read_u32_2;
-    uint8_t read_blob1[sizeof(blob_data1)];
-    uint8_t read_blob2[sizeof(blob_data2)];
-    size_t read_blob1_size = sizeof(read_blob1);
-    size_t read_blob2_size = sizeof(read_blob2);
+
+    size_t read_blob1_size = blob_data1_len;
+    size_t read_blob2_size = blob_data2_len;
+    uint8_t* read_blob1 = new uint8_t[read_blob1_size];
+    uint8_t* read_blob2 = new uint8_t[read_blob2_size];
+
 
     // Skip one page, 2 entries of page header and 3 entries of valueblob1's BLOB_DATA entries
     const size_t blob_index_offset = 4096 + 32 * 2 + 32 * 3;
@@ -2921,10 +2930,10 @@ TEST_CASE("inconsistent fields in item header with correct crc are handled for m
     // initialize buffers
     read_u32_1 = 0;
     read_u32_2 = 0;
-    memset(blob_data1, 0x55, sizeof(blob_data1));
-    memset(blob_data2, 0xaa, sizeof(blob_data2));
-    memset(read_blob1, 0, sizeof(read_blob1));
-    memset(read_blob2, 0, sizeof(read_blob2));
+    memset(blob_data1, 0x55, blob_data1_len);
+    memset(blob_data2, 0xaa, blob_data2_len);
+    memset(read_blob1, 0, read_blob1_size);
+    memset(read_blob2, 0, read_blob2_size);
 
     // Write initial data to the nvs partition
     {
@@ -2933,8 +2942,8 @@ TEST_CASE("inconsistent fields in item header with correct crc are handled for m
         nvs_handle_t handle;
         TEST_ESP_OK(nvs_open("test", NVS_READWRITE, &handle));
         TEST_ESP_OK(nvs_set_u32(handle, ukey1, uval1));
-        TEST_ESP_OK(nvs_set_blob(handle, blob_key1, blob_data1, sizeof(blob_data1)));
-        TEST_ESP_OK(nvs_set_blob(handle, blob_key2, blob_data2, sizeof(blob_data2)));
+        TEST_ESP_OK(nvs_set_blob(handle, blob_key1, blob_data1, blob_data1_len));
+        TEST_ESP_OK(nvs_set_blob(handle, blob_key2, blob_data2, blob_data2_len));
         TEST_ESP_OK(nvs_set_u32(handle, ukey2, uval2));
         nvs_close(handle);
 
@@ -3067,16 +3076,17 @@ TEST_CASE("inconsistent fields in item header with correct crc are handled for m
         nvs_handle_t handle;
         TEST_ESP_OK(nvs_open("test", NVS_READWRITE, &handle));
         TEST_ESP_OK(nvs_get_u32(handle, ukey1, &read_u32_1));
-        TEST_ESP_ERR(nvs_get_blob(handle, blob_key1, &read_blob1, &read_blob1_size), ESP_ERR_NVS_NOT_FOUND);
-        TEST_ESP_OK(nvs_get_blob(handle, blob_key2, &read_blob2, &read_blob2_size));
+        TEST_ESP_ERR(nvs_get_blob(handle, blob_key1, read_blob1,  &read_blob1_size), ESP_ERR_NVS_NOT_FOUND);
+        TEST_ESP_OK(nvs_get_blob(handle, blob_key2, read_blob2, &read_blob2_size));
         TEST_ESP_OK(nvs_get_u32(handle, ukey2, &read_u32_2));
         nvs_close(handle);
         TEST_ESP_OK(nvs_flash_deinit_partition(f.part()->get_partition_name()));
 
         // validate the data
         CHECK(read_u32_1 == uval1);
         CHECK(read_u32_2 == uval2);
-        CHECK(memcmp(read_blob2, blob_data2, sizeof(blob_data2)) == 0);
+        CHECK(read_blob2_size == blob_data2_len);
+        CHECK(memcmp(read_blob2, blob_data2, read_blob2_size) == 0);
     }
 }
 
@@ -3808,6 +3818,8 @@ TEST_CASE("nvs multiple write with same key but different types", "[nvs]")
     TEST_ESP_OK(nvs_flash_deinit_partition(NVS_DEFAULT_PART_NAME));
 }
 
+#ifndef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
+// Following test case is not valid for new behavior not leading to multiple active values under the same key
 TEST_CASE("nvs multiple write with same key blob and string involved", "[nvs]")
 {
     PartitionEmulationFixture f(0, 10);
@@ -3883,6 +3895,7 @@ TEST_CASE("nvs multiple write with same key blob and string involved", "[nvs]")
 
     TEST_ESP_OK(nvs_flash_deinit_partition(NVS_DEFAULT_PART_NAME));
 }
+#endif // !CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
 
 TEST_CASE("nvs find key tests", "[nvs]")
 {

components/nvs_flash/host_test/nvs_host_test/pytest_nvs_host_linux.py

@@ -6,6 +6,14 @@
 
 
 @pytest.mark.host_test
+@pytest.mark.parametrize(
+    'config',
+    [
+        'default_set_key',
+        'legacy_set_key',
+    ],
+    indirect=True,
+)
 @idf_parametrize('target', ['linux'], indirect=['target'])
 def test_nvs_host_linux(dut: Dut) -> None:
     dut.expect_exact('All tests passed', timeout=60)

components/nvs_flash/src/nvs_storage.cpp

@@ -407,6 +407,27 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
         if(err == ESP_OK && findPage != nullptr) {
             matchedTypePageFound = true;
         }
+#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
+        // In legacy mode, we also try to find the item as BLOB if not found as BLOB_INDEX.
+        // In this mode, it is possible to have multiple active values under the same (logical) key.
+        // For BLOBs (which may have different physical representations in V1 it is BLOB, in V2 it is BLOB_INDEX) it in turn means
+        // that we have to check both datatypes to find the old value.
+        // The general case for compatibility flag disabled is below and handles all datatypes including BLOB.
+        // To save some cycles, we do not compile both findItem calls in this case.
+        if(err == ESP_ERR_NVS_NOT_FOUND) {
+            // If not found as BLOB_INDEX, try to find it as BLOB (legacy support).
+            err = findItem(nsIndex, ItemType::BLOB, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex);
+            if(err == ESP_OK && findPage != nullptr) {
+                matchedTypePageFound = false;   // datatype does not match, we cannot extract chunkStart from the item
+
+                // 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
     } else {
         // Handle all other data types than BLOB
         err = findItem(nsIndex, datatype, key, findPage, item, Page::CHUNK_ANY, VerOffset::VER_ANY, &itemIndex);