From 64099bcbe7b024303ed14b01e9f6bad0c328c1be Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Fri, 9 Apr 2021 09:51:37 +0200 Subject: [PATCH] Add locking to UniqueRandomIdGenerator. The SdpOfferAnswerHandler::ssrc_generator_ variable is used from multiple threads. Adding thread checks + tests for UniqueNumberGenerator along the way. Bug: webrtc:12666 Change-Id: Id2973362a27fc1d2c7db60de2ea447d84d18ae3e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214702 Reviewed-by: Harald Alvestrand Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#33668} --- pc/sdp_offer_answer.h | 5 ++- rtc_base/unique_id_generator.cc | 3 ++ rtc_base/unique_id_generator.h | 23 ++++++++--- rtc_base/unique_id_generator_unittest.cc | 51 ++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 7 deletions(-) diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index e168d79859..d04c48401a 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -657,8 +657,9 @@ class SdpOfferAnswerHandler : public SdpStateProvider, // specified by the user (or by the remote party). // The generator is not used directly, instead it is passed on to the // channel manager and the session description factory. - rtc::UniqueRandomIdGenerator ssrc_generator_ - RTC_GUARDED_BY(signaling_thread()); + // TODO(bugs.webrtc.org/12666): This variable is used from both the signaling + // and worker threads. See if we can't restrict usage to a single thread. + rtc::UniqueRandomIdGenerator ssrc_generator_; // A video bitrate allocator factory. // This can be injected using the PeerConnectionDependencies, diff --git a/rtc_base/unique_id_generator.cc b/rtc_base/unique_id_generator.cc index d41fa8d186..9fa3021c6f 100644 --- a/rtc_base/unique_id_generator.cc +++ b/rtc_base/unique_id_generator.cc @@ -26,6 +26,8 @@ UniqueRandomIdGenerator::UniqueRandomIdGenerator(ArrayView known_ids) UniqueRandomIdGenerator::~UniqueRandomIdGenerator() = default; uint32_t UniqueRandomIdGenerator::GenerateId() { + webrtc::MutexLock lock(&mutex_); + RTC_CHECK_LT(known_ids_.size(), std::numeric_limits::max() - 1); while (true) { auto pair = known_ids_.insert(CreateRandomNonZeroId()); @@ -36,6 +38,7 @@ uint32_t UniqueRandomIdGenerator::GenerateId() { } bool UniqueRandomIdGenerator::AddKnownId(uint32_t value) { + webrtc::MutexLock lock(&mutex_); return known_ids_.insert(value).second; } diff --git a/rtc_base/unique_id_generator.h b/rtc_base/unique_id_generator.h index 836dc70b61..22398fd3f2 100644 --- a/rtc_base/unique_id_generator.h +++ b/rtc_base/unique_id_generator.h @@ -16,6 +16,9 @@ #include #include "api/array_view.h" +#include "api/sequence_checker.h" +#include "rtc_base/synchronization/mutex.h" +#include "rtc_base/system/no_unique_address.h" namespace rtc { @@ -47,9 +50,10 @@ class UniqueNumberGenerator { bool AddKnownId(TIntegral value); private: + RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker sequence_checker_; static_assert(std::is_integral::value, "Must be integral type."); - TIntegral counter_; - std::set known_ids_; + TIntegral counter_ RTC_GUARDED_BY(sequence_checker_); + std::set known_ids_ RTC_GUARDED_BY(sequence_checker_); }; // This class will generate unique ids. Ids are 32 bit unsigned integers. @@ -76,7 +80,10 @@ class UniqueRandomIdGenerator { bool AddKnownId(uint32_t value); private: - std::set known_ids_; + // TODO(bugs.webrtc.org/12666): This lock is needed due to an instance in + // SdpOfferAnswerHandler being shared between threads. + webrtc::Mutex mutex_; + std::set known_ids_ RTC_GUARDED_BY(&mutex_); }; // This class will generate strings. A common use case is for identifiers. @@ -104,18 +111,23 @@ class UniqueStringGenerator { }; template -UniqueNumberGenerator::UniqueNumberGenerator() : counter_(0) {} +UniqueNumberGenerator::UniqueNumberGenerator() : counter_(0) { + sequence_checker_.Detach(); +} template UniqueNumberGenerator::UniqueNumberGenerator( ArrayView known_ids) - : counter_(0), known_ids_(known_ids.begin(), known_ids.end()) {} + : counter_(0), known_ids_(known_ids.begin(), known_ids.end()) { + sequence_checker_.Detach(); +} template UniqueNumberGenerator::~UniqueNumberGenerator() {} template TIntegral UniqueNumberGenerator::GenerateNumber() { + RTC_DCHECK_RUN_ON(&sequence_checker_); while (true) { RTC_CHECK_LT(counter_, std::numeric_limits::max()); auto pair = known_ids_.insert(counter_++); @@ -127,6 +139,7 @@ TIntegral UniqueNumberGenerator::GenerateNumber() { template bool UniqueNumberGenerator::AddKnownId(TIntegral value) { + RTC_DCHECK_RUN_ON(&sequence_checker_); return known_ids_.insert(value).second; } } // namespace rtc diff --git a/rtc_base/unique_id_generator_unittest.cc b/rtc_base/unique_id_generator_unittest.cc index 868b348b11..835a57e162 100644 --- a/rtc_base/unique_id_generator_unittest.cc +++ b/rtc_base/unique_id_generator_unittest.cc @@ -15,6 +15,7 @@ #include "absl/algorithm/container.h" #include "api/array_view.h" +#include "api/task_queue/task_queue_base.h" #include "rtc_base/gunit.h" #include "rtc_base/helpers.h" #include "test/gmock.h" @@ -23,6 +24,21 @@ using ::testing::IsEmpty; using ::testing::Test; namespace rtc { +namespace { +// Utility class that registers itself as the currently active task queue. +class FakeTaskQueue : public webrtc::TaskQueueBase { + public: + FakeTaskQueue() : task_queue_setter_(this) {} + + void Delete() override {} + void PostTask(std::unique_ptr task) override {} + void PostDelayedTask(std::unique_ptr task, + uint32_t milliseconds) override {} + + private: + CurrentTaskQueueSetter task_queue_setter_; +}; +} // namespace template class UniqueIdGeneratorTest : public Test {}; @@ -148,4 +164,39 @@ TYPED_TEST(UniqueIdGeneratorTest, EXPECT_FALSE(generator2.AddKnownId(id)); } +// Tests that it's OK to construct the generator in one execution environment +// (thread/task queue) but use it in another. +TEST(UniqueNumberGenerator, UsedOnSecondaryThread) { + const auto* current_tq = webrtc::TaskQueueBase::Current(); + // Construct the generator before `fake_task_queue` to ensure that it is + // constructed in a different execution environment than what + // `fake_task_queue` will represent. + UniqueNumberGenerator generator; + + FakeTaskQueue fake_task_queue; + // Sanity check to make sure we're in a different runtime environment. + ASSERT_NE(current_tq, webrtc::TaskQueueBase::Current()); + + // Generating an id should be fine in this context. + generator.GenerateNumber(); +} + +#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) +TEST(UniqueNumberGeneratorDeathTest, FailsWhenUsedInWrongContext) { + // Instantiate the generator before the `loop`. This ensures that + // thread/sequence checkers will pick up a different thread environment than + // `fake_task_queue` will represent. + UniqueNumberGenerator generator; + // Generate an ID on the current thread. This causes the generator to attach + // to the current thread context. + generator.GenerateNumber(); + + // Instantiate a fake task queue that will register itself as the current tq. + FakeTaskQueue fake_task_queue; + + // Attempting to generate an id should now trigger a dcheck. + EXPECT_DEATH(generator.GenerateNumber(), ""); +} +#endif + } // namespace rtc