feat(L2CAP): add disconnect API and harden CoC send/error handling#1139
feat(L2CAP): add disconnect API and harden CoC send/error handling#1139
Conversation
- Add NimBLEL2CAPChannel::disconnect() and getConnHandle(). - Fix CoC TX mbuf ownership handling in writeFragment(): treat BLE_HS_ENOMEM / BLE_HS_EAGAIN as consumed buffer, only free local tx mbuf on BLE_HS_EBUSY.
📝 WalkthroughWalkthroughArr, this pull request be addin' two new public APIs to the Changes
Possibly related issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/NimBLEL2CAPChannel.cpp`:
- Around line 135-147: The BLE send-error handling incorrectly assumes nimble
frees the mbuf for BLE_HS_ENOMEM and BLE_HS_EAGAIN; add os_mbuf_free_chain(txd)
before the continue in the BLE_HS_ENOMEM and BLE_HS_EAGAIN case blocks (the same
cleanup already present in the BLE_HS_EBUSY branch) in the switch that handles
ble_l2cap_send results so txd is freed on those error paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92085361-7183-4af6-9a07-48e22f76c8e9
📒 Files selected for processing (2)
src/NimBLEL2CAPChannel.cppsrc/NimBLEL2CAPChannel.h
| case BLE_HS_ENOMEM: | ||
| case BLE_HS_EAGAIN: | ||
| /* ble_l2cap_send already consumed and freed txd on these errors */ | ||
| NIMBLE_LOGD(LOG_TAG, "ble_l2cap_send returned %d (consumed buffer). Retrying shortly...", res); | ||
| ble_npl_time_delay(ble_npl_time_ms_to_ticks32(RetryTimeout)); | ||
| continue; | ||
|
|
||
| case BLE_HS_EBUSY: | ||
| NIMBLE_LOGD(LOG_TAG, "ble_l2cap_send returned %d. Retrying shortly...", res); | ||
| /* Channel busy; txd not consumed */ | ||
| NIMBLE_LOGD(LOG_TAG, "ble_l2cap_send returned %d (busy). Retrying shortly...", res); | ||
| os_mbuf_free_chain(txd); | ||
| ble_npl_time_delay(ble_npl_time_ms_to_ticks32(RetryTimeout)); | ||
| continue; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
NimBLE ble_l2cap_send mbuf buffer ownership on BLE_HS_ENOMEM BLE_HS_EAGAIN error codes
💡 Result:
In NimBLE, for ble_l2cap_send(struct ble_l2cap_chan *chan, struct os_mbuf *sdu), the function consumes (frees) the supplied mbuf only on success. On error returns such as BLE_HS_ENOMEM (heap exhaustion, no memory) or BLE_HS_EAGAIN (operation would block), the caller retains ownership of the mbuf and must free it using os_mbuf_free_chain(sdu) to avoid leaks. BLE_HS_ENOMEM typically occurs due to mbuf pool exhaustion when the stack cannot allocate additional buffers for transmission or fragmentation. BLE_HS_EAGAIN indicates the controller cannot accept the full packet immediately (e.g., transmit queue full), so the remainder may be enqueued, but the input mbuf is not consumed on this error.
Citations:
- 1: https://github.com/apache/mynewt-nimble/blob/master/nimble/host/src/ble_l2cap.c
- 2: extmod/nimble: Fix leak in l2cap_send if send-while-stalled. micropython/micropython#7506
- 3: https://github.com/apache/mynewt-nimble/blob/master/nimble/host/include/host/ble_l2cap.h
- 4: ble_l2cap_tx crash on boundaries maybe (IDFGH-2268) espressif/esp-idf#4410
- 5: https://taks.github.io/esp32-nimble/src/esp32_nimble/ble_error.rs.html
Shiver me timbers! Ye've got a memory leak on yer hands, cap'n!
The code be mistaken about buffer ownership. NimBLE's ble_l2cap_send only frees the mbuf on success; on errors like BLE_HS_ENOMEM and BLE_HS_EAGAIN, the caller retains ownership and must free it. Yet yer code be skippin' os_mbuf_free_chain(txd) on those very errors while properly freeein' it for BLE_HS_EBUSY. This be a memory leak that'll drain yer treasure chest over time, matey!
Add os_mbuf_free_chain(txd) before continue in both the BLE_HS_ENOMEM and BLE_HS_EAGAIN cases, just like ye do fer BLE_HS_EBUSY.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEL2CAPChannel.cpp` around lines 135 - 147, The BLE send-error
handling incorrectly assumes nimble frees the mbuf for BLE_HS_ENOMEM and
BLE_HS_EAGAIN; add os_mbuf_free_chain(txd) before the continue in the
BLE_HS_ENOMEM and BLE_HS_EAGAIN case blocks (the same cleanup already present in
the BLE_HS_EBUSY branch) in the switch that handles ble_l2cap_send results so
txd is freed on those error paths.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes