[Bugfix] Missing notification data when length > 255 bytes

[h2zero]

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.

Fixes #1041

Summary by CodeRabbit

• Bug Fixes

  • Robust reassembly of multi-fragment Bluetooth LE notifications so callbacks receive a single, validated payload.
  • Improved length validation and bounded-fragment checks to prevent overflow, mismatches, and data corruption; errors abort delivery early and result in empty values.

• Style

  • Simplified disconnect logging for clearer, single-line messages.

[coderabbitai]

📝 Walkthrough

Walkthrough

Arrr — Client DISCONNECT logging was condensed to a single-line format, and NOTIFY_RX was changed to assemble multi-fragment BLE notifications by walking mbuf fragments (using om_len/SLIST_NEXT), validate total length, update characteristic value, and invoke the notify callback with the assembled buffer (or fail early on error). (50 words)

Changes

Cohort / File(s)

Summary

Multi-Fragment Notification & Disconnect Logging src/NimBLEClient.cpp

Reworked NOTIFY_RX to use om_len, locate pChr, iterate mbuf fragments (SLIST_NEXT) to assemble a bounded buf, validate accumulated length (set BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN on mismatch/overflow), update characteristic value or clear on error, and call notify callback with assembled buf/len. Also collapsed DISCONNECT path logging into a single-line message and added peer lookup by address then conn handle.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Yarrr, the fragments no longer roam the sea,
We plait their bytes till whole they be,
A tidy chest of data, true and sound,
Callbacks get treasure—no pieces drowned,
🏴‍☠️📦

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR addresses the core requirement from issue #1041: ensuring notifications larger than 255 bytes are correctly delivered by assembling multi-fragment mbufs. However, reviewer concerns about buffer overflow risks, missing bounds checks, and incomplete error handling remain unresolved.	Implement dynamic allocation or explicit bounds validation before memcpy operations to prevent buffer overflows when notifications exceed BLE_ATT_ATTR_MAX_LEN; add comprehensive error notification to the application layer.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: handling notification data exceeding 255 bytes by reassembling multi-fragment mbufs into a single buffer.
Out of Scope Changes check	✅ Passed	The DISCONNECT logging adjustment is a minor formatting change unrelated to the multi-fragment notification handling objective, though it does not significantly detract from the primary scope.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch bugfix/notify-data-len

📝 Coding Plan

• Generate coding plan for human review comments

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[doudar]

Arrrr, let’s lay eyes on this code and see if any beasties be lurkin’ in these waters!

---

🔴 [BUG]: Hardcoded buffer size might lead to overflow.

• Where: Refer to the buf declaration on NimBLEClient.cpp, line ~1071 (uint8_t buf[BLE_ATT_ATTR_MAX_LEN];).
• Problem: The buffer size is hardcoded to BLE_ATT_ATTR_MAX_LEN. If BLE_ATT_ATTR_MAX_LEN isn’t large enough for the combined notification data (especially in rare edge cases), this can lead to undefined behavior or data overwriting.
• Fix: Use dynamic memory allocation for buf, properly freeing it after use, or validate the total len at the start to ensure it doesn’t exceed BLE_ATT_ATTR_MAX_LEN before proceeding with copying data. Alternatively, reject buffers too large altogether.

---

🔴 [SECURITY FLAW]: Lack of bounds check before copying data via memcpy.

• Where: Lines involving memcpy(buf, event->notify_rx.om->om_data, len); and subsequent buffer writes within the while loop.
• Problem: There’s no comprehensive check for len vs. sizeof(buf) before copying. If len exceeds the buffer size due to unexpected input, this will result in a buffer overflow.
• Fix: Add a bounds check before each memcpy, ensuring the size doesn’t exceed sizeof(buf).

---

🔴 [PERFORMANCE ISSUE]: Inefficient handling of datagrams from the os_mbuf chain.

• Where: Loop on SLIST_NEXT(event->notify_rx.om, om_next) within the new reassembly code.
• Problem: Copying om_data to buf manually in chunks results in redundant memory copying and scales poorly with large notification data. It adds unnecessary computational overhead.
• Fix: Instead of copying os_mbuf data into a flat buffer, consider iterating through the os_mbuf chain and directly working with the individual fragments, or use existing utilities provided by the NimBLE library to flatten the chain efficiently.

