From 6c8becc8a6abe2fd52d2b0f0701fb9397deadfeb Mon Sep 17 00:00:00 2001 From: Bryan Biedenkapp Date: Mon, 12 Oct 2026 12:34:28 -0400 Subject: [PATCH] correct some STP edge case failures; implement STP unit tests proper; --- src/fne/network/SpanningTree.cpp | 10 +- src/fne/network/packet/NetworkTree.cpp | 50 +++-- tests/fne/SpanningTree_Tests.cpp | 274 +++++++++++++++++++++++++ 3 files changed, 316 insertions(+), 18 deletions(-) create mode 100644 tests/fne/SpanningTree_Tests.cpp diff --git a/src/fne/network/SpanningTree.cpp b/src/fne/network/SpanningTree.cpp index b25a64c8..d88233ac 100644 --- a/src/fne/network/SpanningTree.cpp +++ b/src/fne/network/SpanningTree.cpp @@ -392,13 +392,19 @@ void SpanningTree::internalErasePeer(const uint32_t peerId) } if (tree->hasChildren()) { + // Recursive deletion removes each child from this vector. Snapshot the + // IDs first so recursion cannot invalidate an active vector iterator. + std::vector children; + children.reserve(tree->m_children.size()); for (auto child : tree->m_children) { if (child != nullptr) { - internalErasePeer(child->id()); + children.push_back(child->id()); } } - tree->m_children.clear(); + for (auto childId : children) { + internalErasePeer(childId); + } } delete tree; diff --git a/src/fne/network/packet/NetworkTree.cpp b/src/fne/network/packet/NetworkTree.cpp index 6175b852..ca1166f0 100644 --- a/src/fne/network/packet/NetworkTree.cpp +++ b/src/fne/network/packet/NetworkTree.cpp @@ -29,13 +29,29 @@ void MetadataNetwork::PacketHandler::networkTree(TrafficNetwork* network, Metada return; if (req->fneHeader.getSubFunction() == NET_SUBFUNC::NET_TREE_LIST) { // FNE Network Tree List - if (peerId > 0 && (network->m_peers.find(peerId) != network->m_peers.end())) { - FNEPeerConnection* connection = network->m_peers[peerId]; - if (connection != nullptr) { + if (peerId > 0) { + bool peerFound = false; + bool peerAuthorized = false; + std::string peerIdentity; + + // Snapshot the connection properties while holding the peer map lock. Do not + // retain the raw connection pointer: stale-peer cleanup may erase and delete + // the connection while this metadata task is processing the STP update. + network->m_peers.shared_lock(); + auto peerIt = network->m_peers.find(peerId); + if (peerIt != network->m_peers.end() && peerIt->second != nullptr) { + FNEPeerConnection* connection = peerIt->second; std::string ip = udp::Socket::address(req->address); + peerFound = true; + peerIdentity = connection->identWithQualifier(); + peerAuthorized = connection->connected() && connection->address() == ip && + connection->peerClass() == PEER_CONN_CLASS_NEIGHBOR; + } + network->m_peers.shared_unlock(); + if (peerFound) { // validate peer (simple validation really) - if (connection->connected() && connection->address() == ip && connection->peerClass() == PEER_CONN_CLASS_NEIGHBOR) { + if (peerAuthorized) { DECLARE_UINT8_ARRAY(rawPayload, req->length); ::memcpy(rawPayload, req->buffer, req->length); @@ -44,7 +60,7 @@ void MetadataNetwork::PacketHandler::networkTree(TrafficNetwork* network, Metada PacketBufferEntryPtr pkt = findOrCreatePacketBufferEntry(mdNetwork->m_peerTreeListPkt, peerId, "Network Tree, Tree List", streamId); if (pkt == nullptr) { LogError(LOG_STP, "PEER %u (%s) Network Tree, Tree List, failed to initialize packet buffer", peerId, - connection->identWithQualifier().c_str()); + peerIdentity.c_str()); return; } @@ -57,7 +73,7 @@ void MetadataNetwork::PacketHandler::networkTree(TrafficNetwork* network, Metada if (!pktLock.owns_lock()) { LogError(LOG_STP, "PEER %u (%s) Network Tree, Tree List, timeout waiting for packet buffer to unlock", peerId, - connection->identWithQualifier().c_str()); + peerIdentity.c_str()); // detach the stalled transfer without destroying state that // another worker still owns; its shared_ptr keeps it alive erasePacketBufferEntry(mdNetwork->m_peerTreeListPkt, peerId, pkt); @@ -73,7 +89,7 @@ void MetadataNetwork::PacketHandler::networkTree(TrafficNetwork* network, Metada if (pkt->streamId != streamId) { LogError(LOG_STP, "PEER %u (%s) Network Tree, Tree List, stream ID mismatch, expected %u, got %u", peerId, - connection->identWithQualifier().c_str(), pkt->streamId, streamId); + peerIdentity.c_str(), pkt->streamId, streamId); pkt->buffer->clear(); pkt->streamId = streamId; } @@ -88,7 +104,7 @@ void MetadataNetwork::PacketHandler::networkTree(TrafficNetwork* network, Metada json::value v; std::string err = json::parse(v, payload); if (!err.empty()) { - LogError(LOG_STP, "PEER %u (%s) error parsing network tree list, %s", peerId, connection->identWithQualifier().c_str(), err.c_str()); + LogError(LOG_STP, "PEER %u (%s) error parsing network tree list, %s", peerId, peerIdentity.c_str(), err.c_str()); pkt->buffer->clear(); pkt->buffer.reset(); pkt->streamId = 0U; @@ -101,7 +117,7 @@ void MetadataNetwork::PacketHandler::networkTree(TrafficNetwork* network, Metada else { // ensure parsed JSON is an array if (!v.is()) { - LogError(LOG_STP, "PEER %u (%s) error parsing network tree list, data was not valid", peerId, connection->identWithQualifier().c_str()); + LogError(LOG_STP, "PEER %u (%s) error parsing network tree list, data was not valid", peerId, peerIdentity.c_str()); pkt->buffer->clear(); pkt->buffer.reset(); pkt->streamId = 0U; @@ -113,19 +129,21 @@ void MetadataNetwork::PacketHandler::networkTree(TrafficNetwork* network, Metada } else { json::array arr = v.get(); - LogInfoEx(LOG_STP, "PEER %u (%s) Network Tree, Tree List, updating %u peer entries", peerId, connection->identWithQualifier().c_str(), arr.size()); - - std::lock_guard guard(network->m_treeLock); + LogInfoEx(LOG_STP, "PEER %u (%s) Network Tree, Tree List, updating %u peer entries", peerId, peerIdentity.c_str(), arr.size()); std::vector duplicatePeers; - SpanningTree::deserializeTree(arr, network->m_treeRoot, &duplicatePeers); - - network->logSpanningTree(connection); + { + std::lock_guard guard(network->m_treeLock); + SpanningTree::deserializeTree(arr, network->m_treeRoot, &duplicatePeers); + network->logSpanningTree(); + } + // Network writes acquire the peer map lock, so keep them + // outside m_treeLock to avoid extending the lock chain. if (duplicatePeers.size() > 0U) { for (auto dupPeerId : duplicatePeers) { LogWarning(LOG_STP, "PEER %u (%s) Network Tree, Tree Change, disconnecting duplicate peer connection for PEER %u to prevent network loop", - peerId, connection->identWithQualifier().c_str(), dupPeerId); + peerId, peerIdentity.c_str(), dupPeerId); network->writeTreeDisconnect(peerId, dupPeerId); } } diff --git a/tests/fne/SpanningTree_Tests.cpp b/tests/fne/SpanningTree_Tests.cpp new file mode 100644 index 00000000..8ca8d1df --- /dev/null +++ b/tests/fne/SpanningTree_Tests.cpp @@ -0,0 +1,274 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Digital Voice Modem - Test Suite + * GPLv2 Open Source. Use is subject to license terms. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES FROM THIS FILE HEADER. + * + * Copyright (C) 2026 Bryan Biedenkapp, N2PLL + * + */ +#include "Defines.h" +#include "network/SpanningTree.h" + +#include + +#include +#include +#include + +using namespace network; + +namespace +{ + /** + * @brief Creates a JSON representation of a tree node with the given ID, master ID, and optional children. + * @param id The ID of the tree node. + * @param masterId The master ID of the tree node. + * @param children The optional array of child nodes. + * @return A JSON value representing the tree node. + */ + json::value treeNode(uint32_t id, uint32_t masterId, const json::array& children = json::array()) + { + json::object node = json::object(); + node["id"].set(id); + node["masterId"].set(masterId); + node["identity"].set("test-peer-" + std::to_string(id)); + node["children"].set(children); + return json::value(node); + } +} + +/** + * @brief Fixture for creating and managing a SpanningTree root node. + */ +class TreeFixture { +public: + /** + * @brief Initializes a new instance of the TreeFixture class with the specified root ID. + * @param rootId The ID of the root node of the spanning tree. + */ + explicit TreeFixture(uint32_t rootId) : + root(new SpanningTree(rootId, rootId, nullptr)) + { + root->identity("test-root"); + } + + /** + * @brief Finalizes an instance of the TreeFixture class. + */ + ~TreeFixture() + { + if (root != nullptr && SpanningTree::findByPeerID(root->id()) == root) + SpanningTree::erasePeer(root->id()); + } + + SpanningTree* root; +}; + +TEST_CASE("SpanningTree recursively removes a multi-level subtree", "[fne][stp]") +{ + TreeFixture fixture(0xF1000001U); + SpanningTree* branch = new SpanningTree(0xF1000002U, 0xF1100002U, fixture.root); + SpanningTree* childA = new SpanningTree(0xF1000003U, 0xF1100003U, branch); + SpanningTree* childB = new SpanningTree(0xF1000004U, 0xF1100004U, branch); + new SpanningTree(0xF1000005U, 0xF1100005U, childA); + new SpanningTree(0xF1000006U, 0xF1100006U, childB); + + REQUIRE(SpanningTree::countChildren(fixture.root) == 5U); + + SpanningTree::erasePeer(branch->id()); + + CHECK_FALSE(fixture.root->hasChildren()); + CHECK(SpanningTree::countChildren(fixture.root) == 0U); + for (uint32_t id = 0xF1000002U; id <= 0xF1000006U; id++) + CHECK(SpanningTree::findByPeerID(id) == nullptr); +} + +TEST_CASE("SpanningTree update safely prunes a removed descendant branch", "[fne][stp]") +{ + TreeFixture fixture(0xF2000001U); + SpanningTree* upstream = new SpanningTree(0xF2000002U, 0xF2100002U, fixture.root); + SpanningTree* removedBranch = new SpanningTree(0xF2000003U, 0xF2100003U, upstream); + new SpanningTree(0xF2000004U, 0xF2100004U, removedBranch); + SpanningTree* retainedBranch = new SpanningTree(0xF2000005U, 0xF2100005U, upstream); + SpanningTree* unaffected = new SpanningTree(0xF2000006U, 0xF2100006U, fixture.root); + + json::array retainedChildren = json::array(); + retainedChildren.push_back(treeNode(retainedBranch->id(), retainedBranch->masterId())); + + json::array update = json::array(); + update.push_back(treeNode(upstream->id(), upstream->masterId(), retainedChildren)); + + std::vector duplicatePeers; + SpanningTree::deserializeTree(update, fixture.root, &duplicatePeers); + + CHECK(duplicatePeers.empty()); + CHECK(SpanningTree::findByPeerID(0xF2000003U) == nullptr); + CHECK(SpanningTree::findByPeerID(0xF2000004U) == nullptr); + CHECK(SpanningTree::findByPeerID(retainedBranch->id()) == retainedBranch); + CHECK(retainedBranch->m_parent == upstream); + CHECK(SpanningTree::findByPeerID(unaffected->id()) == unaffected); + CHECK(unaffected->m_parent == fixture.root); + CHECK(SpanningTree::countChildren(fixture.root) == 3U); +} + +TEST_CASE("SpanningTree update removes all descendants from an announced leaf", "[fne][stp]") +{ + TreeFixture fixture(0xF3000001U); + SpanningTree* upstream = new SpanningTree(0xF3000002U, 0xF3100002U, fixture.root); + SpanningTree* child = new SpanningTree(0xF3000003U, 0xF3100003U, upstream); + new SpanningTree(0xF3000004U, 0xF3100004U, child); + + json::array update = json::array(); + update.push_back(treeNode(upstream->id(), upstream->masterId())); + + std::vector duplicatePeers; + SpanningTree::deserializeTree(update, fixture.root, &duplicatePeers); + + CHECK(duplicatePeers.empty()); + CHECK_FALSE(upstream->hasChildren()); + CHECK(SpanningTree::findByPeerID(0xF3000003U) == nullptr); + CHECK(SpanningTree::findByPeerID(0xF3000004U) == nullptr); + CHECK(SpanningTree::countChildren(fixture.root) == 1U); +} + +TEST_CASE("SpanningTree deserializes and serializes a complete hierarchy", "[fne][stp]") +{ + TreeFixture fixture(0xF4000001U); + + json::array grandchildren = json::array(); + grandchildren.push_back(treeNode(0xF4000004U, 0xF4100004U)); + + json::array children = json::array(); + children.push_back(treeNode(0xF4000003U, 0xF4100003U, grandchildren)); + + json::array update = json::array(); + update.push_back(treeNode(0xF4000002U, 0xF4100002U, children)); + + std::vector duplicatePeers; + SpanningTree::deserializeTree(update, fixture.root, &duplicatePeers); + + SpanningTree* upstream = SpanningTree::findByPeerID(0xF4000002U); + SpanningTree* child = SpanningTree::findByPeerID(0xF4000003U); + SpanningTree* grandchild = SpanningTree::findByPeerID(0xF4000004U); + REQUIRE(upstream != nullptr); + REQUIRE(child != nullptr); + REQUIRE(grandchild != nullptr); + CHECK(duplicatePeers.empty()); + CHECK(upstream->m_parent == fixture.root); + CHECK(child->m_parent == upstream); + CHECK(grandchild->m_parent == child); + CHECK(grandchild->identity() == "test-peer-4093640708"); + CHECK(SpanningTree::countChildren(fixture.root) == 3U); + + json::array serialized = json::array(); + SpanningTree::serializeTree(fixture.root, serialized); + REQUIRE(serialized.size() == 1U); + + json::object root = serialized[0U].get(); + CHECK(root["id"].get() == fixture.root->id()); + CHECK(root["masterId"].get() == fixture.root->masterId()); + CHECK(root["identity"].get() == "test-root"); + + json::array serializedChildren = root["children"].get(); + REQUIRE(serializedChildren.size() == 1U); + json::object serializedUpstream = serializedChildren[0U].get(); + CHECK(serializedUpstream["id"].get() == upstream->id()); + REQUIRE(serializedUpstream["children"].get().size() == 1U); +} + +TEST_CASE("SpanningTree ignores malformed announcements and continues processing", "[fne][stp]") +{ + TreeFixture fixture(0xF5000001U); + + uint32_t missingMasterId = 0xF5000002U; + json::object missingMaster = json::object(); + missingMaster["id"].set(missingMasterId); + missingMaster["children"].set(json::array()); + + uint32_t invalidChildrenId = 0xF5000003U; + uint32_t invalidChildrenMasterId = 0xF5100003U; + json::object invalidChildren = json::object(); + invalidChildren["id"].set(invalidChildrenId); + invalidChildren["masterId"].set(invalidChildrenMasterId); + invalidChildren["children"].set("not-an-array"); + + json::array update = json::array(); + update.push_back(json::value(missingMaster)); + update.push_back(json::value(invalidChildren)); + update.push_back(treeNode(0xF5000004U, 0xF5100004U)); + + std::vector duplicatePeers; + SpanningTree::deserializeTree(update, fixture.root, &duplicatePeers); + + CHECK(duplicatePeers.empty()); + CHECK(SpanningTree::findByPeerID(0xF5000002U) == nullptr); + CHECK(SpanningTree::findByPeerID(0xF5000003U) == nullptr); + CHECK(SpanningTree::findByPeerID(0xF5000004U) != nullptr); + CHECK(SpanningTree::countChildren(fixture.root) == 1U); +} + +TEST_CASE("SpanningTree reports a duplicate master without adding the peer", "[fne][stp]") +{ + TreeFixture fixture(0xF6000001U); + SpanningTree* existing = new SpanningTree(0xF6000002U, 0xF6100002U, fixture.root); + + json::array update = json::array(); + update.push_back(treeNode(0xF6000003U, existing->masterId())); + + std::vector duplicatePeers; + SpanningTree::deserializeTree(update, fixture.root, &duplicatePeers); + + REQUIRE(duplicatePeers.size() == 1U); + CHECK(duplicatePeers[0U] == 0xF6000003U); + CHECK(SpanningTree::findByPeerID(0xF6000003U) == nullptr); + CHECK(SpanningTree::findByPeerID(existing->id()) == existing); + CHECK(SpanningTree::countChildren(fixture.root) == 1U); +} + +TEST_CASE("SpanningTree requires repeated updates before reparenting", "[fne][stp]") +{ + TreeFixture fixture(0xF7000001U); + SpanningTree* oldParent = new SpanningTree(0xF7000002U, 0xF7100002U, fixture.root); + SpanningTree* newParent = new SpanningTree(0xF7000003U, 0xF7100003U, fixture.root); + SpanningTree* moving = new SpanningTree(0xF7000004U, 0xF7100004U, oldParent); + + json::array update = json::array(); + update.push_back(treeNode(moving->id(), moving->masterId())); + std::vector duplicatePeers; + + for (uint32_t i = 0U; i < SpanningTree::s_maxUpdatesBeforeReparent; i++) { + SpanningTree::deserializeTree(update, newParent, &duplicatePeers); + CHECK(moving->m_parent == oldParent); + } + + SpanningTree::deserializeTree(update, newParent, &duplicatePeers); + + CHECK(duplicatePeers.empty()); + CHECK(moving->m_parent == newParent); + CHECK_FALSE(oldParent->hasChildren()); + REQUIRE(newParent->m_children.size() == 1U); + CHECK(newParent->m_children[0U] == moving); +} + +TEST_CASE("SpanningTree explicit reparenting preserves the moved subtree", "[fne][stp]") +{ + TreeFixture fixture(0xF8000001U); + SpanningTree* oldParent = new SpanningTree(0xF8000002U, 0xF8100002U, fixture.root); + SpanningTree* newParent = new SpanningTree(0xF8000003U, 0xF8100003U, fixture.root); + SpanningTree* moving = new SpanningTree(0xF8000004U, 0xF8100004U, oldParent); + SpanningTree* descendant = new SpanningTree(0xF8000005U, 0xF8100005U, moving); + + SpanningTree::moveParent(moving, newParent); + + CHECK(moving->m_parent == newParent); + CHECK(descendant->m_parent == moving); + CHECK_FALSE(oldParent->hasChildren()); + REQUIRE(newParent->m_children.size() == 1U); + CHECK(newParent->m_children[0U] == moving); + CHECK(SpanningTree::countChildren(fixture.root) == 4U); + + // Root nodes are never eligible for reparenting. + SpanningTree::moveParent(fixture.root, newParent); + CHECK(fixture.root->isRoot()); +}