fix(outputmonitor): don't let the smart-receiver callback outlive its owner

SetOutput() copied srCallback out from under gpioLock and invoked the copy
after releasing it.  The callback is installed by FalconV5Support and captures
its raw 'this', so the copy carries no ownership: a string-config reload
destroying that object between the copy and the call left the dispatch writing
into freed memory.  The destructor's unregister could not prevent it -- it can
only clear the member, never a copy already on another thread's stack.  The
window is reachable because "Set Port Status" arrives on the API, MQTT, GPIO,
and scheduler threads while the reload runs elsewhere.

Invoke the callback through the member under a dedicated mutex that
setSmartReceiverEventCallback() also takes.  The unregister then blocks behind
any in-flight call and no call can start after it returns, so no copy escapes.
Kept separate from gpioLock so dispatch still happens with the port state
unlocked and can't re-enter into a self-deadlock.

The state the callback publishes had the same problem one level down: the
port, receiver index, and command string were three unsynchronized fields
written from the command thread and read by the packet generator on the output
thread.  As well as pairing a new port with a stale command, assigning and
comparing the std::string concurrently is a torn read of a heap pointer.
Replace them with a single lock-free atomic word, snapshotted once per pass and
cleared with a compare_exchange so a request arriving mid-pass isn't dropped.
static_asserts pin the two properties that relies on: no padding (the exchange
compares the object representation) and lock-freedom, which is what keeps the
32-bit build off a libatomic lock.

Builds clean for 32-bit and 64-bit BeagleBone platforms; the padding assert was
confirmed to fail when deliberately broken.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: dkulp

src/OutputMonitor.cpp

