From ac69d195195c474ba85e7588ce6b00085bf180b3 Mon Sep 17 00:00:00 2001 From: h2zero Date: Fri, 30 Oct 2026 12:33:44 -0600 Subject: [PATCH 1/2] NimBLEAttValue: enforce max length in value constructors, add rvalue overloads * Added std::string&&, std::vector&& and Arduino String&& constructors. * Move assignment now leaves the source in a safe state: null buffer, with max length, length, capacity and timestamp cleared. * The move constructor still delegates to move assignment. Bug fixes: * The buffer constructor in NimBLEAttValue.cpp now rejects a length above max_len, which is capped at 512. It logs an error and leaves the value empty, as append() does. * Container sizes are clamped to 65535 before being narrowed to uint16_t. Previously a 65536+ byte container could wrap to a small length and pass the check. This applies to the const char*, initializer-list, std::string, std::vector, Arduino String and rvalue constructors. Behavior notes: * The rvalue overloads copy the bytes. They do not take over the source container's storage, and the source stays valid. * The by-value const std::string, const std::vector and const String parameters are now const&, which avoids copies and ambiguity with the rvalue overloads. Those constructors now use .data() instead of &x[0], so empty containers are safe. * A moved-from NimBLEAttValue has a null buffer. It is safe to destroy or assign to. --- src/NimBLEAttValue.cpp | 22 +++++++++++++------ src/NimBLEAttValue.h | 49 +++++++++++++++++++++++++++++++++++------- 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/src/NimBLEAttValue.cpp b/src/NimBLEAttValue.cpp index 9ccf7c21..4162154f 100644 --- a/src/NimBLEAttValue.cpp +++ b/src/NimBLEAttValue.cpp @@ -58,9 +58,9 @@ unsigned char __atomic_exchange_1(volatile void* ptr, unsigned char val, int) { # endif static const char* LOG_TAG = "NimBLEAttValue"; -ble_npl_mutex NimBLEAttValue::Lock::m_mutex{}; -std::atomic NimBLEAttValue::Lock::m_initialized{false}; -std::atomic NimBLEAttValue::Lock::m_activeCount{0}; +ble_npl_mutex NimBLEAttValue::Lock::m_mutex{}; +std::atomic NimBLEAttValue::Lock::m_initialized{false}; +std::atomic NimBLEAttValue::Lock::m_activeCount{0}; // Default constructor implementation. NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len) @@ -76,14 +76,21 @@ NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len) NIMBLE_CPP_DEBUG_ASSERT(m_attr_value); if (m_attr_value == nullptr) { NIMBLE_LOGE(LOG_TAG, "Failed to allocate value buffer"); - m_attr_len = 0; - m_capacity = 0; + m_attr_len = 0; + m_capacity = 0; m_attr_max_len = 0; } } // Value constructor implementation. -NimBLEAttValue::NimBLEAttValue(const uint8_t* value, uint16_t len, uint16_t max_len) : NimBLEAttValue(len, max_len) { +NimBLEAttValue::NimBLEAttValue(const uint8_t* value, uint16_t len, uint16_t max_len) + : NimBLEAttValue(len > std::min(max_len, BLE_ATT_ATTR_MAX_LEN) ? 0 : len, max_len) { + // Same behavior as append(): a value larger than max_len is rejected, leaving the value empty. + if (len > m_attr_max_len) { + NIMBLE_LOGE(LOG_TAG, "val > max, len=%u, max=%u", len, m_attr_max_len); + return; + } + if (m_attr_value != nullptr && len > 0) { memcpy(m_attr_value, value, len); m_attr_len = len; @@ -108,6 +115,9 @@ NimBLEAttValue& NimBLEAttValue::operator=(NimBLEAttValue&& source) { m_capacity = source.m_capacity; setTimeStamp(source.getTimeStamp()); source.m_attr_value = nullptr; + source.m_attr_len = 0; + source.m_capacity = 0; + source.setTimeStamp(0); } return *this; diff --git a/src/NimBLEAttValue.h b/src/NimBLEAttValue.h index 4850940a..cccf1553 100644 --- a/src/NimBLEAttValue.h +++ b/src/NimBLEAttValue.h @@ -108,6 +108,11 @@ class NimBLEAttValue { # endif bool deepCopy(const NimBLEAttValue& source); + // Clamp so a large container size can't wrap when narrowed to uint16_t; the constructor then rejects it as > max_len. + static constexpr uint16_t clampLen(size_t len) { + return len > UINT16_MAX ? UINT16_MAX : static_cast(len); + } + public: class Lock { static ble_npl_mutex m_mutex; @@ -191,7 +196,7 @@ class NimBLEAttValue { * @param[in] max_len The max size in bytes that the value can be. */ NimBLEAttValue(const char* value, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) - : NimBLEAttValue((uint8_t*)value, (uint16_t)strlen(value), max_len) {} + : NimBLEAttValue(reinterpret_cast(value), clampLen(strlen(value)), max_len) {} /** * @brief Construct with an initializer list. @@ -199,23 +204,23 @@ class NimBLEAttValue { * @param[in] max_len The max size in bytes that the value can be. */ NimBLEAttValue(std::initializer_list list, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) - : NimBLEAttValue(list.begin(), list.size(), max_len) {} + : NimBLEAttValue(list.begin(), clampLen(list.size()), max_len) {} /** * @brief Construct with an initial value from a std::string. * @param str A std::string containing to the initial value to set. * @param[in] max_len The max size in bytes that the value can be. */ - NimBLEAttValue(const std::string str, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) - : NimBLEAttValue(reinterpret_cast(&str[0]), str.length(), max_len) {} + NimBLEAttValue(const std::string& str, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) + : NimBLEAttValue(reinterpret_cast(str.data()), clampLen(str.length()), max_len) {} /** * @brief Construct with an initial value from a std::vector. * @param vec A std::vector containing to the initial value to set. * @param[in] max_len The max size in bytes that the value can be. */ - NimBLEAttValue(const std::vector vec, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) - : NimBLEAttValue(&vec[0], vec.size(), max_len) {} + NimBLEAttValue(const std::vector& vec, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) + : NimBLEAttValue(vec.data(), clampLen(vec.size()), max_len) {} # if NIMBLE_CPP_ARDUINO_STRING_AVAILABLE /** @@ -223,8 +228,16 @@ class NimBLEAttValue { * @param str An Arduino String containing to the initial value to set. * @param[in] max_len The max size in bytes that the value can be. */ - NimBLEAttValue(const String str, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) - : NimBLEAttValue(reinterpret_cast(str.c_str()), str.length(), max_len) {} + NimBLEAttValue(const String& str, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) + : NimBLEAttValue(reinterpret_cast(str.c_str()), clampLen(str.length()), max_len) {} + + /** + * @brief Construct with an initial value from an Arduino String rvalue. + * @param str An Arduino String containing to the initial value to set. + * @param[in] max_len The max size in bytes that the value can be. + */ + NimBLEAttValue(String&& str, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) + : NimBLEAttValue(reinterpret_cast(str.c_str()), clampLen(str.length()), max_len) {} # endif /** @brief Copy constructor */ @@ -233,6 +246,26 @@ class NimBLEAttValue { /** @brief Move constructor */ NimBLEAttValue(NimBLEAttValue&& source) { *this = std::move(source); } + /** + * @brief Construct with an initial value from a std::string rvalue. + * @param value A std::string containing the initial value to set. + * @param[in] max_len The max size in bytes that the value can be. + * @note The bytes are copied into this object's own buffer; the string's storage is not taken over. + * If the string is larger than max_len the value is left empty. + */ + NimBLEAttValue(std::string&& value, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) noexcept + : NimBLEAttValue(reinterpret_cast(value.data()), clampLen(value.size()), max_len) {} + + /** + * @brief Construct with an initial value from a std::vector rvalue. + * @param value A std::vector containing the initial value to set. + * @param[in] max_len The max size in bytes that the value can be. + * @note The bytes are copied into this object's own buffer; the vector's storage is not taken over. + * If the vector is larger than max_len the value is left empty. + */ + NimBLEAttValue(std::vector&& value, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN) noexcept + : NimBLEAttValue(value.data(), clampLen(value.size()), max_len) {} + /** @brief Destructor */ ~NimBLEAttValue(); From e2326cd677eb3e07f19c417045c15c152035f6b5 Mon Sep 17 00:00:00 2001 From: h2zero Date: Fri, 30 Oct 2026 13:49:31 -0600 Subject: [PATCH 2/2] Memory safety * EMPTY_VALUE: m_attr_value now always points to either a heap buffer the object owns or a shared static EMPTY_VALUE byte. It is used for a moved-from source and for allocation failure. * ownedBuffer(): a private helper returns the heap buffer, or nullptr when the pointer is EMPTY_VALUE. The destructor, move assignment, deepCopy() and append() use it, so free and realloc never see the static byte. *Pointer getters: data(), c_str(), begin() and end() never return null. *Moved-from state: after a move, the source has length 0, capacity 0 and timestamp 0 and points at EMPTY_VALUE. It keeps its max length, so it is safe to destroy, assign to or append to; it allocates on first append. --- src/NimBLEAttValue.cpp | 37 +++++++++++++++++++++---------------- src/NimBLEAttValue.h | 8 +++++++- src/NimBLEClient.cpp | 2 +- src/NimBLEServer.cpp | 2 +- 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/src/NimBLEAttValue.cpp b/src/NimBLEAttValue.cpp index 4162154f..e3bbdadf 100644 --- a/src/NimBLEAttValue.cpp +++ b/src/NimBLEAttValue.cpp @@ -61,10 +61,11 @@ static const char* LOG_TAG = "NimBLEAttValue"; ble_npl_mutex NimBLEAttValue::Lock::m_mutex{}; std::atomic NimBLEAttValue::Lock::m_initialized{false}; std::atomic NimBLEAttValue::Lock::m_activeCount{0}; +uint8_t NimBLEAttValue::EMPTY_VALUE{0}; // Default constructor implementation. NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len) - : m_attr_value{static_cast(calloc(init_len + 1, 1))}, + : m_attr_value{init_len > 0 ? static_cast(calloc(init_len + 1, 1)) : &EMPTY_VALUE}, m_attr_max_len{std::min(BLE_ATT_ATTR_MAX_LEN, max_len)}, m_attr_len{}, m_capacity{init_len} @@ -75,10 +76,10 @@ NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len) { NIMBLE_CPP_DEBUG_ASSERT(m_attr_value); if (m_attr_value == nullptr) { + // Keep max_len so the object can still allocate later; callers detect the failure by size() != requested len. NIMBLE_LOGE(LOG_TAG, "Failed to allocate value buffer"); - m_attr_len = 0; - m_capacity = 0; - m_attr_max_len = 0; + m_attr_value = &EMPTY_VALUE; + m_capacity = 0; } } @@ -91,7 +92,7 @@ NimBLEAttValue::NimBLEAttValue(const uint8_t* value, uint16_t len, uint16_t max_ return; } - if (m_attr_value != nullptr && len > 0) { + if (len > 0 && m_capacity >= len) { memcpy(m_attr_value, value, len); m_attr_len = len; } @@ -99,24 +100,22 @@ NimBLEAttValue::NimBLEAttValue(const uint8_t* value, uint16_t len, uint16_t max_ // Destructor implementation. NimBLEAttValue::~NimBLEAttValue() { - if (m_attr_value != nullptr) { - free(m_attr_value); - } + free(ownedBuffer()); } // Move assignment operator implementation. NimBLEAttValue& NimBLEAttValue::operator=(NimBLEAttValue&& source) { if (this != &source) { Lock lock; - free(m_attr_value); + free(ownedBuffer()); m_attr_value = source.m_attr_value; m_attr_max_len = source.m_attr_max_len; m_attr_len = source.m_attr_len; m_capacity = source.m_capacity; setTimeStamp(source.getTimeStamp()); - source.m_attr_value = nullptr; - source.m_attr_len = 0; - source.m_capacity = 0; + source.m_attr_value = &EMPTY_VALUE; + source.m_attr_len = 0; + source.m_capacity = 0; source.setTimeStamp(0); } @@ -133,8 +132,12 @@ NimBLEAttValue& NimBLEAttValue::operator=(const NimBLEAttValue& source) { // Copy all the data from the source object to this object, including allocated space. bool NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { + if (this == &source) { + return true; // Self-copy; reallocating would leave an aliased, uninitialized buffer when source is EMPTY_VALUE. + } + Lock lock; - uint8_t* res = static_cast(realloc(m_attr_value, source.m_capacity + 1)); + uint8_t* res = static_cast(realloc(ownedBuffer(), source.m_capacity + 1)); NIMBLE_CPP_DEBUG_ASSERT(res); if (res == nullptr) { NIMBLE_LOGE(LOG_TAG, "Failed to realloc deepCopy"); @@ -154,9 +157,11 @@ bool NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { 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. + if (m_attr_value != &EMPTY_VALUE) { + m_attr_value[0] = '\0'; // Set the first byte to 0 incase the len of the new value is 0. + } append(value, len); - return memcmp(m_attr_value, value, len) == 0 && m_attr_len == len; + return m_attr_len == len && memcmp(m_attr_value, value, len) == 0; } // Append the new data, allocate as necessary. @@ -175,7 +180,7 @@ NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) { uint16_t new_len = m_attr_len + len; bool grow = new_len > m_capacity; if (grow) { - res = static_cast(realloc(m_attr_value, (new_len + 1))); + res = static_cast(realloc(ownedBuffer(), (new_len + 1))); } NIMBLE_CPP_DEBUG_ASSERT(res); if (res == nullptr) { diff --git a/src/NimBLEAttValue.h b/src/NimBLEAttValue.h index cccf1553..7543de7c 100644 --- a/src/NimBLEAttValue.h +++ b/src/NimBLEAttValue.h @@ -99,7 +99,10 @@ struct Has_value_type : std:: * many different container classes. */ class NimBLEAttValue { - uint8_t* m_attr_value{}; + // Shared, never-freed buffer that m_attr_value points to when nothing is allocated (reset or allocation failure). + // Non-const and defined in the .cpp so it can be pointed at by a uint8_t* and odr-used before C++17. + static uint8_t EMPTY_VALUE; + uint8_t* m_attr_value{&EMPTY_VALUE}; uint16_t m_attr_max_len{}; uint16_t m_attr_len{}; uint16_t m_capacity{}; @@ -108,6 +111,9 @@ class NimBLEAttValue { # endif bool deepCopy(const NimBLEAttValue& source); + // The heap buffer owned by this object, or nullptr if m_attr_value points at EMPTY_VALUE. + uint8_t* ownedBuffer() const { return m_attr_value == &EMPTY_VALUE ? nullptr : m_attr_value; } + // Clamp so a large container size can't wrap when narrowed to uint16_t; the constructor then rejects it as > max_len. static constexpr uint16_t clampLen(size_t len) { return len > UINT16_MAX ? UINT16_MAX : static_cast(len); diff --git a/src/NimBLEClient.cpp b/src/NimBLEClient.cpp index 98d045e6..298f782b 100644 --- a/src/NimBLEClient.cpp +++ b/src/NimBLEClient.cpp @@ -1184,7 +1184,7 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) { } NimBLEAttValue value(totalLen, BLE_ATT_ATTR_MAX_LEN); - if (value.data() == nullptr) { + if (value.capacity() < totalLen) { NIMBLE_LOGE(LOG_TAG, "Failed to allocate notification value"); return BLE_ATT_ERR_INSUFFICIENT_RES; } diff --git a/src/NimBLEServer.cpp b/src/NimBLEServer.cpp index 2e54f088..aa1b6bce 100644 --- a/src/NimBLEServer.cpp +++ b/src/NimBLEServer.cpp @@ -784,7 +784,7 @@ int NimBLEServer::handleGattEvent(uint16_t connHandle, uint16_t attrHandle, ble_ } NimBLEAttValue buf(ctxt->om->om_data, len, maxLen); - if (buf.data() == nullptr || buf.size() != len) { + if (buf.size() != len) { return BLE_ATT_ERR_INSUFFICIENT_RES; }