mirror/firmware
1
0
Fork 0
mirror of https://github.com/meshtastic/firmware.git synced 2026-05-19 06:14:12 -04:00
Code Issues Packages Projects Releases Wiki Activity

fix(Router): localize p_encrypted to prevent recursive-overwrite leak (#10311)

Browse Source 
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 <noreply@anthropic.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
This commit is contained in:
nightjoker7
2026-04-26 21:02:42 -05:00
committed by Ben Meadors
parent 4ccdd80090
commit 87f1f9d349
2 changed files with 7 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
src/mesh/Router.cpp
View File

@@ -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)

3
src/mesh/Router.h
View File

@@ -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;