Fix/call modules

src/mesh/MeshModule.cpp

@@ -195,7 +195,11 @@ void MeshModule::callModules(meshtastic_MeshPacket &mp, RxSource src)
     if (isDecoded && mp.decoded.want_response && toUs) {
         if (currentReply) {
             printPacket("Send response", currentReply);
-            service->sendToMesh(currentReply);
+            // A reply to a phone request loops back to us as its own destination (RX_SRC_LOCAL),
+            // which the loopback guard above hides from every module - including RoutingModule,
+            // whose handleReceivedProtobuf() is what forwards packets to the phone. Without
+            // ccToPhone the reply would be silently dropped instead of reaching the requester.
+            service->sendToMesh(currentReply, RX_SRC_LOCAL, isToUs(currentReply));
             currentReply = NULL;
         } else if (mp.from != ourNodeNum && !ignoreRequest) {
             // Note: if the message started with the local node or a module asked to ignore the request, we don't want to send a

src/mesh/Router.cpp

@@ -216,10 +216,10 @@ bool Router::shouldDecrementHopLimit(const meshtastic_MeshPacket *p)
  */
 int32_t Router::runOnce()
 {
-    meshtastic_MeshPacket *mp;
-    while ((mp = fromRadioQueue.dequeuePtr(0)) != NULL) {
-        // printPacket("handle fromRadioQ", mp);
-        perhapsHandleReceived(mp);
+    QueuedFromRadio qp;
+    while (fromRadioQueue.dequeue(&qp, 0)) {
+        // printPacket("handle fromRadioQ", qp.packet);
+        perhapsHandleReceived(qp.packet, qp.src);
     }
 
     // LOG_DEBUG("Sleep forever!");
@@ -230,15 +230,14 @@ int32_t Router::runOnce()
  * RadioInterface calls this to queue up packets that have been received from the radio.  The router is now responsible for
  * freeing the packet
  */
-void Router::enqueueReceivedMessage(meshtastic_MeshPacket *p)
+void Router::enqueueReceivedMessage(meshtastic_MeshPacket *p, RxSource src)
 {
     // Try enqueue until successful
-    while (!fromRadioQueue.enqueue(p, 0)) {
-        meshtastic_MeshPacket *old_p;
-        old_p = fromRadioQueue.dequeuePtr(0); // Dequeue and discard the oldest packet
-        if (old_p) {
-            printPacket("fromRadioQ full, drop oldest!", old_p);
-            packetPool.release(old_p);
+    while (!fromRadioQueue.enqueue(QueuedFromRadio{p, src}, 0)) {
+        QueuedFromRadio old_qp;
+        if (fromRadioQueue.dequeue(&old_qp, 0)) { // Dequeue and discard the oldest packet
+            printPacket("fromRadioQ full, drop oldest!", old_qp.packet);
+            packetPool.release(old_qp.packet);
         }
     }
     // Nasty hack because our threading is primitive.  interfaces shouldn't need to know about routers FIXME
@@ -327,10 +326,13 @@ ErrorCode Router::sendLocal(meshtastic_MeshPacket *p, RxSource src)
     // No need to deliver externally if the destination is the local node
     if (isToUs(p)) {
         printPacket("Enqueued local", p);
-        // Preserve the trusted origin explicitly. Queueing used to erase src and make a local
-        // phone/module packet indistinguishable from remote already-decoded ingress.
-        handleReceived(p, src);
-        return ERRNO_SHOULD_RELEASE;
+        // Queue rather than call handleReceived() synchronously: a reply generated from inside
+        // MeshModule::callModules() (e.g. an admin/module-config response) lands here via
+        // sendToMesh(), and calling handleReceived() in-line would re-enter callModules() from
+        // within itself. The queue carries src through so the packet is still replayed with its
+        // true origin instead of defaulting to RX_SRC_RADIO.
+        enqueueReceivedMessage(p, src);
+        return ERRNO_OK;
     } else if (!iface) {
         // We must be sending to remote nodes also, fail if no interface found
         abortSendAndNak(meshtastic_Routing_Error_NO_INTERFACE, p);
@@ -1356,7 +1358,7 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src)
     packetPool.release(p_encrypted); // Release the encrypted packet (release() handles nullptr)
 }
 
-void Router::perhapsHandleReceived(meshtastic_MeshPacket *p)
+void Router::perhapsHandleReceived(meshtastic_MeshPacket *p, RxSource src)
 {
 #if ARCH_PORTDUINO
     // Even ignored packets get logged in the trace
@@ -1365,6 +1367,18 @@ void Router::perhapsHandleReceived(meshtastic_MeshPacket *p)
         LOG_TRACE("%s", MeshPacketSerializer::JsonSerializeEncrypted(p).c_str());
     }
 #endif
+    // A non-radio packet (a module's reply to a phone request, or a phone/serial-originated packet
+    // addressed to us, queued here only to avoid re-entering callModules() synchronously) was never
+    // actually received over the mesh. The ignore-list, PacketHistory/dedup, MQTT and pre-hop filters
+    // below exist to police untrusted radio ingress, and handleReceived() already special-cases non-
+    // RX_SRC_RADIO sources (e.g. it only applies the routing-auth cache for RX_SRC_RADIO) - so skip
+    // straight there instead of risking a trusted local/user packet getting deduped or ignore-listed.
+    if (src != RX_SRC_RADIO) {
+        handleReceived(p, src);
+        packetPool.release(p);
+        return;
+    }
+
     // assert(radioConfig.has_preferences);
     if (is_in_repeated(config.lora.ignore_incoming, p->from)) {
         clearRoutingAuthCache();
@@ -1425,6 +1439,6 @@ void Router::perhapsHandleReceived(meshtastic_MeshPacket *p)
 
     // Note: we avoid calling shouldFilterReceived if we are supposed to ignore certain nodes - because some overrides might
     // cache/learn of the existence of nodes (i.e. FloodRouter) that they should not
-    handleReceived(p);
+    handleReceived(p, src);
     packetPool.release(p);
 }

src/mesh/Router.h

@@ -5,8 +5,8 @@
 #include "MeshTypes.h"
 #include "Observer.h"
 #include "PacketHistory.h"
-#include "PointerQueue.h"
 #include "RadioInterface.h"
+#include "TypedQueue.h"
 #include "concurrency/OSThread.h"
 #include <memory>
 
@@ -16,9 +16,17 @@
 class Router : protected concurrency::OSThread, protected PacketHistory
 {
   private:
-    /// Packets which have just arrived from the radio, ready to be processed by this service and possibly
-    /// forwarded to the phone.
-    PointerQueue<meshtastic_MeshPacket> fromRadioQueue;
+    /** A queued fromRadioQueue entry - pairs the packet with the RxSource it arrived/originated with, so a
+     * locally-addressed packet queued via sendLocal() is replayed with its true origin instead of defaulting
+     * to RX_SRC_RADIO. */
+    struct QueuedFromRadio {
+        meshtastic_MeshPacket *packet;
+        RxSource src;
+    };
+
+    /// Packets which have just arrived from the radio (or were generated locally and addressed to us), ready to
+    /// be processed by this service and possibly forwarded to the phone.
+    TypedQueue<QueuedFromRadio> fromRadioQueue;
 
   protected:
     std::unique_ptr<RadioInterface> iface = nullptr;
@@ -82,9 +90,11 @@ class Router : protected concurrency::OSThread, protected PacketHistory
 
     /**
      * RadioInterface calls this to queue up packets that have been received from the radio.  The router is now responsible for
-     * freeing the packet
+     * freeing the packet. Also used by sendLocal() to defer processing of a locally-addressed packet instead of
+     * re-entering the packet-handling pipeline synchronously; src defaults to RX_SRC_RADIO for the radio/MQTT/UDP
+     * ingress callers.
      */
-    virtual void enqueueReceivedMessage(meshtastic_MeshPacket *p);
+    virtual void enqueueReceivedMessage(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO);
 
     /**
      * Send a packet on a suitable interface.  This routine will
@@ -145,10 +155,9 @@ class Router : protected concurrency::OSThread, protected PacketHistory
      * Handle any packet that is received by an interface on this node.
      * Note: some packets may merely being passed through this node and will be forwarded elsewhere.
      *
-     * Note: this packet will never be called for messages sent/generated by this node.
      * Note: this method will free the provided packet.
      */
-    void perhapsHandleReceived(meshtastic_MeshPacket *p);
+    void perhapsHandleReceived(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO);
 
     /**
      * Called from perhapsHandleReceived() - allows subclass message delivery behavior.

test/test_mqtt/MQTT.cpp

@@ -41,8 +41,9 @@ class MockRouter : public Router
         delete cryptLock;
         cryptLock = NULL;
     }
-    void enqueueReceivedMessage(meshtastic_MeshPacket *p) override
+    void enqueueReceivedMessage(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO) override
     {
+        (void)src;
         packets_.emplace_back(*p);
         packetPool.release(p);
     }

test/test_packet_signing/test_main.cpp

@@ -389,6 +389,10 @@ void setUp(void)
     channels.initDefaults();
     channels.onConfigChanged();
 
+    // sendLocal() defers isToUs() packets to fromRadioQueue rather than handling them in-line; drain
+    // any left behind by a test that failed an assertion before draining its own (e.g. an aborted
+    // TEST_ASSERT partway through), so it can't leak into the next test's counters.
+    pipelineRouter->runOnce();
     pipelineRouter->clearPending();
     pipelineRouter->rxDupe = 0;
     pipelineRouter->txRelayCanceled = 0;
@@ -1216,9 +1220,12 @@ void test_C8_trusted_local_decoded_delivery_is_not_filtered(void)
     meshtastic_MeshPacket *local =
         packetPool.allocCopy(makeDecoded(0, LOCAL_NODE, meshtastic_PortNum_POSITION_APP, SMALL_PAYLOAD));
     TEST_ASSERT_NOT_NULL(local);
-    TEST_ASSERT_EQUAL(ERRNO_SHOULD_RELEASE, pipelineRouter->sendLocal(local, RX_SRC_USER));
+    // sendLocal() defers isToUs() packets to fromRadioQueue instead of calling handleReceived()
+    // in-line (avoids re-entering callModules() from within itself); drain it like the real Router
+    // thread would on its next tick. The queue - not this call - now owns releasing the packet.
+    TEST_ASSERT_EQUAL(ERRNO_OK, pipelineRouter->sendLocal(local, RX_SRC_USER));
+    pipelineRouter->runOnce();
     TEST_ASSERT_EQUAL_MESSAGE(1, pipelineModule->calls, "trusted phone-origin packet must reach local modules");
-    packetPool.release(local);
 }
 
 void test_C9_known_channel_malformed_plaintext_is_not_relayed_as_opaque(void)