fix(mesh): don't assert on malloc() failure in MemoryDynamic::alloc() - #11197
Conversation
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>
📝 WalkthroughWalkthroughAllocation failures now log or return safely in the memory pool, UDP multicast and MQTT receive handlers, and simulated radio receive setup. ChangesMemory allocation handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
|
⚡ 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 (31)
Build artifacts expire on 2026-08-23. Updated for |
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>
|
Audited every `packetPool` allocation call site in the codebase for null-safety before/after this change (the only pool this touches - it's the sole `MemoryDynamic` instantiation). Found and fixed two spots that were missed by the earlier #10948/#10951 hardening because they use the `UniquePacketPoolPacket` (unique_ptr) wrapper instead of a raw pointer, so they didn't match the grep patterns those PRs likely used:
Both were harmless before this PR only because `MemoryDynamic::alloc()`'s `assert(p)` made a null result unreachable (it would hang the whole allocating thread first). With that assert removed, these two would have been genuine null-pointer-dereference regressions on any `MemoryDynamic`-backed platform (portduino, STM32WL, BOARD_HAS_PSRAM) under real heap exhaustion. Fixed in the follow-up commit, build-verified on `wio-e5` and (for the MQTT/UDP-multicast code paths specifically) `tbeam0_7`. Every other `packetPool.allocCopy()`/`allocZeroed()` call site I found was already correctly null-checked (either directly, or via a guarded read of the stored result downstream, e.g. the Telemetry modules' `lastMeasurementPacket`). |
|
@coderabbitai review and you have to add the summary again in the PR body as I have updated it (which removed your review) |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="meshtastic/firmware"
pr="11197"
echo "=== PR metadata and current body ==="
gh pr view "$pr" --repo "$repo" --json number,title,body,baseRefName,headRefName,files
echo
echo "=== PR diff ==="
gh pr diff "$pr" --repo "$repo" --patch
echo
echo "=== MemoryPool declarations and allocation helpers ==="
ast-grep outline src/mesh/MemoryPool.h --items all --view expanded
rg -n -A45 -B12 'alloc\(|allocUnique|MemoryDynamic|MemoryPool' src/mesh/MemoryPool.h
echo
echo "=== UDP multicast allocation and use context ==="
ast-grep outline src/mesh/udp/UdpMulticastHandler.h --items all --view expanded
rg -n -A55 -B15 'allocUniqueCopy|onReceive' src/mesh/udp/UdpMulticastHandler.h
echo
echo "=== MQTT downlink allocation and use context ==="
ast-grep outline src/mqtt/MQTT.cpp --items all --view expanded
rg -n -A70 -B15 'allocUniqueZeroed|onReceive' src/mqtt/MQTT.cppLength of output: 32723 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="meshtastic/firmware"
pr="11197"
echo "=== MemoryDynamic instantiations and packetPool construction ==="
rg -n -A10 -B10 --glob '*.{h,hpp,cpp,cxx,ino}' \
'MemoryDynamic\s*<|packetPool\s*(=|\(|\{)' src
echo
echo "=== packetPool allocation call sites ==="
rg -n -A4 -B4 --glob '*.{h,hpp,cpp,cxx,ino}' \
'\bpacketPool\s*\.\s*alloc(?:Copy|Zeroed|UniqueCopy|UniqueZeroed)\s*\(' src
echo
echo "=== Appending CodeRabbit summary while preserving the author body ==="
body_file="$(mktemp)"
trap 'rm -f "$body_file"' EXIT
gh pr view "$pr" --repo "$repo" --json body --jq .body > "$body_file"
if ! grep -q '^## CodeRabbit summary$' "$body_file"; then
cat >> "$body_file" <<'EOF'
## CodeRabbit summary
`MemoryDynamic<T>::alloc()` no longer asserts after `malloc(sizeof(T))`. On allocation failure, it logs a heap-exhaustion warning and returns `nullptr`, matching the failure contract already used by `MemoryPool<T, MaxSize>::alloc()` and allowing caller-side null checks to handle OOM safely.
The PR also guards the two `UniquePacketPoolPacket` allocation paths that would otherwise dereference a null allocation:
- `UdpMulticastHandler::onReceive()` drops an incoming UDP multicast packet when `packetPool.allocUniqueCopy()` fails.
- MQTT downlink handling drops the message when `packetPool.allocUniqueZeroed()` fails.
Successful allocations retain the existing memory-audit accounting and ownership-transfer behavior. No public API declarations changed.
EOF
fi
gh pr edit "$pr" --repo "$repo" --body-file "$body_file"
echo
echo "=== Confirmed PR body summary section ==="
gh pr view "$pr" --repo "$repo" --json body --jq '.body | split("## CodeRabbit summary") | .[-1]'Length of output: 32573 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== SimRadio allocation-result control flow ==="
ast-grep outline src/platform/portduino/SimRadio.cpp --items all --view expanded
sed -n '300,385p' src/platform/portduino/SimRadio.cpp
echo
echo "=== Router::send allocation-result control flow ==="
sed -n '450,525p' src/mesh/Router.cpp
echo
echo "=== Router::handleReceived allocation-result control flow ==="
sed -n '1200,1265p' src/mesh/Router.cpp
echo
echo "=== All direct packetPool allocations assigned without an immediately visible guard ==="
rg -n -A25 -B3 --glob '*.{h,hpp,cpp,cxx,ino}' \
'(?:auto|meshtastic_MeshPacket\s*\*)\s+\w+\s*=\s*packetPool\s*\.\s*alloc(?:Copy|Zeroed)\s*\(' srcLength of output: 19377
The two newly changed unique-pointer call sites are correctly guarded, and the other inspected I could not restore the PR-body summary: the workflow blocked ## CodeRabbit summary
`MemoryDynamic<T>::alloc()` no longer asserts after `malloc(sizeof(T))`. On allocation failure, it logs a heap-exhaustion warning and returns `nullptr`, matching the failure contract already used by `MemoryPool<T, MaxSize>::alloc()` and allowing caller-side null checks to handle OOM safely.
The PR also guards the two `UniquePacketPoolPacket` allocation paths that would otherwise dereference a null allocation:
- `UdpMulticastHandler::onReceive()` drops an incoming UDP multicast packet when `packetPool.allocUniqueCopy()` fails.
- MQTT downlink handling drops the message when `packetPool.allocUniqueZeroed()` fails.
Successful allocations retain the existing memory-audit accounting and ownership-transfer behavior. No public API declarations changed.✅ Action performedReview finished.
|
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>
|
Fixed in 9c317f4 — `startReceive()` no longer sets `isReceiving = true` until after confirming `packetPool.allocCopy()` succeeded, for both the collision-emulation and non-emulation branches. Verified with `pio run -e native-macos` (portduino, which also uses the `MemoryDynamic` packetPool and links in SimRadio.cpp/MQTT.cpp/UdpMulticastHandler.h). |
|
@coderabbitai review Give me a new summary too since there's more changes |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="meshtastic/firmware"
pr="11197"
echo "=== PR metadata, current body, and changed files ==="
gh pr view "$pr" --repo "$repo" --json number,title,body,baseRefName,headRefName,commits,files
echo
echo "=== Current PR diff ==="
gh pr diff "$pr" --repo "$repo" --patch
echo
echo "=== Changed-file structure ==="
for file in \
src/mesh/MemoryPool.h \
src/mesh/udp/UdpMulticastHandler.h \
src/mqtt/MQTT.cpp \
src/platform/portduino/SimRadio.cpp
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view expanded
fi
done
echo
echo "=== Allocation and OOM-control-flow contexts ==="
rg -n -A55 -B15 \
'MemoryDynamic|alloc\(|allocUniqueCopy|allocUniqueZeroed|startReceive|isReceiving|receivingPacket|handleReceiveInterrupt' \
src/mesh/MemoryPool.h \
src/mesh/udp/UdpMulticastHandler.h \
src/mqtt/MQTT.cpp \
src/platform/portduino/SimRadio.cppLength of output: 63577 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="meshtastic/firmware"
pr="11197"
echo "=== Full receive-state lifecycle in SimRadio ==="
sed -n '145,205p' src/platform/portduino/SimRadio.cpp
sed -n '300,381p' src/platform/portduino/SimRadio.cpp
echo
echo "=== All isReceiving writes and receive-handler call sites ==="
rg -n -A8 -B8 \
'\bisReceiving\s*=|handleReceiveInterrupt\s*\(' \
src/platform/portduino/SimRadio.cpp
echo
echo "=== Base-branch context for the receive handler ==="
git show "origin/develop:src/platform/portduino/SimRadio.cpp" | sed -n '300,381p'
echo
echo "=== MemoryDynamic-backed packetPool selection ==="
rg -n -A16 -B8 \
'MemoryDynamic\s*<|packetPool' \
src/mesh/Router.cppLength of output: 29202 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="meshtastic/firmware"
pr="11197"
body_file="$(mktemp)"
trap 'rm -f "$body_file"' EXIT
gh pr view "$pr" --repo "$repo" --json body --jq '.body' > "$body_file"
python3 - "$body_file" <<'PY'
from pathlib import Path
import sys
path = Path(sys.argv[1])
body = path.read_text()
heading = "## CodeRabbit review summary"
summary = """## CodeRabbit review summary
This PR makes heap-backed packet allocation recoverable under out-of-memory conditions:
- `MemoryDynamic<T>::alloc()` now logs a warning and returns `nullptr` when `malloc()` fails, instead of asserting. Successful allocations retain their existing memory-audit accounting.
- UDP multicast reception and MQTT downlink handling now drop the incoming message if their `UniquePacketPoolPacket` allocation fails, rather than dereferencing a null unique pointer.
- `SimRadio::startReceive()` now allocates the receive packet before setting `isReceiving` and before scheduling or handling reception. This prevents a failed initial allocation from leaving the simulated radio marked as busy.
The allocation behavior is relevant to the dynamic `packetPool` configurations used by Portduino, STM32WL, and PSRAM-enabled boards. The author reports successful builds for `wio-e5`, `tbeam0_7`, and `native-macos`; physical OOM validation remains pending.
No public API declarations changed.
"""
if heading in body:
start = body.index(heading)
# Preserve content preceding this maintained summary, replacing its old text.
body = body[:start].rstrip() + "\n\n" + summary
else:
body = body.rstrip() + "\n\n" + summary
path.write_text(body)
PY
gh pr edit "$pr" --repo "$repo" --body-file "$body_file"
echo "=== Confirmed updated summary ==="
gh pr view "$pr" --repo "$repo" --json body --jq '.body | split("## CodeRabbit review summary") | .[-1]'Length of output: 5338
I attempted to add the expanded summary to the PR body, but ## CodeRabbit review summary
This PR makes heap-backed packet allocation recoverable under out-of-memory conditions:
- `MemoryDynamic<T>::alloc()` now logs a warning and returns `nullptr` when `malloc()` fails, instead of asserting. Successful allocations retain their existing memory-audit accounting.
- UDP multicast reception and MQTT downlink handling now drop the incoming message if their `UniquePacketPoolPacket` allocation fails, rather than dereferencing a null unique pointer.
- `SimRadio::startReceive()` now allocates the receive packet before setting `isReceiving` and before scheduling or handling reception. This prevents a failed initial allocation from leaving the simulated radio marked as busy.
The allocation behavior is relevant to the dynamic `packetPool` configurations used by Portduino, STM32WL, and PSRAM-enabled boards. The author reports successful builds for `wio-e5`, `tbeam0_7`, and `native-macos`; physical OOM validation remains pending.
No public API declarations changed.✅ Action performedReview finished.
|
|
@coderabbitai added your summary verbatim. |
|
Tip For best results, initiate chat on the files or code changes.
|
abf217c
…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>
2 participants
Problem
MemoryDynamic<T>::alloc()(src/mesh/MemoryPool.h) does:#10948/#10951null-checkedallocCopy()/allocZeroed()call sites so callers skip the send/reply/notification on allocation failure instead of dereferencing null. That hardening is unreachable on real OOM here:assert(p)fires one level down, insidealloc(), before any caller sees a return value. The staticMemoryPool<T, MaxSize>::alloc()in the same file returnsnullptrand logs a warning on exhaustion instead.On
ARCH_STM32WL/BOARD_HAS_PSRAM(packetPoolis heap-backed viaMemoryDynamicthere — seesrc/mesh/Router.cpp:48-57),assert()failures route through a platform handler. On STM32WL that handler is an infinitewhile(true);loop (src/platform/stm32wl/main-stm32wl.cpp), not an abort or reset, so a realmalloc()failure hangs the calling thread instead of returningnullptr.Reproduced on wio-e5 hardware: a burst of incoming DMs drained free heap toward zero (
LOG_HEAPshowing ~1.7-1.8KB free right before the hang), then the device locked up.Fix
src/mesh/MemoryPool.h:MemoryDynamic::alloc()returnsnullptrand logs a warning onmalloc()failure, matchingMemoryPool::alloc().src/mesh/udp/UdpMulticastHandler.h,src/mqtt/MQTT.cpp: null-check the twoallocUniqueCopy()/allocUniqueZeroed()call sites that dereferenced the result unconditionally. These were unreachable with a null result before this PR (theassertabove made it impossible), so fix(router): null-check packetPool/clientNotificationPool allocations #10948/fix(modules): null-check packetPool/clientNotificationPool/mqttClientProxyMessagePool allocations #10951's audit of raw-pointer call sites didn't cover them.Audited every
packetPoolallocation call site in the codebase (the only poolMemoryDynamicbacks) for null-safety; the two above were the only unguarded ones.Test plan
pio run -e wio-e5— exercises theMemoryPool.hchange.pio run -e tbeam0_7— exercises the MQTT/UDP-multicast paths (excluded from stm32wl builds).Related: #11196.
🤝 Attestations
CodeRabbit review summary
This PR makes heap-backed packet allocation recoverable under out-of-memory conditions:
MemoryDynamic<T>::alloc()now logs a warning and returnsnullptrwhenmalloc()fails, instead of asserting. Successful allocations retain their existing memory-audit accounting.UniquePacketPoolPacketallocation fails, rather than dereferencing a null unique pointer.SimRadio::startReceive()now allocates the receive packet before settingisReceivingand before scheduling or handling reception. This prevents a failed initial allocation from leaving the simulated radio marked as busy.The allocation behavior is relevant to the dynamic
packetPoolconfigurations used by Portduino, STM32WL, and PSRAM-enabled boards. The author reports successful builds forwio-e5,tbeam0_7, andnative-macos; physical OOM validation remains pending.No public API declarations changed.
Summary by CodeRabbit