TDBStore fails to unlock mutex on set() failure

[thendrickson]

Description

I'm using kv_global_api to set a key value. My SPI flash has some error which is causing the read_record() to return MBED_ERROR_INVALID_DATA_DETECTED. When this error occurs, TDBStore does not call _mutext.unlock() and the mutex is never released from the current thread.

I've profiled the code and have some functions that get called during this error.

[ 3588][DBG ][TDB ]: _mutex.lock() from set_start
[ 3593][DBG ][TDB ]: write_area
[ 3599][DBG ][TDB ]: set_add_data
[ 3601][DBG ][TDB ]: write_area
[ 3607][DBG ][TDB ]: set_add_data
[ 3609][DBG ][TDB ]: write_area
[ 3614][DBG ][TDB ]: set_add_data
[ 3616][DBG ][TDB ]: write_area
[ 3622][DBG ][TDB ]: set_finalize
[ 3624][DBG ][TDB ]: write_area
[ 3631][DBG ][TDB ]: read_record
[ 3632][DBG ][TDB ]: read_area
[ 3637][DBG ][TDB ]: read_record : INVALID_DATA_DETECTED
[ 3644][DBG ][TDB ]: read_area
[ 3646][DBG ][TDB ]: read_area
[ 3652][DBG ][TDB ]: set
[ 3653][DBG ][TDB ]: set_start
[ 3657][DBG ][TDB ]: master record
[ 3661][DBG ][TDB ]: write_area
[ 3667][DBG ][TDB ]: set_add_data
[ 3669][DBG ][TDB ]: write_area
[ 3675][DBG ][TDB ]: set_finalize
[ 3677][DBG ][TDB ]: write_area
[ 3683][DBG ][TDB ]: in set_finalize still: ih->bd_base_offset 72, _master_record_offset, 72 (so _mutex.unlock() doesn't get called)
[ 3690][DBG ][TDB ]: in set_finalize still: ih->bd_base_offset 72, _master_record_offset, 72 (so _mutex.unlock() doesn't get called)
[ 3696][ERR ][NVM ]: /kv/myKeyName: INVALID_DATA_DETECTED

It appears that one of the calls to set_finalize() should have called _mutex.unlock() but didn't because it detected it to be the master record because of the recursive call to set_finalize()

My target is NRF52840
Toolchain is GCC_ARM
Compiling with mbed-cli 1.10.1
mbed-os version 5.14.0 (SHA b6e5a0a)

To reproduce the logic:
In the first call to set_finalize(), read_record() then read_area() will succeed, and then force the debugger to evaluate this condition as true (header.magic != tdbstore_magic) so that the function returns MBED_ERROR_INVALID_DATA_DETECTED.

Issue request type

[ ] Question
[ ] Enhancement
[x] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-1939

[0xc0170]

cc @ARMmbed/mbed-os-storage

[SeppoTakalo]

The code flow for this error is clear:

User -> TDBStore::set(key, value)
  TDBStore::set(key, value) -> TDBStore::set_start()
  TDBStore::set(key, value) -> TDBStore::set_add_data()
  TDBStore::set(key, value) -x TDBStore::finalize()   // This fails, so it sets needs_gc=true
    TDBStore::finalize() -> TDBStore::garbage_colllect()
    TDBStore::garbage_colllect() -> TDBStore::write_master_key
    TDBStore::write_master_key -> TDBStore::set() // This re-uses the set-handle
    TDBStore::set() -> set_start(),set_add_data()
    TDBStore::set() -> finalize() // Now we ARE actually writing the mater key, so no mutex will be released
    return 0
  return failure

Broken code:

    if ((need_gc) && (ih->bd_base_offset != _master_record_offset)) {
        garbage_collection();       // <---- THIS uses the same *ih handle
    }
    ...
    if (ih->bd_base_offset != _master_record_offset) {  // <---- now, the *ih handle WAS modifed to be master record
        _mutex.unlock();
    }