Eliminating foot-gun and placing Phone NodeInfo into a mutex

src/mesh/PhoneAPI.cpp

@@ -71,8 +71,12 @@ void PhoneAPI::handleStartConfig()
     LOG_DEBUG("Got %d files in manifest", filesManifest.size());
 
     LOG_INFO("Start API client config");
-    nodeInfoForPhone.num = 0; // Don't keep returning old nodeinfos
+    // Protect against concurrent BLE callbacks: they run in NimBLE's FreeRTOS task and also touch nodeInfoQueue.
+    {
+        std::lock_guard<std::mutex> guard(nodeInfoMutex);
+        nodeInfoForPhone = {};
         nodeInfoQueue.clear();
+    }
     resetReadIndex();
 }
 
@@ -94,8 +98,12 @@ void PhoneAPI::close()
         onConnectionChanged(false);
         fromRadioScratch = {};
         toRadioScratch = {};
+        // Clear cached node info under lock because NimBLE callbacks can still be draining it.
+        {
+            std::lock_guard<std::mutex> guard(nodeInfoMutex);
             nodeInfoForPhone = {};
             nodeInfoQueue.clear();
+        }
         packetForPhone = NULL;
         filesManifest.clear();
         fromRadioNum = 0;
@@ -241,14 +249,21 @@ size_t PhoneAPI::getFromRadio(uint8_t *buf)
         LOG_DEBUG("Send My NodeInfo");
         auto us = nodeDB->readNextMeshNode(readIndex);
         if (us) {
-            nodeInfoForPhone = TypeConversions::ConvertToNodeInfo(us);
-            nodeInfoForPhone.has_hops_away = false;
-            nodeInfoForPhone.is_favorite = true;
+            auto info = TypeConversions::ConvertToNodeInfo(us);
+            info.has_hops_away = false;
+            info.is_favorite = true;
+            {
+                std::lock_guard<std::mutex> guard(nodeInfoMutex);
+                nodeInfoForPhone = info;
+            }
             fromRadioScratch.which_payload_variant = meshtastic_FromRadio_node_info_tag;
-            fromRadioScratch.node_info = nodeInfoForPhone;
+            fromRadioScratch.node_info = info;
             // Should allow us to resume sending NodeInfo in STATE_SEND_OTHER_NODEINFOS
+            {
+                std::lock_guard<std::mutex> guard(nodeInfoMutex);
                 nodeInfoForPhone.num = 0;
             }
+        }
         if (config_nonce == SPECIAL_NONCE_ONLY_NODES) {
             // If client only wants node info, jump directly to sending nodes
             state = STATE_SEND_OTHER_NODEINFOS;
@@ -434,23 +449,30 @@ size_t PhoneAPI::getFromRadio(uint8_t *buf)
 
     case STATE_SEND_OTHER_NODEINFOS: {
         LOG_DEBUG("Send known nodes");
+        meshtastic_NodeInfo infoToSend = {};
+        {
+            std::lock_guard<std::mutex> guard(nodeInfoMutex);
             if (nodeInfoForPhone.num == 0 && !nodeInfoQueue.empty()) {
                 // Serve the next cached node without re-reading from the DB iterator.
                 nodeInfoForPhone = nodeInfoQueue.front();
                 nodeInfoQueue.pop_front();
             }
-
-        if (nodeInfoForPhone.num != 0) {
-            // Just in case we stored a different user.id in the past, but should never happen going forward
-            sprintf(nodeInfoForPhone.user.id, "!%08x", nodeInfoForPhone.num);
-            LOG_DEBUG("nodeinfo: num=0x%x, lastseen=%u, id=%s, name=%s", nodeInfoForPhone.num, nodeInfoForPhone.last_heard,
-                      nodeInfoForPhone.user.id, nodeInfoForPhone.user.long_name);
-            fromRadioScratch.which_payload_variant = meshtastic_FromRadio_node_info_tag;
-            fromRadioScratch.node_info = nodeInfoForPhone;
+            infoToSend = nodeInfoForPhone;
+            if (infoToSend.num != 0)
                 nodeInfoForPhone = {};
+        }
+
+        if (infoToSend.num != 0) {
+            // Just in case we stored a different user.id in the past, but should never happen going forward
+            sprintf(infoToSend.user.id, "!%08x", infoToSend.num);
+            LOG_DEBUG("nodeinfo: num=0x%x, lastseen=%u, id=%s, name=%s", infoToSend.num, infoToSend.last_heard,
+                      infoToSend.user.id, infoToSend.user.long_name);
+            fromRadioScratch.which_payload_variant = meshtastic_FromRadio_node_info_tag;
+            fromRadioScratch.node_info = infoToSend;
             prefetchNodeInfos();
         } else {
             LOG_DEBUG("Done sending nodeinfo");
+            std::lock_guard<std::mutex> guard(nodeInfoMutex);
             nodeInfoQueue.clear();
             state = STATE_SEND_FILEMANIFEST;
             // Go ahead and send that ID right now
@@ -559,6 +581,8 @@ void PhoneAPI::prefetchNodeInfos()
 {
     bool added = false;
     // Keep the queue topped up so BLE reads stay responsive even if DB fetches take a moment.
+    {
+        std::lock_guard<std::mutex> guard(nodeInfoMutex);
         while (nodeInfoQueue.size() < kNodePrefetchDepth) {
             auto nextNode = nodeDB->readNextMeshNode(readIndex);
             if (!nextNode)
@@ -574,6 +598,7 @@ void PhoneAPI::prefetchNodeInfos()
             nodeInfoQueue.push_back(info);
             added = true;
         }
+    }
 
     if (added)
         onNowHasData(0);
@@ -614,10 +639,17 @@ bool PhoneAPI::available()
     case STATE_SEND_COMPLETE_ID:
         return true;
 
-    case STATE_SEND_OTHER_NODEINFOS:
-        if (nodeInfoQueue.empty())
-            prefetchNodeInfos();
+    case STATE_SEND_OTHER_NODEINFOS: {
+        std::lock_guard<std::mutex> guard(nodeInfoMutex);
+        if (nodeInfoQueue.empty()) {
+            // Drop the lock before prefetching; prefetchNodeInfos() will re-acquire it.
+            goto PREFETCH_NODEINFO;
+        }
+    }
         return true; // Always say we have something, because we might need to advance our state machine
+    PREFETCH_NODEINFO:
+        prefetchNodeInfos();
+        return true;
     case STATE_SEND_PACKETS: {
         if (!queueStatusPacketForPhone)
             queueStatusPacketForPhone = service->getQueueStatusForPhone();

src/mesh/PhoneAPI.h

@@ -5,6 +5,7 @@
 #include "meshtastic/portnums.pb.h"
 #include <deque>
 #include <iterator>
+#include <mutex>
 #include <string>
 #include <unordered_map>
 #include <vector>
@@ -84,6 +85,8 @@ class PhoneAPI
     std::deque<meshtastic_NodeInfo> nodeInfoQueue;
     // Tunable size of the node info cache so we can keep BLE reads non-blocking.
     static constexpr size_t kNodePrefetchDepth = 4;
+    // Protect nodeInfoForPhone + nodeInfoQueue because NimBLE callbacks run in a separate FreeRTOS task.
+    std::mutex nodeInfoMutex;
 
     meshtastic_ToRadio toRadioScratch = {
         0}; // this is a static scratch object, any data must be copied elsewhere before returning

src/nimble/NimbleBluetooth.cpp

@@ -139,7 +139,7 @@ class NimbleBluetoothFromRadioCallback : public NimBLECharacteristicCallbacks
     {
         bluetoothPhoneAPI->phoneWants = true;
         bluetoothPhoneAPI->setIntervalFromNow(0);
-        std::lock_guard<std::mutex> guard(bluetoothPhoneAPI->nimble_mutex);
+        std::lock_guard<std::mutex> guard(bluetoothPhoneAPI->nimble_mutex); // BLE callbacks run in NimBLE task
 
         if (!bluetoothPhoneAPI->hasChecked) {
             // Fetch payload on demand; prefetch keeps this fast for the first read.