@@ -516,7 +516,7 @@ void OutputMonitor::DisableOutputs() {
 }
 
 void OutputMonitor::SetOutput(const std::string& port, bool on) {
-    // Smart-receiver commands are collected under the lock and dispatched
+    // Smart-receiver commands are collected under gpioLock and dispatched
     // after it is released: srCallback lands in FalconV5Support, and calling
     // out of the monitor with gpioLock held invites re-entry deadlocks.
     struct SRAction {
@@ -525,7 +525,6 @@ void OutputMonitor::SetOutput(const std::string& port, bool on) {
         const char* cmd;
     };
     std::vector<SRAction> srActions;
-    std::function<void(int, int, const std::string&)> cb;
     {
         std::unique_lock<std::shared_mutex> lock(gpioLock);
         int pn = 0;
@@ -551,16 +550,22 @@ void OutputMonitor::SetOutput(const std::string& port, bool on) {
             }
             pn++;
         }
-        cb = srCallback;
     }
-    if (cb) {
-        for (auto& a : srActions) {
-            cb(a.port, a.index, a.cmd);
+    if (!srActions.empty()) {
+        // Call through the member under srCallbackLock rather than copying it
+        // out first: a copy captures FalconV5Support's raw 'this' without
+        // owning it, so a config reload destroying that object between the
+        // copy and the call leaves us writing into freed memory.
+        std::unique_lock<std::mutex> cbLock(srCallbackLock);
+        if (srCallback) {
+            for (auto& a : srActions) {
+                srCallback(a.port, a.index, a.cmd);
+            }
         }
     }
 }
 void OutputMonitor::setSmartReceiverEventCallback(std::function<void(int port, int index, const std::string& cmd)>&& f) {
-    std::unique_lock<std::shared_mutex> lock(gpioLock);
+    std::unique_lock<std::mutex> lock(srCallbackLock);
     srCallback = std::move(f);
 }
 void OutputMonitor::RemovePortConfiguration(int port, const Json::Value& config) {

src/OutputMonitor.h

@@ -81,9 +81,9 @@ class OutputMonitor {
     // than assume the pre-output state.
     std::atomic<bool> outputsEnabled{ false };
     // Guards portPins, the pull-high/low pin lists, fusePins, eFuseRetries,
-    // pendingEFusePresets, and srCallback.  Writers (config reload, the eFuse
-    // GPIO callbacks, retry processing, enable/disable) take it exclusive;
-    // status readers (HTTP/MQTT port status, the pixel-count tester) take it
+    // and pendingEFusePresets.  Writers (config reload, the eFuse GPIO
+    // callbacks, retry processing, enable/disable) take it exclusive; status
+    // readers (HTTP/MQTT port status, the pixel-count tester) take it
     // shared.  Presets and the smart-receiver callback must NOT be invoked
     // while it is held: command presets run synchronously and can re-enter
     // EnableOutputs/DisableOutputs/SetOutput on the same thread.
@@ -94,6 +94,18 @@ class OutputMonitor {
     int eFuseRetryCount = 0;
     int eFuseRetryInterval = 100;
 
+    // The smart-receiver callback is owned by a FalconV5Support that is
+    // destroyed on a config reload while commands ("Set Port Status") are
+    // still arriving on the API/MQTT/GPIO threads.  The callback captures that
+    // object, so it must never be copied out and invoked after the lock is
+    // dropped -- the copy carries no ownership and the object can be freed in
+    // between.  Invoking it under this mutex, which ~FalconV5Support() also
+    // takes to clear the callback, means the unregister blocks behind any
+    // in-flight call and no call can start after it returns.
+    //
+    // Deliberately not gpioLock: dispatch has to happen with the port state
+    // unlocked so a callback that re-enters the monitor can't self-deadlock.
+    std::mutex srCallbackLock;
     std::function<void(int port, int index, const std::string& cmd)> srCallback;
     // Port names whose EFUSE_TRIGGERED preset still needs to fire.  Queued by
     // addEFuseWarning() (called with gpioLock held) and drained by

src/non-gpl/FalconV5Support/FalconV5Support.cpp

@@ -92,9 +92,17 @@ class PRUControl {
 
 FalconV5Support::FalconV5Support() {
     OutputMonitor::INSTANCE.setSmartReceiverEventCallback([this](int port, int index, const std::string& cmd) {
-        togglePort = port;
-        toggleIndex = index;
-        command = cmd;
+        PendingToggle pt;
+        pt.port = (int16_t)port;
+        pt.index = (uint8_t)index;
+        if (cmd == "ToggleOutput") {
+            pt.cmd = ToggleCommand::Toggle;
+        } else if (cmd == "ResetOutput") {
+            pt.cmd = ToggleCommand::Reset;
+        } else {
+            return;
+        }
+        pendingToggle.store(pt);
     });
 }
 FalconV5Support::~FalconV5Support() {
@@ -425,6 +433,9 @@ bool FalconV5Support::generateDynamicPacket(std::vector<std::array<uint8_t, 64>>
         }
     }
     curCount++;
+    // Snapshot the pending toggle once for this pass so every chain sees the
+    // same port/index/command triple.
+    PendingToggle toggle = pendingToggle.load();
     for (auto& g : queryData[curMux]) {
         auto rc = g.second.front();
         if (!rc->hasMoreQueries()) {
@@ -434,13 +445,18 @@ bool FalconV5Support::generateDynamicPacket(std::vector<std::array<uint8_t, 64>>
             rc = g.second.front();
         }
         int rcP = rc->getPixelStrings().front()->m_portNumber;
-        if (rcP <= togglePort && (rcP + 4) > togglePort) {
-            if (command == "ToggleOutput") {
-                rc->generateToggleEFusePacket(&packets[rc->getPixelStrings().front()->m_portNumber][0], toggleIndex, togglePort % 4);
-            } else if (command == "ResetOutput") {
-                rc->generateResetEFusePacket(&packets[rc->getPixelStrings().front()->m_portNumber][0], toggleIndex, togglePort % 4);
+        if (toggle.cmd != ToggleCommand::None && rcP <= toggle.port && (rcP + 4) > toggle.port) {
+            if (toggle.cmd == ToggleCommand::Toggle) {
+                rc->generateToggleEFusePacket(&packets[rc->getPixelStrings().front()->m_portNumber][0], toggle.index, toggle.port % 4);
+            } else {
+                rc->generateResetEFusePacket(&packets[rc->getPixelStrings().front()->m_portNumber][0], toggle.index, toggle.port % 4);
             }
-            togglePort = -1;
+            // Clear only the toggle we just acted on: a compare_exchange keeps
+            // a request that arrived since the snapshot from being dropped.
+            // 'expected' is a copy because a failed exchange writes through it.
+            PendingToggle expected = toggle;
+            pendingToggle.compare_exchange_strong(expected, PendingToggle());
+            toggle = PendingToggle();
         } else if (triggerPixelCount) {
             rc->generatePixelCountPacket(&packets[rc->getPixelStrings().front()->m_portNumber][0]);
             listen = false;

src/non-gpl/FalconV5Support/FalconV5Support.h

@@ -12,6 +12,8 @@
  * personal use, but modified copies MAY NOT be redistributed in any form.
  */
 
+#include <atomic>
+#include <cstdint>
 #include <functional>
 #include "fpp-json-fwd.h"
 #include <list>
@@ -85,7 +87,27 @@ class FalconV5Support {
     int curCount = 0;
     bool triggerPixelCount = false;
 
-    int togglePort = -1;
-    int toggleIndex = -1;
-    std::string command;
+    // A pending smart-receiver eFuse action.  Published by the OutputMonitor
+    // callback (API/MQTT/GPIO/scheduler threads) and consumed by
+    // generateDynamicPacket() on the output thread, so it has to be a single
+    // atomic word: as three separate fields the consumer could pair a new
+    // port with the previous command, and the std::string it replaces was
+    // being assigned and compared concurrently, which is a torn read of a
+    // heap pointer.  Kept to 4 bytes so the atomic is lock-free on 32-bit ARM
+    // as well.
+    enum class ToggleCommand : uint8_t {
+        None = 0,
+        Toggle,
+        Reset
+    };
+    struct PendingToggle {
+        int16_t port = -1;
+        uint8_t index = 0;
+        ToggleCommand cmd = ToggleCommand::None;
+    };
+    // compare_exchange compares the object representation, so padding here
+    // would make the exchange in generateDynamicPacket() fail at random.
+    static_assert(sizeof(PendingToggle) == 4, "PendingToggle must be a single unpadded word");
+    static_assert(std::atomic<PendingToggle>::is_always_lock_free, "PendingToggle atomic must be lock-free");
+    std::atomic<PendingToggle> pendingToggle{ PendingToggle() };
 };