diff --git a/src/mesh/NextHopRouter.cpp b/src/mesh/NextHopRouter.cpp index f33de779c..47cc4ebd6 100644 --- a/src/mesh/NextHopRouter.cpp +++ b/src/mesh/NextHopRouter.cpp @@ -159,6 +159,9 @@ bool NextHopRouter::perhapsRebroadcast(const meshtastic_MeshPacket *p) } #endif + if (p->to == NODENUM_BROADCAST_NO_LORA) + return false; + // Allow rebroadcast if hop_limit > 0 OR if we're exhausting hops (which sets hop_limit = 0 but still needs one relay) if (!isToUs(p) && !isFromUs(p) && (p->hop_limit > 0 || exhaustHops)) { if (p->id != 0) { @@ -192,11 +195,10 @@ bool NextHopRouter::perhapsRebroadcast(const meshtastic_MeshPacket *p) } #endif - if (p->next_hop == NO_NEXT_HOP_PREFERENCE) { - FloodingRouter::send(tosend); - } else { - NextHopRouter::send(tosend); - } + ErrorCode res = + (p->next_hop == NO_NEXT_HOP_PREFERENCE) ? FloodingRouter::send(tosend) : NextHopRouter::send(tosend); + if (res == ERRNO_SHOULD_RELEASE) + packetPool.release(tosend); return true; } @@ -401,8 +403,10 @@ int32_t NextHopRouter::doRetransmissions() trafficManagementModule->clearNextHop(p.packet->to); } #endif - if (auto *copy = packetPool.allocCopy(*p.packet)) - FloodingRouter::send(copy); + if (auto *copy = packetPool.allocCopy(*p.packet)) { + if (FloodingRouter::send(copy) == ERRNO_SHOULD_RELEASE) + packetPool.release(copy); + } } else { #if NEXTHOP_EARLY_FLOOD_ON_UNVERIFIED // M4 (gated): if the route isn't proven healthy, don't spend a second directed @@ -416,22 +420,30 @@ int32_t NextHopRouter::doRetransmissions() meshtastic_NodeInfoLite *sentTo = nodeDB->getMeshNode(p.packet->to); if (sentTo) sentTo->next_hop = NO_NEXT_HOP_PREFERENCE; - if (auto *copy = packetPool.allocCopy(*p.packet)) - FloodingRouter::send(copy); + if (auto *copy = packetPool.allocCopy(*p.packet)) { + if (FloodingRouter::send(copy) == ERRNO_SHOULD_RELEASE) + packetPool.release(copy); + } } else { - if (auto *copy = packetPool.allocCopy(*p.packet)) - NextHopRouter::send(copy); + if (auto *copy = packetPool.allocCopy(*p.packet)) { + if (NextHopRouter::send(copy) == ERRNO_SHOULD_RELEASE) + packetPool.release(copy); + } } #else - if (auto *copy = packetPool.allocCopy(*p.packet)) - NextHopRouter::send(copy); + if (auto *copy = packetPool.allocCopy(*p.packet)) { + if (NextHopRouter::send(copy) == ERRNO_SHOULD_RELEASE) + packetPool.release(copy); + } #endif } } else { // Note: we call the superclass version because we don't want to have our version of send() add a new // retransmission record - if (auto *copy = packetPool.allocCopy(*p.packet)) - FloodingRouter::send(copy); + if (auto *copy = packetPool.allocCopy(*p.packet)) { + if (FloodingRouter::send(copy) == ERRNO_SHOULD_RELEASE) + packetPool.release(copy); + } } // Queue again diff --git a/src/mesh/NextHopRouter.h b/src/mesh/NextHopRouter.h index 467d9ca68..e826bda2c 100644 --- a/src/mesh/NextHopRouter.h +++ b/src/mesh/NextHopRouter.h @@ -205,7 +205,11 @@ class NextHopRouter : public FloodingRouter */ std::optional getNextHop(NodeNum to, uint8_t relay_node); +#ifdef PIO_UNIT_TESTING + public: // expose perhapsRebroadcast to the test shim +#else private: +#endif /** Check if we should be rebroadcasting this packet if so, do so. * @return true if we did rebroadcast */ bool perhapsRebroadcast(const meshtastic_MeshPacket *p) override; diff --git a/src/modules/RoutingModule.cpp b/src/modules/RoutingModule.cpp index a46323ac6..aba0751f3 100644 --- a/src/modules/RoutingModule.cpp +++ b/src/modules/RoutingModule.cpp @@ -55,7 +55,8 @@ void RoutingModule::sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketI // Allow the caller to set want_ack on this ACK packet if it's important that the ACK be delivered reliably p->want_ack = ackWantsAck; - router->sendLocal(p); // we sometimes send directly to the local node + if (router->sendLocal(p) == ERRNO_SHOULD_RELEASE) // we sometimes send directly to the local node + packetPool.release(p); } uint8_t RoutingModule::getHopLimitForResponse(const meshtastic_MeshPacket &mp) diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index 7e3d527ee..fedafb48e 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -122,7 +122,8 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length) if (isFromUs(e.packet)) { auto pAck = routingModule->allocAckNak(meshtastic_Routing_Error_NONE, getFrom(e.packet), e.packet->id, ch.index); pAck->transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT; - router->sendLocal(pAck); + if (router->sendLocal(pAck) == ERRNO_SHOULD_RELEASE) + packetPool.release(pAck); } else { LOG_INFO("Ignore downlink message we originally sent"); } diff --git a/test/test_nexthop_routing/test_main.cpp b/test/test_nexthop_routing/test_main.cpp index b3964eb25..c7a27bf5c 100644 --- a/test/test_nexthop_routing/test_main.cpp +++ b/test/test_nexthop_routing/test_main.cpp @@ -15,6 +15,7 @@ #include "gps/RTC.h" #include "mesh/NextHopRouter.h" #include "mesh/NodeDB.h" +#include "mesh/RadioInterface.h" #include #include #include @@ -87,6 +88,7 @@ class NextHopRouterTestShim : public NextHopRouter using NextHopRouter::noteRouteFailure; using NextHopRouter::noteRouteLearned; using NextHopRouter::noteRouteSuccess; + using NextHopRouter::perhapsRebroadcast; using Router::shouldDecrementHopLimit; // protected in Router void resetRouteHealthForTest() @@ -96,6 +98,34 @@ class NextHopRouterTestShim : public NextHopRouter } }; +// --------------------------------------------------------------------------- +// MockRadioInterface - mirrors RadioLibInterface::send()'s NODENUM_BROADCAST_NO_LORA branch, which +// returns ERRNO_SHOULD_RELEASE without releasing. +// --------------------------------------------------------------------------- +class MockRadioInterface : public RadioInterface +{ + public: + ErrorCode send(meshtastic_MeshPacket *p) override + { + sendCount++; + if (declineAll || p->to == NODENUM_BROADCAST_NO_LORA) + return ERRNO_SHOULD_RELEASE; + + packetPool.release(p); + return ERRNO_OK; + } + + uint32_t getPacketTime(uint32_t totalPacketLen, bool received = false) override + { + (void)totalPacketLen; + (void)received; + return 0; + } + + int sendCount = 0; + bool declineAll = false; +}; + static MockNodeDB *mockNodeDB = nullptr; static NextHopRouterTestShim *shim = nullptr; @@ -409,6 +439,65 @@ void test_hoplimit_decrement_when_resolved_not_favorite(void) TEST_ASSERT_TRUE(shim->shouldDecrementHopLimit(&p)); // unique but not a favorite -> decrement } +// =========================================================================== +// Rebroadcast of NODENUM_BROADCAST_NO_LORA +// =========================================================================== + +static MockRadioInterface *installMockIface() +{ + MockRadioInterface *m = new MockRadioInterface(); + shim->addInterface(std::unique_ptr(m)); + return m; +} + +// Eligible for rebroadcast: not from/to us, hops left, nonzero id, no next-hop preference. +// Encrypted variant so Router::send() skips the encode path. +static meshtastic_MeshPacket makeRebroadcastCandidate(NodeNum to) +{ + meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; + p.from = 0x22222222; // not us + p.to = to; + p.id = 0x0BADF00D; + p.hop_start = 3; + p.hop_limit = 3; + p.next_hop = NO_NEXT_HOP_PREFERENCE; + p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + p.encrypted.size = 8; + return p; +} + +// Control: proves the NO_LORA case below turns on the `to` field alone. +void test_rebroadcast_normal_broadcast_is_relayed(void) +{ + MockRadioInterface *mockIface = installMockIface(); + meshtastic_MeshPacket p = makeRebroadcastCandidate(NODENUM_BROADCAST); + + TEST_ASSERT_TRUE_MESSAGE(shim->perhapsRebroadcast(&p), "ordinary broadcast must be rebroadcast"); + TEST_ASSERT_EQUAL_MESSAGE(1, mockIface->sendCount, "exactly one packet should reach the radio"); +} + +void test_rebroadcast_no_lora_broadcast_is_not_relayed(void) +{ + MockRadioInterface *mockIface = installMockIface(); + meshtastic_MeshPacket p = makeRebroadcastCandidate(NODENUM_BROADCAST_NO_LORA); + + TEST_ASSERT_FALSE_MESSAGE(shim->perhapsRebroadcast(&p), "no-LoRa broadcast must not be rebroadcast"); + TEST_ASSERT_EQUAL_MESSAGE(0, mockIface->sendCount, "no packet should be handed to the radio at all"); +} + +// Declining mock bypasses the guard so send() is reached; the release itself is only observable as +// a sanitizer leak report, not an assertion. +void test_rebroadcast_declined_send_releases_packet(void) +{ + MockRadioInterface *mockIface = installMockIface(); + mockIface->declineAll = true; + + meshtastic_MeshPacket p = makeRebroadcastCandidate(NODENUM_BROADCAST); + + TEST_ASSERT_TRUE_MESSAGE(shim->perhapsRebroadcast(&p), "the rebroadcast must still be attempted"); + TEST_ASSERT_EQUAL_MESSAGE(1, mockIface->sendCount, "the copy must have reached the mock radio"); +} + // =========================================================================== void setup() @@ -459,6 +548,11 @@ void setup() RUN_TEST(test_hoplimit_decrement_on_colliding_favorites); RUN_TEST(test_hoplimit_decrement_when_resolved_not_favorite); + printf("\n=== rebroadcast of NODENUM_BROADCAST_NO_LORA ===\n"); + RUN_TEST(test_rebroadcast_normal_broadcast_is_relayed); + RUN_TEST(test_rebroadcast_no_lora_broadcast_is_not_relayed); + RUN_TEST(test_rebroadcast_declined_send_releases_packet); + exit(UNITY_END()); }