From 679b682da459940244678f1c9319241224093c38 Mon Sep 17 00:00:00 2001 From: h2zero Date: Sat, 17 Oct 2026 13:57:23 -0600 Subject: [PATCH] Add a mutex to NimBLEAttValue --- src/NimBLEAttValue.cpp | 17 +++---- src/NimBLEAttValue.h | 90 +++++++++++++++++++++++++++------ src/NimBLEClient.cpp | 17 ++++--- src/NimBLEDevice.cpp | 11 +++- src/NimBLELocalValueAttribute.h | 4 +- src/NimBLEServer.cpp | 3 +- 6 files changed, 105 insertions(+), 37 deletions(-) diff --git a/src/NimBLEAttValue.cpp b/src/NimBLEAttValue.cpp index be769595..99faeff0 100644 --- a/src/NimBLEAttValue.cpp +++ b/src/NimBLEAttValue.cpp @@ -18,16 +18,12 @@ #include "NimBLEAttValue.h" #if CONFIG_BT_NIMBLE_ENABLED -# ifdef USING_NIMBLE_ARDUINO_HEADERS -# include "nimble/nimble/include/nimble/nimble_npl.h" -# else -# include "nimble/nimble_npl.h" -# endif - # include "NimBLEUtils.h" # include "NimBLELog.h" static const char* LOG_TAG = "NimBLEAttValue"; +ble_npl_mutex NimBLEAttValue::Lock::m_mutex; +bool NimBLEAttValue::Lock::m_initialized = false; // Default constructor implementation. NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len) @@ -64,6 +60,7 @@ NimBLEAttValue::~NimBLEAttValue() { // Move assignment operator implementation. NimBLEAttValue& NimBLEAttValue::operator=(NimBLEAttValue&& source) { if (this != &source) { + Lock lock; free(m_attr_value); m_attr_value = source.m_attr_value; m_attr_max_len = source.m_attr_max_len; @@ -86,6 +83,7 @@ NimBLEAttValue& NimBLEAttValue::operator=(const NimBLEAttValue& source) { // Copy all the data from the source object to this object, including allocated space. void NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { + Lock lock; uint8_t* res = static_cast(realloc(m_attr_value, source.m_capacity + 1)); NIMBLE_CPP_DEBUG_ASSERT(res); if (res == nullptr) { @@ -93,18 +91,17 @@ void NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { return; } - ble_npl_hw_enter_critical(); m_attr_value = res; m_attr_max_len = source.m_attr_max_len; m_attr_len = source.m_attr_len; m_capacity = source.m_capacity; setTimeStamp(source.getTimeStamp()); memcpy(m_attr_value, source.m_attr_value, m_attr_len + 1); - ble_npl_hw_exit_critical(0); } // Set the value of the attribute. bool NimBLEAttValue::setValue(const uint8_t* value, uint16_t len) { + Lock lock; m_attr_len = 0; // Just set the value length to 0 and append instead of repeating code. m_attr_value[0] = '\0'; // Set the first byte to 0 incase the len of the new value is 0. append(value, len); @@ -113,6 +110,7 @@ bool NimBLEAttValue::setValue(const uint8_t* value, uint16_t len) { // Append the new data, allocate as necessary. NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) { + Lock lock; if (len == 0) { return *this; } @@ -144,18 +142,17 @@ NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) { time_t t = 0; # endif - ble_npl_hw_enter_critical(); memcpy(res + m_attr_len, value, len); m_attr_value = res; m_attr_len = new_len; m_attr_value[m_attr_len] = '\0'; setTimeStamp(t); - ble_npl_hw_exit_critical(0); return *this; } uint8_t NimBLEAttValue::operator[](int pos) const { + Lock lock; NIMBLE_CPP_DEBUG_ASSERT(pos < m_attr_len); if (pos >= m_attr_len) { NIMBLE_LOGE(LOG_TAG, "pos >= len, pos=%u, len=%u", pos, m_attr_len); diff --git a/src/NimBLEAttValue.h b/src/NimBLEAttValue.h index 9ef92d52..9dcf53e5 100644 --- a/src/NimBLEAttValue.h +++ b/src/NimBLEAttValue.h @@ -21,6 +21,12 @@ #include "syscfg/syscfg.h" #if CONFIG_BT_NIMBLE_ENABLED +# ifdef USING_NIMBLE_ARDUINO_HEADERS +# include "nimble/nimble/include/nimble/nimble_npl.h" +# else +# include "nimble/nimble_npl.h" +# endif + /* Enables the use of Arduino String class for attribute values */ # ifndef NIMBLE_CPP_ARDUINO_STRING_AVAILABLE # define NIMBLE_CPP_ARDUINO_STRING_AVAILABLE (__has_include()) @@ -103,6 +109,39 @@ class NimBLEAttValue { void deepCopy(const NimBLEAttValue& source); public: + class Lock { + static ble_npl_mutex m_mutex; + static bool m_initialized; + bool m_locked{false}; + + public: + Lock() { + if (m_initialized) { + m_locked = ble_npl_mutex_pend(&m_mutex, BLE_NPL_TIME_FOREVER) == BLE_NPL_OK; + } + } + + ~Lock() { + if (m_initialized && m_locked) { + ble_npl_mutex_release(&m_mutex); + } + } + + static bool init() { + if (!m_initialized) { + m_initialized = ble_npl_mutex_init(&m_mutex) == BLE_NPL_OK; + } + return m_initialized; + } + + static void deinit() { + if (m_initialized) { + m_initialized = false; + ble_npl_mutex_deinit(&m_mutex); + } + } + }; + /** * @brief Default constructor. * @param[in] init_len The initial size in bytes. @@ -181,16 +220,22 @@ class NimBLEAttValue { /** @brief Returns the current size of the value in bytes */ uint16_t size() const { return m_attr_len; } - /** @brief Returns a pointer to the internal buffer of the value */ + /** + * @brief Returns a pointer to the internal buffer of the value + * @note The pointer is not protected by the lock and can be invalidated by a concurrent update. + * Only use the pointer getters (`data()`, `c_str()`, `begin()`, `end()`) on a copy of the value. + */ const uint8_t* data() const { return m_attr_value; } - /** @brief Returns a pointer to the internal buffer of the value as a const char* */ + /** @brief Returns a pointer to the internal buffer of the value as a const char* + * @note See data() for thread safety restrictions. + */ const char* c_str() const { return reinterpret_cast(m_attr_value); } - /** @brief Iterator begin */ + /** @brief Iterator begin; see data() for thread safety restrictions. */ const uint8_t* begin() const { return m_attr_value; } - /** @brief Iterator end */ + /** @brief Iterator end; see data() for thread safety restrictions. */ const uint8_t* end() const { return m_attr_value + m_attr_len; } # if MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED) @@ -262,9 +307,7 @@ class NimBLEAttValue { # ifdef _DOXYGEN_ bool # else - typename std::enable_if::value && - std::is_same::type, char>::value, - bool>::type + typename std::enable_if::value && std::is_same::type, char>::value, bool>::type # endif setValue(const T& s) { return setValue(reinterpret_cast(s), strnlen(s, sizeof(T))); @@ -281,8 +324,7 @@ class NimBLEAttValue { bool # else typename std::enable_if::value && !Has_c_str_length::value && !Has_data_size::value && - !(std::is_array::value && - std::is_same::type, char>::value), + !(std::is_array::value && std::is_same::type, char>::value), bool>::type # endif setValue(const T& v) { @@ -354,8 +396,7 @@ class NimBLEAttValue { } } else if constexpr (Has_c_str_length::value) { return setValue(reinterpret_cast(s.c_str()), s.length()); - } else if constexpr (std::is_array::value && - std::is_same::type, char>::value) { + } else if constexpr (std::is_array::value && std::is_same::type, char>::value) { return setValue(reinterpret_cast(s), strnlen(s, sizeof(s))); } else { return setValue(reinterpret_cast(&s), sizeof(s)); @@ -375,6 +416,7 @@ class NimBLEAttValue { */ template T getValue(time_t* timestamp = nullptr, bool skipSizeCheck = false) const { + Lock lock; if (timestamp != nullptr) { # if MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED) *timestamp = m_timestamp; @@ -395,16 +437,30 @@ class NimBLEAttValue { uint8_t operator[](int pos) const; /** @brief Operator; Get the value as a std::vector. */ - operator std::vector() const { return std::vector(m_attr_value, m_attr_value + m_attr_len); } + operator std::vector() const { + Lock lock; + return std::vector(m_attr_value, m_attr_value + m_attr_len); + } /** @brief Operator; Get the value as a std::string. */ - operator std::string() const { return std::string(reinterpret_cast(m_attr_value), m_attr_len); } + operator std::string() const { + Lock lock; + return std::string(reinterpret_cast(m_attr_value), m_attr_len); + } /** @brief Operator; Get the value as a const uint8_t*. */ operator const uint8_t*() const { return m_attr_value; } /** @brief Operator; Append another NimBLEAttValue. */ - NimBLEAttValue& operator+=(const NimBLEAttValue& source) { return append(source.data(), source.size()); } + NimBLEAttValue& operator+=(const NimBLEAttValue& source) { + Lock lock; // Recursive; keeps the source data and size consistent while appending + if (&source == this) { + // append() may reallocate the buffer, so append from a copy of our own data + std::vector copy(m_attr_value, m_attr_value + m_attr_len); + return append(copy.data(), copy.size()); + } + return append(source.data(), source.size()); + } /** @brief Operator; Set the value from a std::string source. */ NimBLEAttValue& operator=(const std::string& source) { @@ -420,6 +476,7 @@ class NimBLEAttValue { /** @brief Equality operator */ bool operator==(const NimBLEAttValue& source) const { + Lock lock; return (m_attr_len == source.size()) ? memcmp(m_attr_value, source.data(), m_attr_len) == 0 : false; } @@ -428,7 +485,10 @@ class NimBLEAttValue { # if NIMBLE_CPP_ARDUINO_STRING_AVAILABLE /** @brief Operator; Get the value as an Arduino String value. */ - operator String() const { return String(reinterpret_cast(m_attr_value)); } + operator String() const { + Lock lock; + return String(reinterpret_cast(m_attr_value)); + } # endif }; diff --git a/src/NimBLEClient.cpp b/src/NimBLEClient.cpp index 2d219a73..14afad0e 100644 --- a/src/NimBLEClient.cpp +++ b/src/NimBLEClient.cpp @@ -1179,13 +1179,16 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) { return BLE_ATT_ERR_INVALID_HANDLE; } - auto len = event->notify_rx.om->om_len; - if (pChr->m_value.setValue(event->notify_rx.om->om_data, len)) { + // Build the value locally so the update of the characteristic value is atomic + // and the callback gets a stable copy without holding the value lock. + NimBLEAttValue value(MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_INIT_LENGTH), pChr->m_value.max_size()); + auto len = event->notify_rx.om->om_len; + if (value.setValue(event->notify_rx.om->om_data, len)) { os_mbuf* next; next = SLIST_NEXT(event->notify_rx.om, om_next); while (next != NULL) { - pChr->m_value.append(next->om_data, next->om_len); - if (pChr->m_value.length() != len + next->om_len) { + value.append(next->om_data, next->om_len); + if (value.length() != len + next->om_len) { rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; break; } @@ -1201,11 +1204,13 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) { return rc; } + pChr->m_value = value; + if (pChr->m_notifyCallback != nullptr) { // TODO: change this callback to use the NimBLEAttValue class instead of raw data and length pChr->m_notifyCallback(pChr, - const_cast(pChr->m_value.getValue().data()), - pChr->m_value.length(), + const_cast(value.data()), + value.length(), !event->notify_rx.indication); } diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index e125aeb9..585aaebd 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -833,7 +833,7 @@ void NimBLEDevice::onReset(int reason) { * @brief Host synced with controller, all clear to make calls to the stack. */ void NimBLEDevice::onSync(void) { - NIMBLE_LOGI(LOG_TAG, "NimBle host synced."); + NIMBLE_LOGI(LOG_TAG, "NimBLE host synced."); // This check is needed due to potentially being called multiple times in succession // If this happens, the call to scan start may get stuck or cause an advertising fault. if (m_synced) { @@ -894,6 +894,12 @@ void NimBLEDevice::host_task(void* param) { bool NimBLEDevice::init(const std::string& deviceName) { if (!m_initialized) { NIMBLE_LOGD(LOG_TAG, "Starting %s", getVersion()); + + if (!NimBLEAttValue::Lock::init()) { + NIMBLE_LOGE(LOG_TAG, "Failed to initialize NimBLEAttValue lock"); + return false; + } + # ifdef ESP_PLATFORM # if defined(CONFIG_ENABLE_ARDUINO_DEPENDS) && SOC_BT_SUPPORTED @@ -1015,7 +1021,7 @@ bool NimBLEDevice::init(const std::string& deviceName) { } m_initialized = true; // Set the initialization flag to ensure we are only initialized once. - NIMBLE_LOGD(LOG_TAG, "Initialized"); + NIMBLE_LOGD(LOG_TAG, "Initialized BLE stack"); return true; } // init @@ -1088,6 +1094,7 @@ bool NimBLEDevice::deinit(bool clearAll) { # endif NimBLEUtils::deleteTaskSems(); + NimBLEAttValue::Lock::deinit(); } return rc == 0; diff --git a/src/NimBLELocalValueAttribute.h b/src/NimBLELocalValueAttribute.h index 5fd37e55..0c4f3909 100644 --- a/src/NimBLELocalValueAttribute.h +++ b/src/NimBLELocalValueAttribute.h @@ -125,8 +125,8 @@ class NimBLELocalValueAttribute : public NimBLELocalAttribute, public NimBLEValu virtual void writeEvent(const uint8_t* val, uint16_t len, NimBLEConnInfo& connInfo) = 0; /** - * @brief Get a pointer to value of the attribute. - * @return A pointer to the value of the attribute. + * @brief Get a reference to value of the attribute. + * @return A reference to the value of the attribute. * @details This function is used by NimBLEServer when handling read/write requests. */ const NimBLEAttValue& getAttVal() const { return m_value; } diff --git a/src/NimBLEServer.cpp b/src/NimBLEServer.cpp index 660a0313..3129d113 100644 --- a/src/NimBLEServer.cpp +++ b/src/NimBLEServer.cpp @@ -760,9 +760,8 @@ int NimBLEServer::handleGattEvent(uint16_t connHandle, uint16_t attrHandle, ble_ pAtt->readEvent(peerInfo); } - ble_npl_hw_enter_critical(); + NimBLEAttValue::Lock lock; int rc = os_mbuf_append(ctxt->om, val.data(), val.size()); - ble_npl_hw_exit_critical(0); return rc == 0 ? 0 : BLE_ATT_ERR_INSUFFICIENT_RES; }