From 6a77c9310f4a4c721bb2ffe10b7282eb89672ca3 Mon Sep 17 00:00:00 2001 From: nightjoker7 Date: Wed, 22 Apr 2026 22:03:18 -0500 Subject: [PATCH 1/4] Router::allocForSending: null-check packet pool allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit packetPool.allocZeroed() returns nullptr when the static MemoryPool (MAX_PACKETS slots) is exhausted — it logs a WARN and returns NULL rather than asserting. Every line after the allocation dereferences p->field, so an exhausted pool crashes instead of letting the caller handle the failure. Callers of allocForSending() already null-check the return value (e.g. RoutingModule::sendAckNak, PositionModule, TelemetryModule, admin paths), but we never reach those checks because we crash first. Under burst send conditions — eg. a node flooding traceroute responses, or multiple modules queueing replies simultaneously — the pool can genuinely run out. On hw71 (ROUTER_LATE, relaying traffic for 100+ neighbors) this is reachable. --- src/mesh/Router.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 836cd1a2291..a79ba1f0902 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -208,6 +208,10 @@ PacketId generatePacketId() meshtastic_MeshPacket *Router::allocForSending() { meshtastic_MeshPacket *p = packetPool.allocZeroed(); + if (!p) { + LOG_ERROR("allocForSending: packet pool exhausted"); + return nullptr; + } p->which_payload_variant = meshtastic_MeshPacket_decoded_tag; // Assume payload is decoded at start. p->from = nodeDB->getNodeNum(); From 931f2edeaad9b199ccd52e7e32b91c37f7668d72 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Thu, 23 Apr 2026 07:35:10 -0500 Subject: [PATCH 2/4] Update src/mesh/Router.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/mesh/Router.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index a79ba1f0902..d64b195fc7c 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -209,7 +209,6 @@ meshtastic_MeshPacket *Router::allocForSending() { meshtastic_MeshPacket *p = packetPool.allocZeroed(); if (!p) { - LOG_ERROR("allocForSending: packet pool exhausted"); return nullptr; } From 43bb5841f3df45c2a2ed687e1c02f2b59f557548 Mon Sep 17 00:00:00 2001 From: nightjoker7 Date: Thu, 23 Apr 2026 16:06:38 -0500 Subject: [PATCH 3/4] Audit allocForSending() callers + null-check unsafe ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback from @thebentern on PR #10261: 'I'm not sure every module actually handles nullptr gracefully.' Full audit of allocForSending() call sites: Already NULL-safe (no change): - modules/TraceRouteModule.cpp:591 (if (p) { ... }) - modules/TraceRouteModule.cpp:712 (if (p) { ... }) - modules/TrafficManagementModule.cpp:1237 (if (!reply) return false;) Were UNSAFE — would deref NULL on first line after alloc — now fixed: - mesh/MeshModule.cpp:59 (allocAckNak) → return nullptr - mesh/MeshModule.cpp:85 (allocErrorResponse → forwards from allocAckNak) → return nullptr - mesh/SinglePortModule.h:34 (allocDataPacket base) → return nullptr - modules/SerialModule.h:73 (allocDataPacket base) → return nullptr - modules/SerialModule.cpp:311 (sendTelemetry) → LOG_WARN + return - graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp:1995 (sendText) → LOG_WARN + return - mqtt/MQTT.cpp:191 (JSON downlink sendtext) → LOG_WARN + return - mqtt/MQTT.cpp:222 (JSON downlink sendposition) → LOG_WARN + return MeshModule::allocAckNak and ::allocErrorResponse change signatures semantically (may now return nullptr). All upstream callers I can find already check for nullptr before dereferencing the return (they use it with router->send() / packetPool.release() / service->sendToMesh() which tolerate NULL). So after this change: every allocForSending() caller EITHER early- returns on NULL, OR forwards NULL up its own call chain for the outer layer to handle. The remaining question 'does every consumer of the outer alloc function handle NULL' is beyond the scope of this PR — that's a caller-audit of allocDataPacket / allocAckNak / allocErrorResponse, not allocForSending itself. --- .../niche/InkHUD/Applets/System/Menu/MenuApplet.cpp | 4 ++++ src/mesh/MeshModule.cpp | 6 ++++++ src/mesh/SinglePortModule.h | 2 ++ src/modules/SerialModule.cpp | 4 ++++ src/modules/SerialModule.h | 2 ++ src/mqtt/MQTT.cpp | 8 ++++++++ 6 files changed, 26 insertions(+) diff --git a/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp b/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp index d489d21ee96..25734456e59 100644 --- a/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp +++ b/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp @@ -1993,6 +1993,10 @@ uint16_t InkHUD::MenuApplet::getSystemInfoPanelHeight() void InkHUD::MenuApplet::sendText(NodeNum dest, ChannelIndex channel, const char *message) { meshtastic_MeshPacket *p = router->allocForSending(); + if (!p) { + LOG_WARN("MenuApplet::sendText: packet pool exhausted, dropping message"); + return; + } p->decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; p->to = dest; p->channel = channel; diff --git a/src/mesh/MeshModule.cpp b/src/mesh/MeshModule.cpp index 83b64a8733d..efa59b6a76d 100644 --- a/src/mesh/MeshModule.cpp +++ b/src/mesh/MeshModule.cpp @@ -57,6 +57,10 @@ meshtastic_MeshPacket *MeshModule::allocAckNak(meshtastic_Routing_Error err, Nod // So we manually call pb_encode_to_bytes and specify routing port number // auto p = allocDataProtobuf(c); meshtastic_MeshPacket *p = router->allocForSending(); + if (!p) { + LOG_WARN("allocAckNak: packet pool exhausted, dropping ack/nak"); + return nullptr; + } p->decoded.portnum = meshtastic_PortNum_ROUTING_APP; p->decoded.payload.size = pb_encode_to_bytes(p->decoded.payload.bytes, sizeof(p->decoded.payload.bytes), &meshtastic_Routing_msg, &c); @@ -79,6 +83,8 @@ meshtastic_MeshPacket *MeshModule::allocErrorResponse(meshtastic_Routing_Error e uint8_t channelIndex = p->which_payload_variant == meshtastic_MeshPacket_decoded_tag ? p->channel : channels.getPrimaryIndex(); auto r = allocAckNak(err, getFrom(p), p->id, channelIndex); + if (!r) + return nullptr; setReplyTo(r, *p); diff --git a/src/mesh/SinglePortModule.h b/src/mesh/SinglePortModule.h index e43de09d12c..67444329acc 100644 --- a/src/mesh/SinglePortModule.h +++ b/src/mesh/SinglePortModule.h @@ -32,6 +32,8 @@ class SinglePortModule : public MeshModule { // Update our local node info with our position (even if we don't decide to update anyone else) meshtastic_MeshPacket *p = router->allocForSending(); + if (!p) + return nullptr; p->decoded.portnum = ourPortNum; return p; diff --git a/src/modules/SerialModule.cpp b/src/modules/SerialModule.cpp index 20d4d7d8c7e..f491c24e47d 100644 --- a/src/modules/SerialModule.cpp +++ b/src/modules/SerialModule.cpp @@ -309,6 +309,10 @@ int32_t SerialModule::runOnce() void SerialModule::sendTelemetry(meshtastic_Telemetry m) { meshtastic_MeshPacket *p = router->allocForSending(); + if (!p) { + LOG_WARN("SerialModule::sendTelemetry: packet pool exhausted, dropping"); + return; + } p->decoded.portnum = meshtastic_PortNum_TELEMETRY_APP; p->decoded.payload.size = pb_encode_to_bytes(p->decoded.payload.bytes, sizeof(p->decoded.payload.bytes), &meshtastic_Telemetry_msg, &m); diff --git a/src/modules/SerialModule.h b/src/modules/SerialModule.h index dbe4f75dbc3..768feeec632 100644 --- a/src/modules/SerialModule.h +++ b/src/modules/SerialModule.h @@ -71,6 +71,8 @@ class SerialModuleRadio : public MeshModule { // Update our local node info with our position (even if we don't decide to update anyone else) meshtastic_MeshPacket *p = router->allocForSending(); + if (!p) + return nullptr; p->decoded.portnum = ourPortNum; return p; diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index aba06c21005..49cb92648d3 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -189,6 +189,10 @@ inline void onReceiveJson(byte *payload, size_t length) // construct protobuf data packet using TEXT_MESSAGE, send it to the mesh meshtastic_MeshPacket *p = router->allocForSending(); + if (!p) { + LOG_WARN("MQTT downlink sendtext dropped: packet pool exhausted"); + return; + } p->decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; if (json.find("channel") != json.end() && json["channel"]->IsNumber() && (json["channel"]->AsNumber() < channels.getNumChannels())) @@ -220,6 +224,10 @@ inline void onReceiveJson(byte *payload, size_t length) // construct protobuf data packet using POSITION, send it to the mesh meshtastic_MeshPacket *p = router->allocForSending(); + if (!p) { + LOG_WARN("MQTT downlink sendposition dropped: packet pool exhausted"); + return; + } p->decoded.portnum = meshtastic_PortNum_POSITION_APP; if (json.find("channel") != json.end() && json["channel"]->IsNumber() && (json["channel"]->AsNumber() < channels.getNumChannels())) From ac1dddc2eac41b178422c997d57a6918bbe6e878 Mon Sep 17 00:00:00 2001 From: nightjoker7 Date: Fri, 24 Apr 2026 17:18:35 -0500 Subject: [PATCH 4/4] Address Copilot caller-audit follow-ups: ProtobufModule null-safe + sendtext leak + MeshModule dedupe log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot follow-up feedback after the allocForSending() caller audit. Three distinct issues it identified on call sites the audit didn't cover: 1. ProtobufModule::allocDataProtobuf() still assumed a non-null return from allocDataPacket() and dereferenced `p->decoded` without a check. The audit fixed the direct allocDataPacket callers but missed this helper that many modules layer on top. Propagate nullptr to the caller; every current caller either null-checks or forwards the return. 2. MQTT JSON downlink 'sendtext' path allocated a MeshPacket before validating payload length — if the payload was too long the LOG_WARN branch returned without releasing the packet, permanently reducing pool slots. Add packetPool.release(p) on the oversize drop. 3. MeshModule::allocAckNak added an unthrottled LOG_WARN on pool exhaustion. The allocator itself already LOG_WARNs, so this double-logs — worst-case during an ACK/NAK storm, which is exactly when the pool exhausts. Drop the redundant log. --- src/mesh/MeshModule.cpp | 4 +++- src/mesh/ProtobufModule.h | 8 +++++++- src/mqtt/MQTT.cpp | 4 ++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/mesh/MeshModule.cpp b/src/mesh/MeshModule.cpp index efa59b6a76d..7c95ac280fd 100644 --- a/src/mesh/MeshModule.cpp +++ b/src/mesh/MeshModule.cpp @@ -58,7 +58,9 @@ meshtastic_MeshPacket *MeshModule::allocAckNak(meshtastic_Routing_Error err, Nod // auto p = allocDataProtobuf(c); meshtastic_MeshPacket *p = router->allocForSending(); if (!p) { - LOG_WARN("allocAckNak: packet pool exhausted, dropping ack/nak"); + // Router::allocForSending / MemoryPool::alloc already LOG_WARN on exhaustion; + // don't double-log. An unthrottled WARN here would spam during an ACK/NAK storm + // — which is exactly the scenario that drives the pool to empty in the first place. return nullptr; } p->decoded.portnum = meshtastic_PortNum_ROUTING_APP; diff --git a/src/mesh/ProtobufModule.h b/src/mesh/ProtobufModule.h index 27e653efe2a..1eef1b5b63f 100644 --- a/src/mesh/ProtobufModule.h +++ b/src/mesh/ProtobufModule.h @@ -42,8 +42,14 @@ template class ProtobufModule : protected SinglePortModule */ meshtastic_MeshPacket *allocDataProtobuf(const T &payload) { - // Update our local node info with our position (even if we don't decide to update anyone else) + // allocDataPacket() now returns nullptr on packet-pool exhaustion (since + // Router::allocForSending was made null-safe in #10261). Propagate the nullptr + // to the caller rather than dereferencing `p->decoded`. All current callers + // either null-check the return or forward it to a helper that does — see the + // caller audit in the #10261 PR description. meshtastic_MeshPacket *p = allocDataPacket(); + if (!p) + return nullptr; p->decoded.payload.size = pb_encode_to_bytes(p->decoded.payload.bytes, sizeof(p->decoded.payload.bytes), fields, &payload); diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index 49cb92648d3..ebc277229c9 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -206,7 +206,11 @@ inline void onReceiveJson(byte *payload, size_t length) p->decoded.payload.size = jsonPayloadStr.length(); service->sendToMesh(p, RX_SRC_LOCAL); } else { + // Release the allocated packet back to the pool — `p` would otherwise leak, + // permanently reducing the number of available slots for every future + // send attempt. LOG_WARN("Received MQTT json payload too long, drop"); + packetPool.release(p); } } else if (json["type"]->AsString().compare("sendposition") == 0 && json["payload"]->IsObject()) { // invent the "sendposition" type for a valid envelope