From 7505fd910444eebb19de0ea6561b4e42a9dc0915 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Wed, 29 Jul 2026 21:33:54 +0200 Subject: [PATCH 1/5] Guard the deferred local queue and depth counter --- src/mesh/Router.cpp | 38 ++++++++++++++++++++++++++------------ src/mesh/Router.h | 5 +++++ 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index b60942bcd6b..375e9507b5e 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1227,6 +1227,7 @@ NodeNum Router::getNodeNum() bool Router::enqueueDeferredLocal(meshtastic_MeshPacket *p, RxSource src) { + concurrency::LockGuard g(&deferredLock); if (deferredLocalCount >= deferredLocalCapacity) return false; uint8_t tail = (deferredLocalHead + deferredLocalCount) % deferredLocalCapacity; @@ -1238,6 +1239,7 @@ bool Router::enqueueDeferredLocal(meshtastic_MeshPacket *p, RxSource src) bool Router::dequeueDeferredLocal(DeferredLocal &out) { + concurrency::LockGuard g(&deferredLock); if (deferredLocalCount == 0) return false; out = deferredLocalQueue[deferredLocalHead]; @@ -1249,7 +1251,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; } @@ -1278,21 +1285,25 @@ 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) { + // Only the outermost frame drains, after the triggering frame unwound, so a second + // handleReceived() never sits on a module handler. Depth stays >=1, keeping the drain flat. + bool outermost; + { + concurrency::LockGuard g(&deferredLock); + outermost = handleDepth == 1; + } + if (outermost) { DeferredLocal d; while (dequeueDeferredLocal(d)) { dispatchReceived(d.p, d.src); @@ -1300,7 +1311,10 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) } } - handleDepth--; + { + concurrency::LockGuard g(&deferredLock); + handleDepth--; + } } void Router::dispatchReceived(meshtastic_MeshPacket *p, RxSource src) diff --git a/src/mesh/Router.h b/src/mesh/Router.h index be686ee159f..ba54ea1eb02 100644 --- a/src/mesh/Router.h +++ b/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 @@ -192,6 +193,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). From 390e6ed3512a20fde8a78f7c2d694e15538d4c58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Wed, 29 Jul 2026 23:53:29 +0200 Subject: [PATCH 2/5] Close the drain and enqueue race on the deferred queue --- src/mesh/Router.cpp | 36 ++++++++++++++++++++++++++++++------ src/mesh/Router.h | 2 ++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 375e9507b5e..b5680ed450e 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1227,7 +1227,6 @@ NodeNum Router::getNodeNum() bool Router::enqueueDeferredLocal(meshtastic_MeshPacket *p, RxSource src) { - concurrency::LockGuard g(&deferredLock); if (deferredLocalCount >= deferredLocalCapacity) return false; uint8_t tail = (deferredLocalHead + deferredLocalCount) % deferredLocalCapacity; @@ -1239,7 +1238,6 @@ bool Router::enqueueDeferredLocal(meshtastic_MeshPacket *p, RxSource src) bool Router::dequeueDeferredLocal(DeferredLocal &out) { - concurrency::LockGuard g(&deferredLock); if (deferredLocalCount == 0) return false; out = deferredLocalQueue[deferredLocalHead]; @@ -1265,8 +1263,25 @@ 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; deliver it here instead. + dispatchReceived(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 @@ -1304,8 +1319,17 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) outermost = handleDepth == 1; } if (outermost) { - DeferredLocal d; - while (dequeueDeferredLocal(d)) { + // Drop to zero only while the ring is observed empty under the lock, so an enqueuer that + // re-checks depth either lands in the ring we still drain, or sees 0 and delivers itself. + for (;;) { + DeferredLocal d; + { + concurrency::LockGuard g(&deferredLock); + if (!dequeueDeferredLocal(d)) { + handleDepth--; + return; + } + } dispatchReceived(d.p, d.src); packetPool.release(d.p); } diff --git a/src/mesh/Router.h b/src/mesh/Router.h index ba54ea1eb02..142ea93b49a 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -215,8 +215,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 */ From 8bd36faa340f58e53920683393e2c92314251eb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 00:05:19 +0200 Subject: [PATCH 3/5] Route the raced loopback through handleReceived --- src/mesh/Router.cpp | 5 +++-- src/mesh/Router.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index b5680ed450e..719b7b8e13a 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1276,8 +1276,9 @@ void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src) if (queued) return; if (!stillNested) { - // The drain finished first, so nothing would pick this up; deliver it here instead. - dispatchReceived(copy, src); + // 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; } diff --git a/src/mesh/Router.h b/src/mesh/Router.h index 142ea93b49a..4888be1edb0 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -168,7 +168,7 @@ class Router : protected concurrency::OSThread, protected PacketHistory * 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. + * Note: this does NOT free the provided packet; the caller still owns it. */ void handleReceived(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO); From eda1a30fc8ba6a83c03973f341a3834cf965b004 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 08:33:01 +0200 Subject: [PATCH 4/5] Make the last-frame check and depth decrement atomic --- src/mesh/Router.cpp | 44 +++++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 719b7b8e13a..38048fddbe1 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1312,33 +1312,27 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) dispatchReceived(p, src); - // Only the outermost frame drains, after the triggering frame unwound, so a second - // handleReceived() never sits on a module handler. Depth stays >=1, keeping the drain flat. - bool outermost; - { - concurrency::LockGuard g(&deferredLock); - outermost = handleDepth == 1; - } - if (outermost) { - // Drop to zero only while the ring is observed empty under the lock, so an enqueuer that - // re-checks depth either lands in the ring we still drain, or sees 0 and delivers itself. - for (;;) { - DeferredLocal d; - { - concurrency::LockGuard g(&deferredLock); - if (!dequeueDeferredLocal(d)) { - handleDepth--; - return; - } + // 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; + { + 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; } - dispatchReceived(d.p, d.src); - packetPool.release(d.p); } - } - - { - concurrency::LockGuard g(&deferredLock); - handleDepth--; + // 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); } } From db01c457dbc861a809a04736319f227b6faeee69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 09:37:34 +0200 Subject: [PATCH 5/5] Correct the handleReceived doc comment --- src/mesh/Router.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/mesh/Router.h b/src/mesh/Router.h index 4888be1edb0..2a4d979c2e8 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -163,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 does NOT free the provided packet; the caller still owns it. + * 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);