fix(router): null-check packetPool/clientNotificationPool allocations - #10948
Merged
thebentern merged 1 commit intoJul 9, 2026
Merged
Conversation
On platforms where these pools are heap-backed (MemoryDynamic - used whenever there isn't enough static RAM for a fixed pool, e.g. ARCH_STM32WL or BOARD_HAS_PSRAM), allocCopy()/allocZeroed() return nullptr on allocation failure and already log a warning, but most callers dereferenced the result unconditionally. Under real heap pressure this reliably produced a HardFault - reproduced on STM32WL hardware under sustained mesh traffic, including the RX entry point (RadioLibInterface::handleReceiveInterrupt) where every received packet is allocated. Adds null checks at all call sites that were missing one, mirroring the guard pattern already used correctly elsewhere in the same files (e.g. RadioInterface.cpp's sendErrorNotification). On allocation failure, callers now skip the send/retransmission/notification and log nothing further (the allocator already did) rather than crash. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds null-pointer guards after packet, queue-status, and client-notification pool allocation calls (allocCopy/allocZeroed) across MeshService, NextHopRouter, ReliableRouter, NodeDB, PhoneAPI, RadioLibInterface, and Router, preventing sends or dereferences when allocation fails. ChangesAllocation failure guards
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (27)
Build artifacts expire on 2026-08-07. Updated for |
ndoo
marked this pull request as ready for review
July 8, 2026 23:47
9 tasks
Contributor
Firmware Size Report42 targets | vs
Show 37 more target(s)
Size budgets
Budgets live in Updated for 96be2ca |
thebentern
added a commit
that referenced
this pull request
Jul 9, 2026
…ProxyMessagePool allocations (#10951) * fix(modules): null-check packetPool/clientNotificationPool/mqttClientProxyMessagePool allocations Follow-up to fix/meshpacket-alloc-null-checks (PR #10948), which fixed the core routing path (Router/NextHopRouter/ReliableRouter/ MeshService/RadioLibInterface) after reproducing a HardFault on STM32WL hardware under real mesh traffic. This covers the same allocCopy()/ allocZeroed() unchecked-return pattern in lower-frequency paths that were out of scope for that PR: SimRadio.cpp (portduino sim RX/TX), NodeInfoModule, the Telemetry modules' power-saving-sleep notifications, MQTT (map report, client-proxy messages, config-validation notifications), PositionModule, SerialModule, and KeyVerificationModule. Sites where the allocation result was already read back through an existing null check downstream (e.g. Telemetry's lastMeasurementPacket, SimRadio's receivingPacket, AdminModule::sendWarning) were left as-is - verified safe, not touched. Router::allocForSending() (and everything that funnels through it, e.g. allocDataProtobuf()) remains unguarded and out of scope here too - it's called from 30+ sites across the codebase and needs its own audit. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(KeyVerificationModule): bound cn->message writes with snprintf CodeRabbit flagged unbounded sprintf() writes into the fixed-size ClientNotification.message buffer as a static-analysis nitpick while reviewing this PR's allocation null-checks. Align these four sites with the snprintf(dest, sizeof(dest), ...) pattern already used for the same field elsewhere in this PR (SerialModule.cpp, MQTT.cpp). Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
11 tasks
ndoo
added a commit
to meshmy/meshtastic-firmware
that referenced
this pull request
Jul 24, 2026
meshtastic#10948/meshtastic#10951 null-checked every raw-pointer packetPool.allocCopy()/ allocZeroed() call site, but missed the two spots using the UniquePacketPoolPacket (unique_ptr) wrapper: UdpMulticastHandler::onReceive() and MQTT::onReceive() both dereferenced the allocation result unconditionally. Previously unreachable in practice: MemoryDynamic::alloc() asserted before ever returning null, so nothing downstream saw it. Fixed alongside that assert removal (this branch) since these two are now reachable with a genuine null - same fix as everywhere else, just an unchecked pointer that turns up under OOM. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
t-miura
pushed a commit
to t-miura/firmware
that referenced
this pull request
Jul 25, 2026
MemoryDynamic<T>::alloc() called assert(p) right after malloc(), instead of returning nullptr like the static MemoryPool<T,N>::alloc() already does on exhaustion. All of packetPool's callers were already hardened to null-check allocCopy()/allocZeroed() (meshtastic#10948, meshtastic#10951), but that path is unreachable on real OOM here: assert() fires first, one level down, before the caller ever gets a chance to check anything. On STM32WL (packetPool is MemoryDynamic there - not enough static RAM for the fixed pool), assert() failures are wrapped to an infinite `while(true);` loop rather than aborting or resetting, so the thread just hangs forever instead of returning nullptr. Reproduced on wio-e5 hardware under a burst of incoming DMs, with free heap draining toward zero right before the hang. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
jeremiah-k
pushed a commit
to jeremiah-k/meshtastic-firmware
that referenced
this pull request
Jul 25, 2026
…meshtastic#11197) * fix(mesh): don't assert on malloc() failure in MemoryDynamic::alloc() MemoryDynamic<T>::alloc() called assert(p) right after malloc(), instead of returning nullptr like the static MemoryPool<T,N>::alloc() already does on exhaustion. All of packetPool's callers were already hardened to null-check allocCopy()/allocZeroed() (meshtastic#10948, meshtastic#10951), but that path is unreachable on real OOM here: assert() fires first, one level down, before the caller ever gets a chance to check anything. On STM32WL (packetPool is MemoryDynamic there - not enough static RAM for the fixed pool), assert() failures are wrapped to an infinite `while(true);` loop rather than aborting or resetting, so the thread just hangs forever instead of returning nullptr. Reproduced on wio-e5 hardware under a burst of incoming DMs, with free heap draining toward zero right before the hang. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix: null-check the two allocUnique*() call sites missed by prior audits meshtastic#10948/meshtastic#10951 null-checked every raw-pointer packetPool.allocCopy()/ allocZeroed() call site, but missed the two spots using the UniquePacketPoolPacket (unique_ptr) wrapper: UdpMulticastHandler::onReceive() and MQTT::onReceive() both dereferenced the allocation result unconditionally. Previously unreachable in practice: MemoryDynamic::alloc() asserted before ever returning null, so nothing downstream saw it. Fixed alongside that assert removal (this branch) since these two are now reachable with a genuine null - same fix as everywhere else, just an unchecked pointer that turns up under OOM. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(SimRadio): don't leave isReceiving stuck true on allocation failure startReceive() set isReceiving = true before allocCopy(), so a failed allocation (now reachable with a real nullptr instead of hanging in assert()) left the simulated radio permanently marked as receiving: handleReceiveInterrupt() returns immediately on a null receivingPacket, before ever reaching the code that would clear isReceiving. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
madeofstown
pushed a commit
to madeofstown/meshtastic-firmware
that referenced
this pull request
Jul 26, 2026
…meshtastic#11197) * fix(mesh): don't assert on malloc() failure in MemoryDynamic::alloc() MemoryDynamic<T>::alloc() called assert(p) right after malloc(), instead of returning nullptr like the static MemoryPool<T,N>::alloc() already does on exhaustion. All of packetPool's callers were already hardened to null-check allocCopy()/allocZeroed() (meshtastic#10948, meshtastic#10951), but that path is unreachable on real OOM here: assert() fires first, one level down, before the caller ever gets a chance to check anything. On STM32WL (packetPool is MemoryDynamic there - not enough static RAM for the fixed pool), assert() failures are wrapped to an infinite `while(true);` loop rather than aborting or resetting, so the thread just hangs forever instead of returning nullptr. Reproduced on wio-e5 hardware under a burst of incoming DMs, with free heap draining toward zero right before the hang. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix: null-check the two allocUnique*() call sites missed by prior audits meshtastic#10948/meshtastic#10951 null-checked every raw-pointer packetPool.allocCopy()/ allocZeroed() call site, but missed the two spots using the UniquePacketPoolPacket (unique_ptr) wrapper: UdpMulticastHandler::onReceive() and MQTT::onReceive() both dereferenced the allocation result unconditionally. Previously unreachable in practice: MemoryDynamic::alloc() asserted before ever returning null, so nothing downstream saw it. Fixed alongside that assert removal (this branch) since these two are now reachable with a genuine null - same fix as everywhere else, just an unchecked pointer that turns up under OOM. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(SimRadio): don't leave isReceiving stuck true on allocation failure startReceive() set isReceiving = true before allocCopy(), so a failed allocation (now reachable with a real nullptr instead of hanging in assert()) left the simulated radio permanently marked as receiving: handleReceiveInterrupt() returns immediately on a null receivingPacket, before ever reaching the code that would clear isReceiving. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
9 tasks
Itzdavid01
pushed a commit
to Itzdavid01/firmware
that referenced
this pull request
Sep 5, 2026
…meshtastic#10948) On platforms where these pools are heap-backed (MemoryDynamic - used whenever there isn't enough static RAM for a fixed pool, e.g. ARCH_STM32WL or BOARD_HAS_PSRAM), allocCopy()/allocZeroed() return nullptr on allocation failure and already log a warning, but most callers dereferenced the result unconditionally. Under real heap pressure this reliably produced a HardFault - reproduced on STM32WL hardware under sustained mesh traffic, including the RX entry point (RadioLibInterface::handleReceiveInterrupt) where every received packet is allocated. Adds null checks at all call sites that were missing one, mirroring the guard pattern already used correctly elsewhere in the same files (e.g. RadioInterface.cpp's sendErrorNotification). On allocation failure, callers now skip the send/retransmission/notification and log nothing further (the allocator already did) rather than crash. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
Itzdavid01
pushed a commit
to Itzdavid01/firmware
that referenced
this pull request
Sep 5, 2026
…ProxyMessagePool allocations (meshtastic#10951) * fix(modules): null-check packetPool/clientNotificationPool/mqttClientProxyMessagePool allocations Follow-up to fix/meshpacket-alloc-null-checks (PR meshtastic#10948), which fixed the core routing path (Router/NextHopRouter/ReliableRouter/ MeshService/RadioLibInterface) after reproducing a HardFault on STM32WL hardware under real mesh traffic. This covers the same allocCopy()/ allocZeroed() unchecked-return pattern in lower-frequency paths that were out of scope for that PR: SimRadio.cpp (portduino sim RX/TX), NodeInfoModule, the Telemetry modules' power-saving-sleep notifications, MQTT (map report, client-proxy messages, config-validation notifications), PositionModule, SerialModule, and KeyVerificationModule. Sites where the allocation result was already read back through an existing null check downstream (e.g. Telemetry's lastMeasurementPacket, SimRadio's receivingPacket, AdminModule::sendWarning) were left as-is - verified safe, not touched. Router::allocForSending() (and everything that funnels through it, e.g. allocDataProtobuf()) remains unguarded and out of scope here too - it's called from 30+ sites across the codebase and needs its own audit. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(KeyVerificationModule): bound cn->message writes with snprintf CodeRabbit flagged unbounded sprintf() writes into the fixed-size ClientNotification.message buffer as a static-analysis nitpick while reviewing this PR's allocation null-checks. Align these four sites with the snprintf(dest, sizeof(dest), ...) pattern already used for the same field elsewhere in this PR (SerialModule.cpp, MQTT.cpp). Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> --------- Signed-off-by: Andrew Yong <me@ndoo.sg> Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
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
2 participants
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.
On very low memory platforms (namely STM32WL), devices reliably HardFault after some time (an hour to a few hours, depending on mesh activity), including at the RX entry point (
RadioLibInterface::handleReceiveInterrupt) where every received MeshPacket is allocated.Allocator<T>::allocCopy()/allocZeroed()would returnnullptrand log a warning when the pool is heap-backed (MemoryDynamic— used onARCH_STM32WLandBOARD_HAS_PSRAMtargets because there isn't enough static RAM for a fixed pool) and the underlyingmalloc()fails.Most callers across
Router.cpp,NextHopRouter.cpp,ReliableRouter.cpp,MeshService.cpp,PhoneAPI.cpp,NodeDB.cpp, andRadioLibInterface.cppdereferenced the result unconditionally, which results in a HardFault when trying to access the packet in the lines of code after.This PR adds null checks at every missed call site, matching the guard already used correctly elsewhere in the same files (e.g.
RadioInterface.cpp'ssendErrorNotification). On allocation failure, callers now skip the send/retransmission/notification instead of crashing.🤝 Attestations
Tested on STM32WL (rak3172-based custom hardware) under real mesh traffic, where this was reproduced as a live HardFault crash and confirmed fixed via multi-hour on-device soak testing. Not yet tested on the devices listed above, nor on PSRAM devices.
Summary by CodeRabbit