Guard the deferred local queue and depth counter (#11284)

* Guard the deferred local queue and depth counter

* Close the drain and enqueue race on the deferred queue

* Route the raced loopback through handleReceived

* Make the last-frame check and depth decrement atomic

* Correct the handleReceived doc comment

Author: caveman99

src/mesh/Router.cpp

@@ -1249,7 +1249,12 @@ bool Router::dequeueDeferredLocal(DeferredLocal &out)
 void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src)
 {
     // Top level: handle synchronously, exactly as before the depth guard existed.
-    if (handleDepth == 0) {
+    bool nested;
+    {
+        concurrency::LockGuard g(&deferredLock);
+        nested = handleDepth > 0;
+    }
+    if (!nested) {
         handleReceived(p, src);
         return;
     }
@@ -1258,8 +1263,26 @@ void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src)
     // handleReceived() drains it once the current dispatch unwinds, instead of stacking another
     // handleReceived() frame on top of the module handler (nRF52 stack overflow on config save).
     meshtastic_MeshPacket *copy = packetPool.allocCopy(*p);
-    if (copy && enqueueDeferredLocal(copy, src))
-        return;
+    if (copy) {
+        // Re-check depth under the lock that also gates the drain's decrement, so a drain finishing
+        // while we allocated cannot leave this copy stranded in the ring.
+        bool stillNested = false, queued = false;
+        {
+            concurrency::LockGuard g(&deferredLock);
+            stillNested = handleDepth > 0;
+            if (stillNested)
+                queued = enqueueDeferredLocal(copy, src);
+        }
+        if (queued)
+            return;
+        if (!stillNested) {
+            // The drain finished first, so nothing would pick this up. Go through handleReceived()
+            // rather than dispatchReceived() so a loopback from its modules still defers.
+            handleReceived(copy, src);
+            packetPool.release(copy);
+            return;
+        }
+    }
 
     // Pool exhausted or queue full: drop the deferral. Leak-free and degraded but safe - the
     // packet still followed its normal non-loopback path (SHOULD_RELEASE, or the TX path for a
@@ -1278,29 +1301,39 @@ void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src)
  */
 void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src)
 {
-    handleDepth++;
+    {
+        concurrency::LockGuard g(&deferredLock);
+        handleDepth++;
 #ifdef PIO_UNIT_TESTING
-    if (handleDepth > maxHandleDepthObserved)
-        maxHandleDepthObserved = handleDepth;
+        if (handleDepth > maxHandleDepthObserved)
+            maxHandleDepthObserved = handleDepth;
 #endif
+    }
 
     dispatchReceived(p, src);
 
-    // Only the outermost frame drains. Deferred packets were produced by modules sending from
-    // inside dispatchReceived()'s callModules(); process them here, after the triggering frame has
-    // unwound, so a second handleReceived() never sits on top of a module handler. handleDepth
-    // stays >= 1 through the drain, so a drained packet whose own modules send more loopback
-    // packets enqueues them for this same loop rather than recursing: the stack stays flat and
-    // processing is breadth-first.
-    if (handleDepth == 1) {
+    // Decide "am I the last frame" and drop the depth in one critical section. Splitting them lets
+    // two frames both read the same pre-decrement value, skip the drain, and strand the ring.
+    for (;;) {
         DeferredLocal d;
-        while (dequeueDeferredLocal(d)) {
-            dispatchReceived(d.p, d.src);
-            packetPool.release(d.p);
+        {
+            concurrency::LockGuard g(&deferredLock);
+            if (handleDepth > 1) {
+                // Another frame is still live and will own the drain once it is last.
+                handleDepth--;
+                return;
+            }
+            if (!dequeueDeferredLocal(d)) {
+                // Last frame and nothing queued, so zero is reached only with the ring empty.
+                handleDepth--;
+                return;
+            }
         }
+        // Depth stays at 1 across the drain, so a loopback from these modules defers instead of
+        // recursing, and dispatch runs outside the lock.
+        dispatchReceived(d.p, d.src);
+        packetPool.release(d.p);
     }
-
-    handleDepth--;
 }
 
 void Router::dispatchReceived(meshtastic_MeshPacket *p, RxSource src)

src/mesh/Router.h

@@ -7,6 +7,7 @@
 #include "PacketHistory.h"
 #include "PointerQueue.h"
 #include "RadioInterface.h"
+#include "concurrency/LockGuard.h"
 #include "concurrency/OSThread.h"
 #include <memory>
 
@@ -162,12 +163,8 @@ class Router : protected concurrency::OSThread, protected PacketHistory
     void perhapsHandleReceived(meshtastic_MeshPacket *p);
 
     /**
-     * Called from perhapsHandleReceived() - allows subclass message delivery behavior.
-     * 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.
+     * Called from perhapsHandleReceived() for radio ingress and from deliverLocal() for our own
+     * loopback, so p may be locally generated. Does NOT free p; the caller still owns it.
      */
     void handleReceived(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO);
 
@@ -192,6 +189,10 @@ class Router : protected concurrency::OSThread, protected PacketHistory
     /// so a locally-sent loopback packet must be deferred rather than handled synchronously.
     uint8_t handleDepth = 0;
 
+    /// Guards handleDepth and the deferred ring below. nRF52 drives the router from the BLE task as
+    /// well as the loop task, so both are read-modify-written from two tasks.
+    concurrency::Lock deferredLock;
+
     /// A local loopback packet whose handleReceived() was deferred because it was produced from
     /// inside callModules(). The queue owns the packet; its RxSource travels with it so the drain
     /// dispatches it with the origin the sender intended (RX_SRC_LOCAL stays local).
@@ -210,8 +211,10 @@ class Router : protected concurrency::OSThread, protected PacketHistory
     uint8_t deferredLocalCount = 0; // entries currently queued
 
     /// Queue a deferred local packet. Returns false (and queues nothing) when full.
+    /// Caller must hold deferredLock: the enqueue decision is atomic with the drain's depth update.
     bool enqueueDeferredLocal(meshtastic_MeshPacket *p, RxSource src);
     /// Pop the oldest deferred local packet into out. Returns false when empty.
+    /// Caller must hold deferredLock.
     bool dequeueDeferredLocal(DeferredLocal &out);
 
     /** Frees the provided packet, and generates a NAK indicating the specifed error while sending */