fix(mesh): plug pooled-object and driver leaks in core paths - #11450
Conversation
- MeshService::sendQueueStatusToPhone: release the pooled QueueStatus when the toPhone queue enqueue fails, matching what sendMqttMessageToClientProxy and sendClientNotification already do. The full-queue guard makes this failure rare, but the check/enqueue sequence is not atomic and this path is reachable concurrently from the main loop and the nRF52 BLE write callback; each failure permanently lost one of the four pool slots, and after four losses the phone never receives QueueStatus again until reboot. - MessageStore::storeTextInPool: bail out when the boot-time pool allocation failed instead of memcpy'ing through a null pointer. The read side (getTextFromPool) already guards and maps offset 0 to an empty string. - RF95Interface: hold the RadioLibRF95 driver in a unique_ptr. It is constructed in init(), and when init() subsequently fails (e.g. chip probe NOT_FOUND) initLoRa() destroys the interface, leaking the driver; every sibling interface holds its driver by value so nothing else needed a destructor here.
📝 WalkthroughWalkthroughThe changes add allocation-failure handling for message storage and queue-status delivery. They also give ChangesResource safety updates
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The PR addresses three localized resource-safety paths. The remaining comment-length adjustment does not affect runtime behavior, so no actionable merge-blocking risk remains after normal checks. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/RF95Interface.h (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the ownership comment. Keep the initialization and ownership rationale within the repository’s one- or two-line comment limit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/RF95Interface.h` around lines 14 - 17, Shorten the ownership comment in RF95Interface to one or two lines while preserving that the driver is uniquely owned and constructed during init() to avoid leaks if initialization fails. Apply the same fix in `@src/mesh/RF95Interface.h` at line 18.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/RF95Interface.h`:
- Around line 14-17: Shorten the ownership comment in RF95Interface to one or
two lines while preserving that the driver is uniquely owned and constructed
during init() to avoid leaks if initialization fails.
Apply the same fix in `@src/mesh/RF95Interface.h` at line 18.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bfa51b8c-1be2-44cd-89bb-0a1dd6806f7a
📒 Files selected for processing (4)
src/MessageStore.cppsrc/mesh/MeshService.cppsrc/mesh/RF95Interface.cppsrc/mesh/RF95Interface.h
1 participant
Three small memory fixes in core mesh paths:
MeshService::sendQueueStatusToPhone — pool slot lost on enqueue failure
The function allocates from the 4-slot
staticQueueStatusPool, then returns without releasing whentoPhoneQueueStatusQueue.enqueue()fails. Its two siblings in the same file (sendMqttMessageToClientProxy,sendClientNotification) both release on exactly this failure. The full-queue guard makes the failure impossible single-threaded, but the numFree/dequeue/enqueue sequence is not atomic and this path runs concurrently from the main loop and (on nRF52) the Bluefruit BLE write callback (NRF52Bluetooth.cpponToRadioWrite→handleToRadio→sendToMesh→sendQueueStatusToPhone). Each occurrence permanently loses a pool slot; after four, QueueStatus to the phone is dead until reboot.MessageStore::storeTextInPool — write through null pool pointer
resetMessagePool()leavesg_messagePoolnull when the bootmallocfails; the read side (getTextFromPool) guards for that, but the write side memcpy'd through the null pointer on every stored text message. Now returns offset 0, which the read side already maps to"".RF95Interface — driver leaked when radio probe fails
init()doeslora = new RadioLibRF95(&module)with no destructor anywhere in the class. Wheninit()then fails (e.g.begin()returnsCHIP_NOT_FOUNDwhile probing),initLoRa()destroys the interface viaunique_ptr<RadioInterface>and the driver object leaks. Hold it in aunique_ptr— every other radio interface holds its driver by value, so this is the only one that needed it.ifaceremains a non-owning alias (~RadioLibInterfacedoes not delete it).Testing
trunk fmtclean; native test suite run planned across the fix series before merge.Summary by CodeRabbit