Data race: Store::get_map_internal reads maps without maps_lock via get_map_unsafe

[achamayou]

Describe the bug

ccf::kv::Store::get_map_internal reads the maps container without holding maps_lock, when reached via get_map_unsafe() from a caller that does not hold that lock. ThreadSanitizer reports it as a data race.

src/kv/store.h:

ccf::pal::Mutex maps_lock;
Maps maps;
...
std::shared_ptr<AbstractMap> get_map(ccf::kv::Version v, const std::string& map_name)
{
  std::lock_guard<ccf::pal::Mutex> mguard(maps_lock);   // locked
  return get_map_internal(v, map_name);
}

std::shared_ptr<AbstractMap> get_map_unsafe(
  ccf::kv::Version v, const std::string& map_name) override
{
  return get_map_internal(v, map_name);                  // caller must hold maps_lock
}

The _unsafe suffix, and the lock_map_set() / unlock_map_set() pair on AbstractStore, make the contract clear: callers of get_map_unsafe are required to hold maps_lock. At least one caller does not.

The likely path is apply_changes() in src/kv/apply_changes.h, which calls store->get_map_unsafe(current_v, map_name) in its new-map conflict check. Its caller CommittableTx::commit() only takes the lock when the transaction itself created maps:

const bool maps_created = !pimpl->created_maps.empty();
if (maps_created)
{
  this->pimpl->store->lock_map_set();
}

so a transaction that creates no new maps reaches the read with no lock held, while a concurrent transaction that does create maps writes to maps under maps_lock.

To Reproduce

Intermittent, under the TSAN CI job. Observed on an unrelated PR (#8249, which changes only src/consensus/aft/):

WARNING: ThreadSanitizer: data race (pid=7742)
SUMMARY: ThreadSanitizer: data race CCF/build/CCF/src/kv/store.h:314:26 in
  ccf::kv::Store::get_map_internal(unsigned long, std::string const&)

reported four times in one run, failing e2e_logging_http2.

Expected behavior

Every caller of get_map_unsafe holds maps_lock, or the read is otherwise synchronised.

Additional context

Worth checking whether the conditional lock_map_set() is the intended design. If the conflict check genuinely needs to observe the map set, it arguably needs the lock regardless of whether this transaction creates maps, since the race is against other transactions creating them.

Clang thread-safety analysis is already enabled for PAL mutexes (#8180). Annotating maps with CCF_GUARDED_BY(maps_lock) and get_map_internal with CCF_REQUIRES(maps_lock) would surface this at compile time and prevent recurrence, though it may flag further call sites.

Found while reviewing CI on #8249; unrelated to that change.

[achamayou]

Correction to the diagnosis above, from the full TSAN stack traces.

I originally guessed the unsynchronised side was a reader reaching get_map_unsafe via apply_changes' new-map conflict check, because CommittableTx::commit() only takes the lock when the transaction creates maps. That was wrong, and the conditional in commit() is fine: the conflict check only runs when there are new maps, which is exactly when the lock is held.

The reader in the trace is correctly synchronised - it goes through Store::get_map(), which takes maps_lock:

Read of size 8 by thread T3 (mutexes: write M0):
  #7 ccf::kv::Store::get_map_internal   src/kv/store.h:314
  #8 ccf::kv::Store::get_map            src/kv/store.h:302

The unsynchronised side is the writer:

Previous write of size 8 by thread T4:
  #9 ccf::kv::Store::add_dynamic_map        src/kv/store.h:356
 #10 ccf::kv::apply_changes                 src/kv/apply_changes.h:131
 #11 ccf::kv::ReservedTx::commit_reserved() src/kv/committable_tx.h:461
 #14 ccf::HashedTxHistory::emit_signature()

ReservedTx::commit_reserved() calls apply_changes without the lock_map_set() / unlock_map_set() bracketing that CommittableTx::commit() has, so add_dynamic_map writes maps with no lock. A reserved transaction does create maps - the first signature creates the signature tables.

Fix in #8252, with a kv_test regression that reproduces the exact signature under TSAN without the fix and is clean with it.

On the thread-safety annotations suggested above: I tried CCF_GUARDED_BY(maps_lock) on maps. It produces around 100 errors, largely where the lock genuinely is held but the analysis cannot see it - through std::scoped_lock over two mutexes in compact(), and across the lock_map_set() / unlock_map_set() boundary, which spans function calls. Worth doing, but it is its own refactor rather than something to fold into the fix.