From 8a0412524a37393b1f3465149aac593472007173 Mon Sep 17 00:00:00 2001 From: Bryan Biedenkapp Date: Fri, 8 Nov 2024 13:11:13 -0500 Subject: [PATCH 1/2] fix issue with DataBlock destructor trying to free a unallocated array; fix issue with dispatchUserFrameToFNE allowing local FNE network packets to cross external peer boundary; allow peers to connect if the peer list ACL is enabled, but the peer list itself is empty; --- configs/fne-config.example.yml | 2 + src/common/lookups/PeerListLookup.h | 6 +++ src/common/p25/data/DataBlock.cpp | 3 +- src/fne/network/FNENetwork.cpp | 21 ++++++-- src/fne/network/FNENetwork.h | 1 + .../callhandler/packetdata/P25PacketData.cpp | 49 ++++++++----------- .../callhandler/packetdata/P25PacketData.h | 2 +- 7 files changed, 50 insertions(+), 34 deletions(-) diff --git a/configs/fne-config.example.yml b/configs/fne-config.example.yml index 8264e5fc..19fa9d74 100644 --- a/configs/fne-config.example.yml +++ b/configs/fne-config.example.yml @@ -70,6 +70,8 @@ master: disablePacketData: false # Flag indicating whether verbose dumping of data packets is enabled. dumpPacketData: false + # Flag indicating whether verbose logging of data packet operations is enabled. + verbosePacketData: false # Delay from when a call on a parrot TG ends to when the playback starts (in milliseconds). parrotDelay: 2000 diff --git a/src/common/lookups/PeerListLookup.h b/src/common/lookups/PeerListLookup.h index 5fced292..eed88808 100644 --- a/src/common/lookups/PeerListLookup.h +++ b/src/common/lookups/PeerListLookup.h @@ -194,6 +194,12 @@ namespace lookups */ bool isPeerAllowed(uint32_t id) const; + /** + * @brief Checks if the peer list is empty. + * @returns bool True, if list is empty, otherwise false. + */ + bool isPeerListEmpty() const { return m_table.size() == 0U; } + /** * @brief Sets the mode to either WHITELIST or BLACKLIST. * @param mode The mode to set. diff --git a/src/common/p25/data/DataBlock.cpp b/src/common/p25/data/DataBlock.cpp index 782fcd94..cf6b44bc 100644 --- a/src/common/p25/data/DataBlock.cpp +++ b/src/common/p25/data/DataBlock.cpp @@ -42,7 +42,8 @@ DataBlock::DataBlock() : DataBlock::~DataBlock() { - delete[] m_data; + if (m_data != nullptr) + delete[] m_data; } /* Decodes P25 PDU data block. */ diff --git a/src/fne/network/FNENetwork.cpp b/src/fne/network/FNENetwork.cpp index 84104871..c239df4f 100644 --- a/src/fne/network/FNENetwork.cpp +++ b/src/fne/network/FNENetwork.cpp @@ -95,6 +95,7 @@ FNENetwork::FNENetwork(HostFNE* host, const std::string& address, uint16_t port, m_influxLogRawData(false), m_disablePacketData(false), m_dumpPacketData(false), + m_verbosePacketData(false), m_reportPeerPing(reportPeerPing), m_verbose(verbose) { @@ -154,6 +155,7 @@ void FNENetwork::setOptions(yaml::Node& conf, bool printOptions) m_disablePacketData = conf["disablePacketData"].as(false); m_dumpPacketData = conf["dumpPacketData"].as(false); + m_verbosePacketData = conf["verbosePacketData"].as(false); /* ** Drop Unit to Unit Peers @@ -585,7 +587,11 @@ void* FNENetwork::threadedNetworkRx(void* arg) // check if the peer is in the peer ACL list if (network->m_peerListLookup->getACL()) { - if (!network->m_peerListLookup->isPeerAllowed(peerId)) { + if (network->m_peerListLookup->isPeerListEmpty()) { + LogWarning(LOG_NET, "Peer List ACL enabled, but we have an empty peer list? Passing all peers."); + } + + if (!network->m_peerListLookup->isPeerAllowed(peerId) && !network->m_peerListLookup->isPeerListEmpty()) { if (network->m_peerListLookup->getMode() == lookups::PeerListLookup::BLACKLIST) { LogWarning(LOG_NET, "PEER %u RPTL, blacklisted from access", peerId); } else { @@ -619,7 +625,11 @@ void* FNENetwork::threadedNetworkRx(void* arg) // check if the peer is in the peer ACL list if (network->m_peerListLookup->getACL()) { - if (!network->m_peerListLookup->isPeerAllowed(peerId)) { + if (network->m_peerListLookup->isPeerListEmpty()) { + LogWarning(LOG_NET, "Peer List ACL enabled, but we have an empty peer list? Passing all peers."); + } + + if (!network->m_peerListLookup->isPeerAllowed(peerId) && !network->m_peerListLookup->isPeerListEmpty()) { if (network->m_peerListLookup->getMode() == lookups::PeerListLookup::BLACKLIST) { LogWarning(LOG_NET, "PEER %u RPTL, blacklisted from access", peerId); } else { @@ -675,7 +685,7 @@ void* FNENetwork::threadedNetworkRx(void* arg) // check if the peer is in the peer ACL list bool validAcl = true; if (network->m_peerListLookup->getACL()) { - if (!network->m_peerListLookup->isPeerAllowed(peerId)) { + if (!network->m_peerListLookup->isPeerAllowed(peerId) && !network->m_peerListLookup->isPeerListEmpty()) { if (network->m_peerListLookup->getMode() == lookups::PeerListLookup::BLACKLIST) { LogWarning(LOG_NET, "PEER %u RPTK, blacklisted from access", peerId); } else { @@ -694,6 +704,11 @@ void* FNENetwork::threadedNetworkRx(void* arg) } } } + + if (network->m_peerListLookup->isPeerListEmpty()) { + LogWarning(LOG_NET, "Peer List ACL enabled, but we have an empty peer list? Passing all peers."); + validAcl = true; + } } if (validAcl) { diff --git a/src/fne/network/FNENetwork.h b/src/fne/network/FNENetwork.h index a6ea52e6..c9e8238a 100644 --- a/src/fne/network/FNENetwork.h +++ b/src/fne/network/FNENetwork.h @@ -476,6 +476,7 @@ namespace network bool m_disablePacketData; bool m_dumpPacketData; + bool m_verbosePacketData; bool m_reportPeerPing; bool m_verbose; diff --git a/src/fne/network/callhandler/packetdata/P25PacketData.cpp b/src/fne/network/callhandler/packetdata/P25PacketData.cpp index a0671936..0d298743 100644 --- a/src/fne/network/callhandler/packetdata/P25PacketData.cpp +++ b/src/fne/network/callhandler/packetdata/P25PacketData.cpp @@ -683,10 +683,10 @@ void P25PacketData::dispatchToFNE(uint32_t peerId) } write_PDU_User(peer.first, nullptr, status->header, status->extendedAddress, status->pduUserData, true); - if (m_network->m_debug) { +// if (m_network->m_debug) { LogDebug(LOG_NET, "P25, srcPeer = %u, dstPeer = %u, duid = $%02X, srcId = %u, dstId = %u", peerId, peer.first, DUID::PDU, srcId, dstId); - } +// } i++; } @@ -713,16 +713,16 @@ void P25PacketData::dispatchToFNE(uint32_t peerId) } write_PDU_User(dstPeerId, peer.second, status->header, status->extendedAddress, status->pduUserData); - if (m_network->m_debug) { +// if (m_network->m_debug) { LogDebug(LOG_NET, "P25, srcPeer = %u, dstPeer = %u, duid = $%02X, srcId = %u, dstId = %u", peerId, dstPeerId, DUID::PDU, srcId, dstId); - } +// } } } } } -/* Helper to dispatch PDU user data back to the FNE network. */ +/* Helper to dispatch PDU user data back to the local FNE network. (Will not transmit to external peers.) */ void P25PacketData::dispatchUserFrameToFNE(p25::data::DataHeader& dataHeader, bool extendedAddress, uint8_t* pduUserData) { @@ -759,19 +759,6 @@ void P25PacketData::dispatchUserFrameToFNE(p25::data::DataHeader& dataHeader, bo } m_network->m_frameQueue->flushQueue(); } - - // repeat traffic to external peers - if (m_network->m_host->m_peerNetworks.size() > 0U) { - for (auto peer : m_network->m_host->m_peerNetworks) { - uint32_t dstPeerId = peer.second->getPeerId(); - - write_PDU_User(dstPeerId, peer.second, dataHeader, extendedAddress, pduUserData); - if (m_network->m_debug) { - LogDebug(LOG_NET, "P25, dstPeer = %u, duid = $%02X, srcId = %u, dstId = %u", - dstPeerId, DUID::PDU, srcId, dstId); - } - } - } } /* Helper used to process SNDCP control data from PDU data. */ @@ -962,10 +949,11 @@ void P25PacketData::write_PDU_User(uint32_t peerId, network::PeerNetwork* peerNe uint32_t blocksToFollow = dataHeader.getBlocksToFollow(); - LogMessage(LOG_NET, P25_PDU_STR ", OSP, peerId = %u, ack = %u, outbound = %u, fmt = $%02X, mfId = $%02X, sap = $%02X, fullMessage = %u, blocksToFollow = %u, padLength = %u, packetLength = %u, S = %u, n = %u, seqNo = %u, lastFragment = %u, hdrOffset = %u, llId = %u", - peerId, dataHeader.getAckNeeded(), dataHeader.getOutbound(), dataHeader.getFormat(), dataHeader.getMFId(), dataHeader.getSAP(), dataHeader.getFullMessage(), - dataHeader.getBlocksToFollow(), dataHeader.getPadLength(), dataHeader.getPacketLength(), dataHeader.getSynchronize(), dataHeader.getNs(), dataHeader.getFSN(), dataHeader.getLastFragment(), - dataHeader.getHeaderOffset(), dataHeader.getLLId()); + if (m_network->m_verbosePacketData) + LogMessage(LOG_NET, P25_PDU_STR ", OSP, peerId = %u, ack = %u, outbound = %u, fmt = $%02X, mfId = $%02X, sap = $%02X, fullMessage = %u, blocksToFollow = %u, padLength = %u, packetLength = %u, S = %u, n = %u, seqNo = %u, lastFragment = %u, hdrOffset = %u, llId = %u", + peerId, dataHeader.getAckNeeded(), dataHeader.getOutbound(), dataHeader.getFormat(), dataHeader.getMFId(), dataHeader.getSAP(), dataHeader.getFullMessage(), + dataHeader.getBlocksToFollow(), dataHeader.getPadLength(), dataHeader.getPacketLength(), dataHeader.getSynchronize(), dataHeader.getNs(), dataHeader.getFSN(), dataHeader.getLastFragment(), + dataHeader.getHeaderOffset(), dataHeader.getLLId()); // generate the PDU header and 1/2 rate Trellis dataHeader.encode(buffer); @@ -995,16 +983,18 @@ void P25PacketData::write_PDU_User(uint32_t peerId, network::PeerNetwork* peerNe blocksToFollow--; networkBlock++; - LogMessage(LOG_NET, P25_PDU_STR ", OSP, extended address, sap = $%02X, srcLlId = %u", - dataHeader.getEXSAP(), dataHeader.getSrcLLId()); + if (m_network->m_verbosePacketData) + LogMessage(LOG_NET, P25_PDU_STR ", OSP, extended address, sap = $%02X, srcLlId = %u", + dataHeader.getEXSAP(), dataHeader.getSrcLLId()); } // are we processing extended address data from the first block? if ((dataHeader.getFormat() == PDUFormatType::CONFIRMED) && (dataHeader.getSAP() == PDUSAP::EXT_ADDR) && extendedAddress) { dataHeader.encodeExtAddr(pduUserData); - LogMessage(LOG_NET, P25_PDU_STR ", OSP, sap = $%02X, srcLlId = %u", - dataHeader.getEXSAP(), dataHeader.getSrcLLId()); + if (m_network->m_verbosePacketData) + LogMessage(LOG_NET, P25_PDU_STR ", OSP, sap = $%02X, srcLlId = %u", + dataHeader.getEXSAP(), dataHeader.getSrcLLId()); } if (dataHeader.getFormat() != PDUFormatType::AMBT) { @@ -1022,9 +1012,10 @@ void P25PacketData::write_PDU_User(uint32_t peerId, network::PeerNetwork* peerNe dataBlock.setSerialNo(i); dataBlock.setData(pduUserData + dataOffset); - LogMessage(LOG_NET, P25_PDU_STR ", OSP, peerId = %u, block %u, fmt = $%02X, lastBlock = %u", - peerId, (dataHeader.getFormat() == PDUFormatType::CONFIRMED) ? dataBlock.getSerialNo() : i, dataBlock.getFormat(), - dataBlock.getLastBlock()); + if (m_network->m_verbosePacketData) + LogMessage(LOG_NET, P25_PDU_STR ", OSP, peerId = %u, block %u, fmt = $%02X, lastBlock = %u", + peerId, (dataHeader.getFormat() == PDUFormatType::CONFIRMED) ? dataBlock.getSerialNo() : i, dataBlock.getFormat(), + dataBlock.getLastBlock()); ::memset(buffer, 0x00U, P25_PDU_FEC_LENGTH_BYTES); dataBlock.encode(buffer); diff --git a/src/fne/network/callhandler/packetdata/P25PacketData.h b/src/fne/network/callhandler/packetdata/P25PacketData.h index b5187c7e..190ed778 100644 --- a/src/fne/network/callhandler/packetdata/P25PacketData.h +++ b/src/fne/network/callhandler/packetdata/P25PacketData.h @@ -185,7 +185,7 @@ namespace network */ void dispatchToFNE(uint32_t peerId); /** - * @brief Helper to dispatch PDU user data back to the FNE network. + * @brief Helper to dispatch PDU user data back to the local FNE network. (Will not transmit to external peers.) * @param dataHeader Instance of a PDU data header. * @param extendedAddress Flag indicating whether or not to extended addressing is in use. * @param pduUserData Buffer containing user data to transmit. From ccab09c1916d8bc9fe6012eca3621166020c2660 Mon Sep 17 00:00:00 2001 From: Bryan Biedenkapp Date: Fri, 8 Nov 2024 14:06:26 -0500 Subject: [PATCH 2/2] handle peer IDs better, Peer-Link will mangle some peer IDs and we need to check the peerId embedded in the status update instead of using the raw network peer Id; --- src/sysview/NodeStatusWnd.h | 4 +--- src/sysview/network/PeerNetwork.cpp | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/sysview/NodeStatusWnd.h b/src/sysview/NodeStatusWnd.h index 653855c0..8bf42a87 100644 --- a/src/sysview/NodeStatusWnd.h +++ b/src/sysview/NodeStatusWnd.h @@ -465,10 +465,8 @@ public: getNetwork()->unlockPeerStatus(); for (auto entry : peerStatus) { - uint32_t peerId = entry.first; json::object peerObj = entry.second; - if (peerObj["peerId"].is()) - peerId = peerObj["peerId"].get(); + uint32_t peerId = peerObj["peerId"].getDefault(entry.first); auto it = std::find_if(m_nodes.begin(), m_nodes.end(), [&](NodeStatusWidget* wdgt) { if (wdgt->peerId == peerId && wdgt->uniqueId == (int32_t)peerId) diff --git a/src/sysview/network/PeerNetwork.cpp b/src/sysview/network/PeerNetwork.cpp index f3128011..29f73048 100644 --- a/src/sysview/network/PeerNetwork.cpp +++ b/src/sysview/network/PeerNetwork.cpp @@ -111,8 +111,9 @@ void PeerNetwork::userPacketHandler(uint32_t peerId, FrameQueue::OpcodePair opco } json::object obj = v.get(); + uint32_t actualPeerId = obj["peerId"].getDefault(peerId); std::lock_guard lock(m_peerStatusMutex); - peerStatus[peerId] = obj; + peerStatus[actualPeerId] = obj; } break;