This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.2.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 0889d49968ad87118f461167715735e830cf165e Author: Masaori Koshiba <[email protected]> AuthorDate: Fri Jun 26 07:52:16 2026 +0900 Annotate BRAVO locks for thread-safety analysis (#13330) * Annotate BRAVO locks for thread-safety analysis Enabling -Wthread-safety tree-wide surfaced findings in Bravo.h on macOS, where libc++ marks the underlying std::shared_mutex as a capability: the wrapper acquires and releases it across method boundaries and on fast/slow-path branches the analyzer cannot follow. Fedora CI uses libstdc++, which does not annotate std types, so it never saw these. Make ts::bravo::shared_mutex_impl a real capability so the contract is checked on every Clang, not papered over on one std lib; its lock-driving bodies are the trusted implementation and stay exempted, matching the ts::shared_mutex pattern. Make the reader guard a rigid scoped capability -- acquire in constructor, release in destructor, no copy/move/defer/release -- which the analysis tracks with no exemption; the movable std::shared_lock-style forms it replaced could not be tracked, and no caller used them. Restructure the unit test's try-lock asserts so the acquire gates a branch. * Reset BRAVO Token on entry to harden slow-path unlock lock_shared/try_lock_shared assigned the Token only on the fast path, so a reused non-zero Token surviving into a slow-path acquisition would make unlock_shared release a reader slot instead of the underlying mutex. Clear it on entry to enforce the documented 0-init contract. (cherry picked from commit 4fabae0c7299a45f4d448e9a8e6de0917b2416c9) --- include/tsutil/Bravo.h | 140 +++++++++++------------------------- src/tsutil/unit_tests/test_Bravo.cc | 35 ++++++--- 2 files changed, 65 insertions(+), 110 deletions(-) diff --git a/include/tsutil/Bravo.h b/include/tsutil/Bravo.h index 4660f69aca..1dc1043d68 100644 --- a/include/tsutil/Bravo.h +++ b/include/tsutil/Bravo.h @@ -39,6 +39,7 @@ #include "tsutil/DenseThreadId.h" #include "tsutil/Assert.h" +#include "tsutil/ts_thread_safety.h" #include <array> #include <atomic> @@ -68,129 +69,54 @@ using Token = size_t; /** ts::bravo::shared_lock + + Reader guard for shared_mutex_impl, carrying the BRAVO Token. A rigid scoped + capability: it acquires the shared lock in its constructor and releases it in + its destructor, with no copy, move, defer or release. That rigidity is what + lets the analysis track it -- the deferred and movable forms of + std::shared_lock let the held state escape a single scope and cannot be + modelled -- and BRAVO readers need only this scoped form. */ -template <class Mutex> class shared_lock +template <class Mutex> class TS_SCOPED_CAPABILITY shared_lock { public: using mutex_type = Mutex; - shared_lock() noexcept = default; - shared_lock(Mutex &m) : _mutex(&m) { lock(); } - shared_lock(Mutex &m, std::try_to_lock_t) : _mutex(&m) { try_lock(); } - shared_lock(Mutex &m, std::defer_lock_t) noexcept : _mutex(&m) {} - - ~shared_lock() - { - if (_owns) { - _mutex->unlock_shared(_token); - } - }; + explicit shared_lock(Mutex &m) TS_ACQUIRE_SHARED(m) : _mutex(&m) { _mutex->lock_shared(_token); } + ~shared_lock() TS_RELEASE() { _mutex->unlock_shared(_token); } //// - // Not Copyable + // Neither copyable nor movable: the held shared lock must not escape this scope. // shared_lock(shared_lock const &) = delete; shared_lock &operator=(shared_lock const &) = delete; - - //// - // Moveable - // - shared_lock(shared_lock &&s) : _mutex(s._mutex), _token(s._token), _owns(s._owns) - { - s._mutex = nullptr; - s._token = 0; - s._owns = false; - }; - - shared_lock & - operator=(shared_lock &&s) - { - if (_owns) { - _mutex->unlock_shared(_token); - } - _mutex = s._mutex; - _token = s._token; - _owns = s._owns; - - s._mutex = nullptr; - s._token = 0; - s._owns = false; - }; - - //// - // Shared locking - // - void - lock() - { - _mutex->lock_shared(_token); - _owns = true; - } - - bool - try_lock() - { - _owns = _mutex->try_lock_shared(_token); - return _owns; - } - - // not implemented yet - bool try_lock_for() = delete; - bool try_lock_until() = delete; - - void - unlock() - { - _mutex->unlock_shared(_token); - _owns = false; - } - - //// - // Modifiers - // - void - swap(shared_lock &s) - { - std::swap(_mutex, s._mutex); - std::swap(_token, s._token); - std::swap(_owns, s._owns); - } - - mutex_type * - release() - { - mutex_type *m = _mutex; - _mutex = nullptr; - _token = 0; - _owns = false; - return m; - } + shared_lock(shared_lock &&) = delete; + shared_lock &operator=(shared_lock &&) = delete; //// // Observers // mutex_type * - mutex() + mutex() const { return _mutex; } Token - token() + token() const { return _token; } bool - owns_lock() + owns_lock() const { - return _owns; + return _mutex != nullptr; } private: mutex_type *_mutex = nullptr; Token _token = 0; - bool _owns = false; }; /** @@ -201,7 +127,8 @@ private: Set the SLOT_SIZE larger than DenseThreadId::num_possible_values to go fast-path. */ -template <typename T = std::shared_mutex, size_t SLOT_SIZE = 256, int SLOWDOWN_GUARD = 7> class shared_mutex_impl +template <typename T = std::shared_mutex, size_t SLOT_SIZE = 256, int SLOWDOWN_GUARD = 7> +class TS_CAPABILITY("shared_mutex") shared_mutex_impl { public: shared_mutex_impl() = default; @@ -219,15 +146,22 @@ public: //// // Exclusive locking // + // These lock/unlock methods are the trusted implementation of this capability: + // their bodies drive the underlying lock (T, by default std::shared_mutex, + // which libc++ annotates as a capability) across method boundaries and along + // the BRAVO fast/slow-path branches -- patterns the analysis cannot follow. + // Exempt the bodies so only the capability contract on each signature is + // checked; the data-race checking happens at the call sites. + // void - lock() + lock() TS_ACQUIRE() TS_NO_THREAD_SAFETY_ANALYSIS { _mutex.underlying.lock(); _revoke(); } bool - try_lock() + try_lock() TS_TRY_ACQUIRE(true) TS_NO_THREAD_SAFETY_ANALYSIS { bool r = _mutex.underlying.try_lock(); if (!r) { @@ -240,19 +174,22 @@ public: } void - unlock() + unlock() TS_RELEASE() TS_NO_THREAD_SAFETY_ANALYSIS { _mutex.underlying.unlock(); } //// - // Shared locking + // Shared locking (bodies exempted for the same reason as the exclusive ones above) // void - lock_shared(Token &token) + lock_shared(Token &token) TS_ACQUIRE_SHARED() TS_NO_THREAD_SAFETY_ANALYSIS { debug_assert(SLOT_SIZE >= DenseThreadId::num_possible_values()); + // Clear up front so a slow-path acquisition never leaves a stale fast-path slot for unlock_shared(). + token = 0; + // Fast path if (_mutex.read_bias.load(std::memory_order_acquire)) { size_t index = DenseThreadId::self() % SLOT_SIZE; @@ -277,10 +214,13 @@ public: } bool - try_lock_shared(Token &token) + try_lock_shared(Token &token) TS_TRY_ACQUIRE_SHARED(true) TS_NO_THREAD_SAFETY_ANALYSIS { debug_assert(SLOT_SIZE >= DenseThreadId::num_possible_values()); + // Clear up front so a slow-path acquisition never leaves a stale fast-path slot for unlock_shared(). + token = 0; + // Fast path if (_mutex.read_bias.load(std::memory_order_acquire)) { size_t index = DenseThreadId::self() % SLOT_SIZE; @@ -313,7 +253,7 @@ public: } void - unlock_shared(const Token token) + unlock_shared(const Token token) TS_RELEASE_SHARED() TS_NO_THREAD_SAFETY_ANALYSIS { if (token == 0) { _mutex.underlying.unlock_shared(); diff --git a/src/tsutil/unit_tests/test_Bravo.cc b/src/tsutil/unit_tests/test_Bravo.cc index 1a825cd946..666ae4f5cc 100644 --- a/src/tsutil/unit_tests/test_Bravo.cc +++ b/src/tsutil/unit_tests/test_Bravo.cc @@ -41,8 +41,11 @@ TEST_CASE("BRAVO - simple check", "[libts][BRAVO]") std::thread t{[](ts::bravo::shared_mutex &mutex) { ts::bravo::Token token{0}; - CHECK(mutex.try_lock_shared(token) == true); - mutex.unlock_shared(token); + bool locked = mutex.try_lock_shared(token); + CHECK(locked == true); + if (locked) { + mutex.unlock_shared(token); + } }, std::ref(mutex)}; @@ -95,28 +98,40 @@ TEST_CASE("BRAVO - multiple try-lock", "[libts][BRAVO]") { ts::bravo::Token token{0}; - CHECK(mutex.try_lock_shared(token)); + bool locked = mutex.try_lock_shared(token); + CHECK(locked); CHECK(i == 0); - mutex.unlock_shared(token); + if (locked) { + mutex.unlock_shared(token); + } } { - CHECK(mutex.try_lock()); + bool locked = mutex.try_lock(); + CHECK(locked); CHECK(++i == 1); - mutex.unlock(); + if (locked) { + mutex.unlock(); + } } { ts::bravo::Token token{0}; - CHECK(mutex.try_lock_shared(token)); + bool locked = mutex.try_lock_shared(token); + CHECK(locked); CHECK(i == 1); - mutex.unlock_shared(token); + if (locked) { + mutex.unlock_shared(token); + } } { - CHECK(mutex.try_lock()); + bool locked = mutex.try_lock(); + CHECK(locked); CHECK(++i == 2); - mutex.unlock(); + if (locked) { + mutex.unlock(); + } } CHECK(i == 2);
