Azure / Azure/azure-sdk-for-cpp

Lock the sender map read in ProducerClient::GetSender

Open
#7,285 0 comments 0 reactions 1 assignee Claimed by @j7nw4r View on GitHub
Event Hubs
Dominant language
C++
Stars
205
Forks
172
Avg merge
1d 3h
Merged PRs (30d)
37

Description

## Summary

`ProducerClient::GetSender` reads the sender map with no lock. `EnsureSender` changes the same map under `m_sendersLock`. Two threads that send to different partitions for the first time cause a data race. One thread reads a `std::map` while the other thread writes it. This is undefined behavior. `Send` and `CreateBatch` both use this path.

Parent: #7284. Severity tags follow the key in #7252. The code anchors refer to commit `2de7c583e`.

## Motivation

`GetSender` has three lines and takes no lock (`src/producer_client.cpp:259-263`):

```cpp
Azure::Core::Amqp::_internal::MessageSender ProducerClient::GetSender(
std::string const& partitionId)
{
return m_senders.at(partitionId);
}
```

`EnsureSender` writes to the same map under `m_sendersLock` (`src/producer_client.cpp:229-257`). A write can rebalance the red-black tree that holds a `std::map`. A parallel `at()` call then reads that tree while it moves.

`ProducerClient::Send` calls `GetSender` in its retry lambda (`src/producer_client.cpp:111`). `CreateBatch` calls it at `src/producer_client.cpp:90`. Both callers call `EnsureSender` first, so one thread alone is safe. Two threads with different partitions are not safe. Thread A can write the sender for partition 1 while thread B reads the sender for partition 0.

`GetSession` takes `m_sessionsLock` for the same pattern. The missing lock in `GetSender` therefore looks like an error.

## Proposal

- [ ] Take `m_sendersLock` in `ProducerClient::GetSender` before it reads `m_senders` (`src/producer_client.cpp:259-263`). **[GA blocker]**
- [ ] Examine `ConsumerClient` for the same pattern on `m_receivers` and `m_receiversLock` (`inc/azure/messaging/eventhubs/consumer_client.hpp:206`). **[GA blocker]**

This is the smallest correct change. It keeps all the signatures. The method releases the lock before the caller uses the `MessageSender` handle. The method returns the handle by value, so no reference stays alive after the lock. #7288 replaces this mutex with a `std::shared_mutex` later. Do this change first, because it is small and it stops the undefined behavior now.

## Validation

- [ ] The thread sanitizer reports no race. Use N threads. Each thread sends to a different partition that no thread used before.
- [ ] The same test causes a thread sanitizer error before the change.
- [ ] The Event Hubs unit tests pass.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.