fix: defensive fixes for packet/routing related memory leakages - #11223
fix: defensive fixes for packet/routing related memory leakages#11223t-miura wants to merge 17 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes add runtime validation for memory pools, radio packets, routing dependencies, queue operations, and local-node state. They also improve packet release handling, transmit failure cleanup, pointer logging, and radio safety tests. ChangesRuntime safety and packet lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/mesh/MemoryPool.h`:
- Around line 153-162: Update pointer logging in the pool deallocation paths,
including the misalignment and double-free messages and the nearby LOG_HEAP
“Freeing” message, to use the %p format specifier and pass p or &pool[i] as
static_cast<void *>(...). Leave integer index formatting unchanged.
In `@src/mesh/RadioInterface.cpp`:
- Around line 1476-1480: Update the beginSending handling around sendingPacket
so an active transmission is never released. When sendingPacket is already set,
reject the newly supplied packet p and release p instead, preserving
sendingPacket for its completion path and maintaining the existing warning
behavior.
In `@test/test_radio/test_main.cpp`:
- Around line 374-380: Remove the packetPool.release(p) call from the test that
invokes testRadio->beginSending(p), since beginSending already releases
oversized packets; retain the result and getSendingPacket assertions unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 936ccaec-27b0-4535-b85b-45a5505d2a0e
📒 Files selected for processing (7)
src/mesh/MemoryPool.hsrc/mesh/MeshService.cppsrc/mesh/NextHopRouter.cppsrc/mesh/RadioInterface.cppsrc/mesh/Router.cppsrc/modules/MeshBeaconModule.cpptest/test_radio/test_main.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens packet lifecycle handling across routing and radio paths to prevent memory leaks (notably around ERRNO_SHOULD_RELEASE), adds defensive null/invalid-state guards, and extends unit tests to cover the targeted leak scenarios—motivated by broader platform robustness needs (including STM32WL trials).
Changes:
- Ensure packets are released when
router->send()/Router::send()returnsERRNO_SHOULD_RELEASEin beacon broadcast and next-hop relay paths. - Add defensive checks in routing/radio/service code paths (null router/module/interface, invalid payload variants, oversize radio payloads) with appropriate cleanup.
- Add/extend unit tests to validate oversized payload handling, null-router delivery cleanup, and queue-status behavior under full queues.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_radio/test_main.cpp | Adds tests covering oversized send abort, null-router delivery release, and full queue-status queue behavior. |
| src/modules/MeshBeaconModule.cpp | Releases beacon packets when router->send() indicates caller-owned cleanup (ERRNO_SHOULD_RELEASE). |
| src/mesh/Router.cpp | Adds defensive checks (null routingModule, invalid payload variant handling, null interface handling, OOM warnings). |
| src/mesh/RadioInterface.cpp | Adds defensive guards in receive/send paths and drops+releases invalid/oversized/overlapping sends. |
| src/mesh/NextHopRouter.cpp | Fixes potential relay-copy leak when Router::send() returns ERRNO_SHOULD_RELEASE. |
| src/mesh/MeshService.cpp | Hardens queue scanning/re-enqueue and ensures queue-status allocations are released on enqueue failure. |
| src/mesh/MemoryPool.h | Hardens pool release behavior (misalignment/double-free detection) and adjusts UniqueAllocation deleter type/logging. |
| /// Variations of the above methods that return std::unique_ptr instead of raw pointers. | ||
| using UniqueAllocation = std::unique_ptr<T, const std::function<void(T *)> &>; | ||
| using UniqueAllocation = std::unique_ptr<T, std::function<void(T *)>>; | ||
| /// Return a queable object which has been prefilled with zeros. | ||
| /// std::unique_ptr wrapped variant of allocZeroed(). | ||
| UniqueAllocation allocUniqueZeroed() { return UniqueAllocation(allocZeroed(), deleter); } |
There was a problem hiding this comment.
hm, depends on how much it increases the stack/heap comsumption,
but i feel like re-inventing smaller wheel doesn't sound healthy...
| uintptr_t offset = reinterpret_cast<uintptr_t>(p) - reinterpret_cast<uintptr_t>(pool); | ||
| if (offset % sizeof(T) != 0) { | ||
| LOG_WARN("Pointer %p is misaligned inside static pool!", static_cast<void *>(p)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mesh/RadioInterface.cpp (1)
1476-1480: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAbort
startSend()afterbeginSending()drops the packet.The
beginSending()failure paths releasepand return0, butRadioLibInterface::startSend()still callsstartTransmit()and laterprintPacket(..., txp)in the success path. Add an immediate failure check afterbeginSending()so it never transmits a zero-length buffer or log/access a freed packet; also preserve exactly-once ownership cleanup.🤖 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/RadioInterface.cpp` around lines 1476 - 1480, Update RadioLibInterface::startSend() to immediately detect a failed beginSending() result and return before calling startTransmit() or accessing txp via printPacket. Preserve the existing success path, and ensure packet ownership cleanup occurs exactly once without releasing the packet again after beginSending() has already released it.
🤖 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.
Outside diff comments:
In `@src/mesh/RadioInterface.cpp`:
- Around line 1476-1480: Update RadioLibInterface::startSend() to immediately
detect a failed beginSending() result and return before calling startTransmit()
or accessing txp via printPacket. Preserve the existing success path, and ensure
packet ownership cleanup occurs exactly once without releasing the packet again
after beginSending() has already released it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 167bd282-1750-4605-a50c-b51d6e2a6ed1
📒 Files selected for processing (2)
src/mesh/RadioInterface.cpptest/test_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_radio/test_main.cpp
|
fix added for #11223 (review) , while it's outside of this PR, so now it's separated: #11229 Verification & Findings
Changes Made
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/mesh/RadioLibInterface.cpp`:
- Around line 775-777: Update the send flow around beginSending() so a zero-byte
result invokes the same TX-state cleanup and RX restart used by the
startTransmit() failure path after configHardwareForSend(). Preserve the
existing false return while ensuring TX power state is cleared before returning.
In `@src/platform/portduino/SimRadio.cpp`:
- Around line 210-212: Update SimRadio::startSend() to return a send-status
value consistent with RadioLibInterface::startSend(), propagating beginSending()
failure when it returns zero. In onNotify(), only call
RadioInterface::getPacketTime(txp) and schedule or log transmit airtime after
startSend() reports success, so released packets are not reused.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 41178263-6d5e-407c-adf5-28ea72692a56
📒 Files selected for processing (2)
src/mesh/RadioLibInterface.cppsrc/platform/portduino/SimRadio.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/platform/portduino/SimRadio.cpp`:
- Around line 188-194: Update SimRadio::startSend() to return true after the
successful transmission path so its caller-side status check is valid. Remove
the value-return from void SimRadio::unpackAndReceive(), leaving that function
without a returned value.
- Around line 210-216: Update the send path around beginSending and
packetPool.allocCopy in SimRadio so every failure releases txp when ownership
has been accepted and restores isReceiving before returning. Route both
failed-send branches through the existing send-completion or equivalent cleanup
mechanism, preserving normal handling when allocation succeeds.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d13333c9-356b-48a9-8105-c3a679825913
📒 Files selected for processing (3)
src/mesh/RadioLibInterface.cppsrc/platform/portduino/SimRadio.cppsrc/platform/portduino/SimRadio.h
58b33c7 to
8231bca
Compare
8231bca to
832d2b7
Compare
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/mesh/RadioLibInterface.cpp`:
- Around line 776-780: Update the zero-byte handling in the sending flow around
beginSending() so completeSending(), power-monitor cleanup, and startReceive()
run only when no transmission is already active. Preserve the existing
sendingPacket and radio state when beginSending() returns 0 because another
packet is in progress, while retaining cleanup for genuine failures without an
active transmission.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c589280-783b-428c-a522-688ddb165be0
📒 Files selected for processing (4)
src/mesh/MemoryPool.hsrc/mesh/RadioLibInterface.cppsrc/platform/portduino/SimRadio.cppsrc/platform/portduino/SimRadio.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/platform/portduino/SimRadio.h
- src/platform/portduino/SimRadio.cpp
- src/mesh/MemoryPool.h
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
checkov/CKV_SECRET_6 flags the commented-out "large4cats" example, which is the public default credential for the meshtastic.org MQTT broker rather than a real secret. Ignore the file in .trunk/trunk.yaml so the example config stays free of lint-suppression comments. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
d6f7206 to
7db47da
Compare
… aligns with latest develop
|
updated heap-freeing logs to align with #11374, that's much simpler and works just fine! |
fix test_radio code, fix QueStatus packet sending with proper clean-up
df86a4e to
c9ecb42
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/test_radio/test_main.cpp (1)
401-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the decorative test banner.
This three-line banner does not explain a non-obvious reason. Remove it.
As per coding guidelines: “Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious.”
🤖 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 `@test/test_radio/test_main.cpp` around lines 401 - 403, Remove the three-line decorative comment banner above the verification and stress tests, leaving the surrounding test code unchanged.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.
Inline comments:
In `@src/mesh/MeshService.cpp`:
- Around line 182-184: In the failed re-enqueue branch of
getNodenumFromRequestId, increment fromNum after packetPool.release(p) so
observers are notified of the dropped phone-bound packet, matching sendToPhone()
and reconcilePendingRxTimes().
In `@src/mesh/RadioLibInterface.cpp`:
- Around line 776-782: Update the rejected-send flow around beginSending() and
the numbytes == 0 branch so beacon-switch state is cleared and the radio is
restored before beginSending() releases txp. Ensure the fix handles a null
sendingPacket, clears the beacon sidecar entry, and invokes the existing radio
restoration path instead of relying on sendingPacket being present.
In `@test/test_radio/test_main.cpp`:
- Around line 417-420: The tests around beginSendingPublic() and
deliverToReceiverPublic() currently verify only return values and packet state;
add memaudit::snapshot() assertions for the pktpool(live) baseline before packet
allocation and confirm it is restored after each rejected-packet path at
test/test_radio/test_main.cpp lines 417-420, 429-433, and 441-450. Update all
three listed sites as applicable, while leaving the existing QueueStatus
eviction coverage unchanged.
---
Nitpick comments:
In `@test/test_radio/test_main.cpp`:
- Around line 401-403: Remove the three-line decorative comment banner above the
verification and stress tests, leaving the surrounding test code unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f0d0c174-032d-4d2e-ab0a-19a26eb4659f
📒 Files selected for processing (9)
src/mesh/MemoryPool.hsrc/mesh/MeshService.cppsrc/mesh/PhoneAPI.cppsrc/mesh/RadioInterface.cppsrc/mesh/RadioLibInterface.cppsrc/mesh/Router.cppsrc/modules/MeshBeaconModule.cppsrc/modules/PositionModule.cpptest/test_radio/test_main.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
This PR includes following fixes which applies to all platforms,
while it's part of enhancing supports trials on STM32WL platform.
some changes also requires updated test codes, and they're also in this PR as well.
acknoledgements: huge tnx for @ndoo and many others on Discord's #stm32
1.
MeshBeaconBroadcastModule::sendBeaconPacketPacket Leakrouter->send(p)was called directly insendBeaconPacket(). Whenrouter->send(p)returnedERRNO_SHOULD_RELEASE(e.g. for non-LoRa destinations or duty cycle limit drops),sendBeaconPacketignored the return code and failed to releasep.src/modules/MeshBeaconModule.cppto releasepwhenrouter->send(p)returnsERRNO_SHOULD_RELEASE.2.
NextHopRouter::sniffReceivedRelayed Packet LeakNextHopRouter::sniffReceivedallocated a relay copymeshtastic_MeshPacket *relay = packetPool.allocCopy(*p)and evaluatedRouter::send(relay) == ERRNO_OK. IfRouter::sendreturnedERRNO_SHOULD_RELEASE,relaywas leaked.src/mesh/NextHopRouter.cpp] to capture the return code and callpacketPool.release(relay)onERRNO_SHOULD_RELEASE.3. Static
MemoryPoolSafety & Double-Free Protection(p - pool)in static memory pool deallocation could lead to memory corruption if invalid or misaligned pointers were released or if a pointer was freed twice.src/mesh/MemoryPool.hwith pointer alignment validation (offset % sizeof(T) == 0) and double-free detection (if (!used[index]) return).4. Fixing logging format on data size-related logs(can be separate PR)
used Gemini 3.6 Flash via Antigravitiy 2.0, while it exhausted quota before finishing all task, had to do the rest by my own✋
🤝 Attestations
Summary by CodeRabbit