From 87f1f9d349759d043d62647a2e44c2f953cc1372 Mon Sep 17 00:00:00 2001 From: nightjoker7 <47129685+nightjoker7@users.noreply.github.com> Date: Sun, 26 Apr 2026 21:02:42 -0500 Subject: [PATCH] fix(Router): localize p_encrypted to prevent recursive-overwrite leak (#10311) Router::handleReceived stores its allocCopy of the encrypted packet in the class member p_encrypted. callModules() invokes module replies that re-enter the router via MeshService::sendToMesh -> Router::sendLocal, which on a broadcast reply recursively calls handleReceived. The inner call overwrites the outer's p_encrypted without releasing it; on the way out it nulls the member, the outer release(p_encrypted) now releases nullptr, and the original allocation is permanently leaked. ~381 B per recursion. Promote p_encrypted to a function-local so each invocation owns its own copy for its full lifetime. The MQTT-publish null check at the call site (added by PR #9136 as a workaround for this bug) stays in place because allocCopy can still legitimately return nullptr on packetPool exhaustion. Copilot's review of PR #8999 (the original introduction) flagged this exact pattern at merge time: "Storing p_encrypted as a class member can cause issues with recursive or concurrent calls to handleReceived() since each call would overwrite the previous packet pointer." The historical reason for the member (S&F needing to retain the encrypted copy across calls) was satisfied differently by PR #9799 (ServerAPI converted to std::unique_ptr + cleanup on connection close), so the member is no longer load-bearing. Reproduces issues #9632 / #10101 / #8729 (heap leak when MeshMonitor connected; TCP drops on Station G2 / LILYGO ServerAPI dump abort). Hardware A/B on Station G2 under sustained TCP-API retry storm (open :4403, request config, disconnect mid-stream, repeat at ~0.6/s) - 9 min run: | Build | heapFree drift | rebootCount delta | | this patch | -1.5 KB (noise)| 0 | | stock 2.7.13 | -73 KB (8.1KB/min) | +1 (OOM crash) | Co-authored-by: Claude Opus 4.7 Co-authored-by: Ben Meadors --- src/mesh/Router.cpp | 11 +++++++---- src/mesh/Router.h | 3 --- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index b231261b5..eb5fd41ff 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -719,9 +719,13 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) // Also, we should set the time from the ISR and it should have msec level resolution p->rx_time = getValidTime(RTCQualityFromNet); // store the arrival timestamp for the phone - // Store a copy of encrypted packet for MQTT + // Store a copy of the encrypted packet for MQTT. + // Local, not a class member: handleReceived re-enters itself when a module + // reply broadcast goes through MeshService::sendToMesh -> Router::sendLocal, + // and a member would be silently overwritten without release on the inner + // call. Each invocation now owns its own copy (issue #9632, #10101, #8729). DEBUG_HEAP_BEFORE; - p_encrypted = packetPool.allocCopy(*p); + meshtastic_MeshPacket *p_encrypted = packetPool.allocCopy(*p); DEBUG_HEAP_AFTER("Router::handleReceived", p_encrypted); // Take those raw bytes and convert them back into a well structured protobuf we can understand @@ -815,8 +819,7 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) #endif } - packetPool.release(p_encrypted); // Release the encrypted packet - p_encrypted = nullptr; + packetPool.release(p_encrypted); // Release the encrypted packet (release() handles nullptr) } void Router::perhapsHandleReceived(meshtastic_MeshPacket *p) diff --git a/src/mesh/Router.h b/src/mesh/Router.h index 0f342d57b..bd4188693 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -92,9 +92,6 @@ class Router : protected concurrency::OSThread, protected PacketHistory before us */ uint32_t rxDupe = 0, txRelayCanceled = 0; - // pointer to the encrypted packet - meshtastic_MeshPacket *p_encrypted = nullptr; - protected: friend class RoutingModule;