---

🔴 [CODE SMELL]: Incomplete error handling for oversized messages.

• Where: NIMBLE_LOGE and return BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; when overflow is detected.
• Problem: Dropping oversized notifications silently (after logging) risks inconsistency in behavior and debugging difficulty later. Developers might struggle to identify a lost message.
• Fix: Explicitly alert the application/client that a notification was truncated or dropped. Either invoke a dedicated callback to report the state or set a flag that can be queried.

---

🔴 [STYLE ISSUE]: Nested if blocks lacking braces reduce readability.

• Where: Within the while loop analyzing next mbufs and in other areas of the function.
• Problem: Although technically valid, the omission of braces for single-line blocks introduces future maintenance risks if additional logic is appended.
• Fix: Use braces consistently around all blocks, even for single-line conditions.

---

Arrr, some rough seas here, but the ship can be patched up nicely with these fixes. Don’t ignore the dangers of buffer overflows and inefficiencies, or ye may lose critical treasure (yer users’ data)!

—CodePirate (AI)

[coderabbitai]

🧹 Nitpick comments (2)

src/NimBLEClient.cpp (2)

1069-1072: Arrr, guard the first fragment before ye memcpy.

The first chunk is copied before any size/null check. Add a fast guard so malformed input can’t overrun buf.

⚓ Proposed hardening patch

-                        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);
+                        uint8_t  buf[BLE_ATT_ATTR_MAX_LEN];
+                        if (event->notify_rx.om == nullptr ||
+                            event->notify_rx.om->om_len > BLE_ATT_ATTR_MAX_LEN) {
+                            NIMBLE_LOGE(LOG_TAG, "Invalid notification mbuf length");
+                            return BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN;
+                        }
+                        uint16_t len = event->notify_rx.om->om_len;
+                        memcpy(buf, event->notify_rx.om->om_data, len);

🤖 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, Add a fast guard before the
memcpy in NimBLEClient.cpp: validate that event->notify_rx.om is non-NULL, that
event->notify_rx.om->om_data is non-NULL, and that the length
(event->notify_rx.om->om_len) does not exceed the destination buffer size
(BLE_ATT_ATTR_MAX_LEN) and is non-negative/meaningful; if the checks fail, bail
out (return/continue/log) rather than calling memcpy into buf. Specifically
protect the memcpy(buf, event->notify_rx.om->om_data, len) by checking the om
pointer, om_data, and that len <= BLE_ATT_ATTR_MAX_LEN (and optionally len > 0)
before performing the copy.

---

1076-1080: Arrr, overlong notification drops still be hidden from application code.

When overflow hits, the packet is dropped with only a log. Consider an app-level overflow callback so user code can react (metrics/retry/reconnect).

Based on learnings: In NimBLEStream (src/NimBLEStream.cpp, NimBLE-Arduino), the author's preference regarding RX buffer overflow in pushRx is that the bigger concern is not partial vs. full drops, but that the application layer has no notification mechanism when data is dropped. A user-settable overflow callback is the preferred direction over silently accepting partial writes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEClient.cpp` around lines 1076 - 1080, The notification-drop branch
in NimBLEClient.cpp currently only logs and returns
BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN, leaving apps unaware of dropped data; add a
user-settable overflow callback on the client (e.g., a std::function<void(size_t
attempted, size_t available)> named rxOverflowCallback and a setter
setRxOverflowCallback) in the NimBLEClient class, invoke this callback right
before returning in the overflow branch (pass attempted length and remaining
buffer size or next->om_len), and ensure the callback is default-null-checked so
behavior remains unchanged if unset.

