[Bugfix] Missing notification data when length > 255 bytes#402
Closed
[Bugfix] Missing notification data when length > 255 bytes#402
Conversation
Rather than asserting for a harmless situation, this will just create a dummy descriptor object, marked as removed, and print a warning.
* Trigger onRead callbacks when the connection handle is NONE (internal).
This function is not required as the stack will return an error code in the case of invalid params.
* Adds the NIMBLE_CPP_DEBUG_ASSERT macro that calls a weak function to allow user defined action, defaults to critical error message and aborts. * Adds a config option to enable asserts using the NIMBLE_CPP_DEBUG_ASSERT macro.
Replaces all use of NIMBLE_LOGC with NIMBLE_LOGE and redefines NIMBLE_LOGC to use printf. This allows NIMBLE_CPP_DEBUG_ASSERT messages to print event when log level filtering is set to none.
* Replaces the use of std::list with a fixed array to track and manage created client instances. * Removes: NimBLEDevice::getClientList * Replaces: NimBLEDevice::getClientListSize with NimBLEDevice::getCreatedClientCount
This simplifies the NimBLEAddress code by directly using the NimBLE core `ble_addr_t` type to hold the address and allows using NimBLE core functions and macros to replace code in some methods. * `getNative()` replaced with `getBase()` and now returns a pointer to `const ble_addr_t` instead of a pointer to the address value. * Adds `isNrpa()` method to test if an address is random non-resolvable. * Adds `isStatic()` method to test if an address is random static. * Adds `isPublic()` method to test if an address is a public address. * Adds `isNull()` methods to test if an address is NULL. * Adds `getValue()` method which returns a read-only pointer to the address value. * Adds `reverseByteOrder()` method which will reverse the byte order of the address value. * `equals()` method and == operator will now also test if the address types are the same. * Code cleanup.
* Construct the device with the parameters from the advertisement in the initialization list. * Removed no longer needed methods; setAddress, setAdvType, setRSSI, setSetId, setPrimaryPhy, setSecondaryPhy, setPeriodicInterval. * Removed `hasRSSI()` method, the RSSI is always reported so this is redundant. * Replace setPayload with new method; `update` which will update the device info when new advertisement data is received. * getPayload now returns `const std::vector<uint8_t>` instead of a pointer to internal memory. * Added `begin` and `end` read-only iterators for convienience and use in range loops. * Timestamp removed, if needed then the app should track the time in the callback. * Consolidate some functions to use getPayloadByType. * Add optional index parameter to getPayloadByType. * Change payload indexing to use 0 as the first item. * Code cleanup and apply const correctness.
* Add length parameter to `setValue()` templates, with defaults for backward compatibility. * Changed `setValue(const char*)` to add an optional length parameter so that a NULL character can be included, defaults to strlen. * Moved non-inline functions to `NimBLEAttValue.cpp` file. * Corrected includes to permit compilation as a stand-alone utility. * Removed repeated code in `setValue()` by using `append()` after clearing the value. * General code cleanup. # Conflicts: # CMakeLists.txt
* msbFirst parameter has been removed from constructor as it was unnecessary, caller should reverse the data first or call the new `reverseByteOrder` method after. * `getNative` method replaced with `getBase` which returns a read-only pointer to the UUID size underlying. * Added `reverseByteOrder` method, this will reverse the bytes of the UUID, which can be useful for advertising/logging. * Added `getValue` method, which returns a read-only `uint8_t` pointer to the UUID value. * Removed `m_valueSet` member variable, `bitSize()` can be used as a replacement. * General code cleanup.
Refactor attributes to reduce code duplication and improve maintainability. * Add attribute base classes to provide common code. * Add const where possible to functions and parameters. * `NimBLECharacteristic::notify` no longer takes a `bool is_notification` parameter, instead `indicate()` should be called to send indications. * `NimBLECharacteristic::indicate` now takes the same parameters as `notify`. * `NimBLECharacteristicCallbacks` and `NimBLEDescriptorCallbacks` methods now take `const NimBLEConnInfo&` instead of non-const. * `NimBLECharacteristic::onNotify` callback removed as unnecessary, the library does not call notify without app input. * `NimBLERemoteCharacteristic::getRemoteService` now returns a `const NimBLERemoteService*` instead of non-const. * Add NimBLEUUID constructor that takes a reference to `ble_uuid_any_t`. * `NimBLERemoteService::getCharacteristics` now returns a `const std::vector<NimBLERemoteCharacteristic*>&` instead of non-const `std::vector<NimBLERemoteCharacteristic*>*` * `NimBLERemoteService::getValue` now returns `NimBLEAttValue` instead of `std::string` * `NimBLEService::getCharacteristics` now returns a `const std::vector<NimBLECharacteristic*>&` instead of a copy of std::vector<NimBLECharacteristic *>. * Remove const requirement for NimBLEConnInfo parameter in callbacks. Const is unnecessary as the data can't be changed by application code. * Change NimBLERemoteCharacteristic::getRemoteService to return const pointer.
The connection handle was not initialized correctly when recording the subscibe status causing the call to send notification/indications to fail with BLE_HS_ENOTCONN.
* Use `data()`/`size()` instead of `c_str()`/`length()` * Reduce duplication, only allow template function in characteristic and remote value attr if the type is not a pointer (otherwise sizeof is useless). add appropriate notes * clean up AttValue::setValue to remove unnecessary length parameter enabling requirement of non-pointer type
Document that setting `duration` to zero means to scan forever.
* General code cleanup * `NimBLEDevice::getInitialized` renamed to `NimBLEDevice::isInitialized`. * `NimBLEDevice::setPower` no longer takes the `esp_power_level_t` and `esp_ble_power_type_t`, instead only an integer value in dbm units is accepted. * `NimBLEDevice::setPower` now returns a bool value, true = success. * `NimBLEDevice::setMTU` now returns a bool value, true = success. * `NimBLEDevice::injectConfirmPIN` renamed to `NimBLEDevice::injectConfirmPasskey` to use Bluetooth naming. * Fixes crash if `NimBLEDevice::deinit` is called when the stack has not been initialized. * Reverts 2db4756 as it would cause a crash when the NimBLEServer instance has a connection. * `NimBLEDevice::getAddress` will now return the address currently in use. * `NimBLEDevice::init` now returns a bool with `true` indicating success. * `NimBLEDevice::deinit` now returns a bool with `true` inidicating success. * `NimBLEDevice::setDeviceName` now returns a bool with `true` indicating success. * `NimBLEDevice::setCustomGapHandler` now returns a bool with `true` indicating success. * `NimBLEDevice::setOwnAddrType` now returns a bool with `true` indicating success. * `NimBLEDevice::setOwnAddrType` will now correctly apply the provided address type for all devices. * `NimBLEDevice::setOwnAddrType` no longer takes a `bool nrpa` parameter.
Adds a utility to generate random BLE addresses and set the address used.
* General code cleanup and rename variables to use a consistent style. * Removes the disconnect timer and will use the BLE_GAP_EVENT_TERM_FAILURE event to handle failed disconnects. * `NimBLEClient::getConnId` has been renamed to `getConnHandle` to be consistent with bluetooth terminology. * `NimBLEClient::disconnect` now returns a `bool = true on success` instead of an int to be consistent with the rest of the library. * `NimBLEClient::setPeerAddress` now returns a bool, true on success. * `NimBLEClientCallbacks::onConfirmPIN` renamed to `NimBLEClientCallbacks::onConfirmPasskey` to be consistent with bluetooth terminology. * `NimBLEClient::setDataLen` now returns bool, true if successful. * `NimBLEClient::updateConnParams` now returns bool, true if successful. * `NimBLEClient::getServices` now returns a const reference to std::vector<NimBLERemoteService*> instead of a pointer to the internal vector.
The previous implementation incorrectly used the service's end handle when searching for descriptors, which caused it to retrieve descriptors from subsequent characteristics as well. This fix calculates the correct end handle by finding the next characteristic's handle and using (next_handle - 1) as the search limit. This ensures only descriptors belonging to the current characteristic are retrieved. Fixes incorrect descriptor retrieval when multiple characteristics exist in the same service.
In a busy environment the number of buffers could be insufficent causing a crash. This increases the buffer count to avoid this issue. Also this will use the controller allocated buffers when nimble is enabled in the Arduino core to prevent extra memory being allocated.
This fixes a bug caused by the arduino core enabling mesh/mesh filtering which sets the incorrect mode for normal BLE scanning. In Mesh filtering mode the controller does not flush the device cache so subsequent restarts of the scan will not provide duplicates as expected.
Adds a missing macro to enable vendor specific HCI commands to allow reading the harware mac address.
This changes how attribute handles are set so they can be correctly identified when there is more than one attribute with the same UUID. Instead of reading from the stack by UUID to get the handles this will now use the registration callback to set them correctly. This also improves handling of dynamic service changes by properly removing characteristics/descriptors when required and resetting the GATT when advertising is started instead of after the last client disconnects. * Adds NimBLEUUID constructor overload for ble_uuid_t*. * NimBLECharacteristic::getDescriptorByUUID now takes an optional index value to support multiple same-uuid descriptors.
Co-authored-by: doudar <17362216+doudar@users.noreply.github.com>
Co-authored-by: doudar <17362216+doudar@users.noreply.github.com>
📝 WalkthroughWalkthroughArr, the BLE notify handler now reassembles chained OS mbufs into a single buffer with bounds checks and passes the full assembled value to the notification callback; a disconnect log line was slightly reformatted. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Client Controller
participant NimBLE as NimBLE Stack
participant Mbuf as OS mbuf Chain
participant Callback as Notification Callback
Controller->>NimBLE: BLE_GAP_EVENT_NOTIFY_RX(p_conn, p_chr, om)
NimBLE->>Mbuf: read om (om_len, om_data) and traverse om_next*
Mbuf-->>NimBLE: assembled bytes (verify lengths each append)
NimBLE->>Callback: notify(p_chr, assembled_value, full_length) or error (clear value)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEClient.cpp`:
- Around line 1069-1072: The code copies event->notify_rx.om->om_data into a
fixed stack buffer buf[BLE_ATT_ATTR_MAX_LEN] using memcpy without first bounding
the source length, risking stack overflow if om_len > BLE_ATT_ATTR_MAX_LEN;
change the logic in NimBLEClient.cpp around the memcpy of buf (and the later
memcpy at the subsequent block handling other mbuf chunks) to first clamp len =
min((size_t)event->notify_rx.om->om_len, (size_t)BLE_ATT_ATTR_MAX_LEN) (or check
and bail/resize) and use that bounded length for memcpy, and apply the same
defensive length check before the second memcpy that copies from
om->om_next->om_data. Ensure you reference buf, BLE_ATT_ATTR_MAX_LEN, len,
event->notify_rx.om, and the memcpy calls when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a113969f-9cea-47b9-8545-a438ab3c2a3e
📒 Files selected for processing (1)
src/NimBLEClient.cpp
src/NimBLEClient.cpp
Outdated
Comment on lines
+1069
to
+1072
| uint8_t buf[BLE_ATT_ATTR_MAX_LEN]; | ||
| uint16_t len = event->notify_rx.om->om_len; | ||
| memcpy(buf, event->notify_rx.om->om_data, len); | ||
|
|
There was a problem hiding this comment.
Guard the first memcpy, matey — stack overflow risk.
On Line 1071, ye copy the first mbuf chunk before any size check. If om_len > BLE_ATT_ATTR_MAX_LEN, this overflows buf before the later loop guard ever runs.
🏴☠️ Patch suggestion
uint8_t buf[BLE_ATT_ATTR_MAX_LEN];
uint16_t len = event->notify_rx.om->om_len;
+ if (len > BLE_ATT_ATTR_MAX_LEN) {
+ NIMBLE_LOGE(LOG_TAG,
+ "Received notification is too long for buffer, dropping data");
+ return BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN;
+ }
memcpy(buf, event->notify_rx.om->om_data, len);Also applies to: 1076-1079
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEClient.cpp` around lines 1069 - 1072, The code copies
event->notify_rx.om->om_data into a fixed stack buffer buf[BLE_ATT_ATTR_MAX_LEN]
using memcpy without first bounding the source length, risking stack overflow if
om_len > BLE_ATT_ATTR_MAX_LEN; change the logic in NimBLEClient.cpp around the
memcpy of buf (and the later memcpy at the subsequent block handling other mbuf
chunks) to first clamp len = min((size_t)event->notify_rx.om->om_len,
(size_t)BLE_ATT_ATTR_MAX_LEN) (or check and bail/resize) and use that bounded
length for memcpy, and apply the same defensive length check before the second
memcpy that copies from om->om_next->om_data. Ensure you reference buf,
BLE_ATT_ATTR_MAX_LEN, len, event->notify_rx.om, and the memcpy calls when making
the changes.
h2zero
force-pushed
the
bugfix/notify-data-len
branch
2 times, most recently
from
ae31577 to
809b284
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1074-1076: Usecontinuehere, matey, notreturn.Returning early bakes in lookup assumptions;
continuekeeps the hunt going if this service has no matching characteristic.⚓ Suggested patch
- if (pChr == nullptr) { - return 0; - } + if (pChr == nullptr) { + continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1074 - 1076, In NimBLEClient.cpp, inside the loop where pChr is inspected (the block that currently does "if (pChr == nullptr) { return 0; }"), replace the early return with "continue" so the loop proceeds to the next candidate instead of exiting the surrounding function; locate the loop that iterates services/characteristics and the variable pChr to make this change (keep surrounding logic and return path unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NimBLEClient.cpp`:
- Around line 1074-1076: In NimBLEClient.cpp, inside the loop where pChr is
inspected (the block that currently does "if (pChr == nullptr) { return 0; }"),
replace the early return with "continue" so the loop proceeds to the next
candidate instead of exiting the surrounding function; locate the loop that
iterates services/characteristics and the variable pChr to make this change
(keep surrounding logic and return path unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01635653-f457-4f87-a2ae-829093b15b57
📒 Files selected for processing (1)
src/NimBLEClient.cpp
When the ACL buffer is less than the MTU, the data arrives in more than one mbuf. This combines the data from the mbuf chain and stores it before calling the appliation callback, ensuring it has all the data.
h2zero
force-pushed
the
bugfix/notify-data-len
branch
from
809b284 to
c4d4f62
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the ACL buffer is less than the MTU, the data arrives in more than one mbuf. This combines the data from the mbuf chain and stores it before calling the appliation callback, ensuring it has all the data
Summary by CodeRabbit