nvs: nvs_set_*() calls on same key but different types succeed and create multiple entries (IDFGH-9727)

[crackwitz]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v5.0.1

Operating System used.

Windows

How did you build your project?

VS Code IDE

If you are using Windows, please specify command line type.

PowerShell

Development Kit.

ESP32 Ethernet Kit

Power Supply used.

USB

What is the expected behavior?

nvs_set_*() returns error code indicating that the key exists but has a different type.

What is the actual behavior?

nvs_set_*() call succeeds, creates additional entry for the same key.

Steps to reproduce.

ESP_ERROR_CHECK( nvs_set_str(nvshandle, "test", "hello world") );
ESP_ERROR_CHECK( nvs_set_i32(nvshandle, "test", 1234) );

Note: presence/absence of commit() calls makes no difference.

Debug Logs.

No response

More Information.

The call creates multiple entries for the same string key, as visible from nvs_entry_find().

I can even create multiple entries with the same type.

The data of all set() calls does make it into the partition. I can access all of it by repeatedly deleting the topmost entry, which reveals the value of the next entry for that key.

This directly contradicts documentation.

[igrr]

@crackwitz Thanks for reporting the issue. Indeed, the docs are wrong — we never checked whether there is an existing key-value pair with the same key but a different type, during the write operation.

It's possible to fix this so that it behaves as described in the docs, however we would have to be careful about this... Existing applications may have relied on this (incorrect) behavior, so worst case fixing this might affect devices deployed in the field after they are updated to use the new IDF version.

We can update the docs to describe the existing behavior as the first step, then revisit this bug before the next major release (v6.0)

[crackwitz]

Allow me to describe the behavior I see.

Multiple entries for the same key can exist, even multiple entries for the same key and type. I shall refer to them in order of iteration, "top" (older) to "bottom" (newer).

erase(): erases the topmost entry matching the key (any type), or NOT_FOUND.

set() with certain type:

• no matching key: new entry.
• matching keys exist, topmost entry's type matches: erased, new entry added.
• topmost entry's type does not match: nothing is erased (!), just another entry added.

get()with certain type:

• topmost entry is found
• if type matches, success. if type does not match: failure

There is no way to get the value of any entry but the topmost one. In some situations, the topmost entry will not even be erased by an update (set), which means that set() calls on an inconsistent state may not have a direct effect.

Given the right sequence of set() calls, I can produce an arbitrary amount of entries with arbitrary types, all with the same key.

I bow before anyone who relies on that behavior. ;)

[igrr]

the topmost entry will not even be erased by an update (set), which means that set() calls on an inconsistent state may not have a direct effect

That's a very good point! Indeed this can lead to the behavior which is pretty confusing and hard to troubleshoot.

[rrtandler]

Hi @crackwitz,

We have had internal discussion and it resulted in making the nvs_set* behaviour more predictable while respecting the numerous implementations already deployed in the field.

As Is state:
The actual documentation states, that an attempt to set key with value of different data type returns failure code (and nothing is written). While in actual implementation the result of non matching set operation is ESP_OK code with value written to the storage, although as hidden value not accessible by nvs_get* function. Moreover multiple nvs_set* calls with data type not matching the initial data type are effectively creating history of values. At the same moment it is not deterministic, which value (and expected data type) will be returned by the nvs_get* operation after nvs_erase_key is called.
To Be state:
The nvs_set* operation using already existing key will delete current value regardless its data type and set the new value and new data type. To read the value back, user has to use nvs_get* variant matching most recently called nvs_set* function for given key.
For backward compatibility, we will introduce kconfig option enabling the "old" implementation.

We believe this compromise helps to reduce potential risks and reduces efforts required at users side when updating their applications.