🤖 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 1069-1072: Add a fast guard before the memcpy in NimBLEClient.cpp:
validate that event->notify_rx.om is non-NULL, that event->notify_rx.om->om_data
is non-NULL, and that the length (event->notify_rx.om->om_len) does not exceed
the destination buffer size (BLE_ATT_ATTR_MAX_LEN) and is
non-negative/meaningful; if the checks fail, bail out (return/continue/log)
rather than calling memcpy into buf. Specifically protect the memcpy(buf,
event->notify_rx.om->om_data, len) by checking the om pointer, om_data, and that
len <= BLE_ATT_ATTR_MAX_LEN (and optionally len > 0) before performing the copy.
- Around line 1076-1080: The notification-drop branch in NimBLEClient.cpp
currently only logs and returns BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN, leaving apps
unaware of dropped data; add a user-settable overflow callback on the client
(e.g., a std::function<void(size_t attempted, size_t available)> named
rxOverflowCallback and a setter setRxOverflowCallback) in the NimBLEClient
class, invoke this callback right before returning in the overflow branch (pass
attempted length and remaining buffer size or next->om_len), and ensure the
callback is default-null-checked so behavior remains unchanged if unset.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 314745aa-ca61-4c82-89e5-baa7bc8eca67

📥 Commits

Reviewing files that changed from the base of the PR and between 13dc292 and 970537a.

📒 Files selected for processing (1)

• src/NimBLEClient.cpp

[coderabbitai]

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-1085: Pre-calc the total length of the mbuf chain starting at
event->notify_rx.om (traverse with SLIST_NEXT) before calling
chr->m_value.setValue so you can preventively check against the characteristic's
maximum instead of reacting to NimBLEAttValue::append failures; handle and honor
the return value of chr->m_value.setValue (do not ignore it) and if the total
exceeds capacity, invoke a user-settable overflow/notification-truncation
callback on the characteristic (add an onOverflow/onTruncated callback to the
characteristic class) with details of the truncated data, then return a non-zero
error code to the NimBLE stack instead of 0 to signal failure to the app; also
remove the post-append length-only check and use the precomputed total_len for
validation to simplify logic (refer to event->notify_rx.om, SLIST_NEXT,
chr->m_value, setValue, NimBLEAttValue::append).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ff66abd2-73a6-443c-8981-f2fbde59bf86

📥 Commits

Reviewing files that changed from the base of the PR and between 970537a and 7e9783c.

📒 Files selected for processing (1)

• src/NimBLEClient.cpp

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/NimBLEClient.cpp (1)

1054-1076: Arrr, keep handle lookup on the existing helper.

This open-codes work NimBLEClient::getCharacteristic() already does at Lines 865-877, and the two paths already differ on service-range checks. Reusing the helper keeps notification lookup rules in one place.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEClient.cpp` around lines 1054 - 1076, The notification handling
currently duplicates characteristic lookup logic; replace the manual
service/characteristic iteration with the existing helper
NimBLEClient::getCharacteristic() to centralize lookup rules: call
pClient->getCharacteristic(event->notify_rx.attr_handle) (or the appropriate
overload) to obtain the NimBLERemoteCharacteristic pointer, check for nullptr
and handle as before, and remove the duplicated loops and service-range checks
so all lookup behavior is maintained in getCharacteristic().

🤖 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 1084-1096: The logged ATT status `rc` is a raw ATT error but
`NimBLEUtils::returnCodeToString()` expects errors with `BLE_HS_ERR_ATT_BASE`
added, so the string becomes "Unknown"; update the log call in the error path to
pass the adjusted code (e.g., call
NimBLEUtils::returnCodeToString(BLE_HS_ERR_ATT_BASE + rc)) only for ATT status
values, leaving non-ATT rc untouched so the human-readable string matches the
numeric code.

---

Nitpick comments:
In `@src/NimBLEClient.cpp`:
- Around line 1054-1076: The notification handling currently duplicates
characteristic lookup logic; replace the manual service/characteristic iteration
with the existing helper NimBLEClient::getCharacteristic() to centralize lookup
rules: call pClient->getCharacteristic(event->notify_rx.attr_handle) (or the
appropriate overload) to obtain the NimBLERemoteCharacteristic pointer, check
for nullptr and handle as before, and remove the duplicated loops and
service-range checks so all lookup behavior is maintained in
getCharacteristic().

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 46972862-ec15-47a5-9c6a-ab4fac8aa48f

📥 Commits

Reviewing files that changed from the base of the PR and between 7e9783c and 21c2cf6.

📒 Files selected for processing (1)

• src/NimBLEClient.cpp