From e3acf861ff36e1de82412f177228b98818b1208d Mon Sep 17 00:00:00 2001
From: Stephen Birarda <commit@birarda.com>
Date: Thu, 7 May 2015 14:07:14 -0700
Subject: [PATCH] fix for unverified datagram writes
---
domain-server/src/DomainServer.cpp | 6 +--
libraries/networking/src/LimitedNodeList.cpp | 42 +++++++++++++++-----
libraries/networking/src/LimitedNodeList.h | 5 +--
libraries/networking/src/NodeList.cpp | 7 ++--
4 files changed, 41 insertions(+), 19 deletions(-)
diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp
index ea69b5bc95..7841381422 100644
--- a/domain-server/src/DomainServer.cpp
+++ b/domain-server/src/DomainServer.cpp
@@ -985,7 +985,7 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif
// we need to break here and start a new packet
// so send the current one
- limitedNodeList->writeDatagram(broadcastPacket, node, senderSockAddr);
+ limitedNodeList->writeUnverifiedDatagram(broadcastPacket, node, senderSockAddr);
// reset the broadcastPacket structure
broadcastPacket.resize(numBroadcastPacketLeadBytes);
@@ -998,9 +998,9 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif
});
}
}
-
+
// always write the last broadcastPacket
- limitedNodeList->writeDatagram(broadcastPacket, node, senderSockAddr);
+ limitedNodeList->writeUnverifiedDatagram(broadcastPacket, node, senderSockAddr);
}
void DomainServer::readAvailableDatagrams() {
diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp
index 5d04122d19..d8d1e9289b 100644
--- a/libraries/networking/src/LimitedNodeList.cpp
+++ b/libraries/networking/src/LimitedNodeList.cpp
@@ -230,8 +230,7 @@ qint64 LimitedNodeList::readDatagram(QByteArray& incomingPacket, QHostAddress* a
return result;
}
-qint64 LimitedNodeList::writeDatagram(const QByteArray& datagram, const HifiSockAddr& destinationSockAddr,
- const QUuid& connectionSecret) {
+qint64 LimitedNodeList::writeDatagram(const QByteArray& datagram, const HifiSockAddr& destinationSockAddr) {
// XXX can BandwidthRecorder be used for this?
// stat collection for packets
++_numCollectedPackets;
@@ -251,6 +250,12 @@ qint64 LimitedNodeList::writeDatagram(const QByteArray& datagram,
const SharedNodePointer& destinationNode,
const HifiSockAddr& overridenSockAddr) {
if (destinationNode) {
+ PacketType packetType = packetTypeForPacket(datagram);
+
+ if (NON_VERIFIED_PACKETS.contains(packetType)) {
+ return writeUnverifiedDatagram(datagram, destinationNode, overridenSockAddr);
+ }
+
// if we don't have an overridden address, assume they want to send to the node's active socket
const HifiSockAddr* destinationSockAddr = &overridenSockAddr;
if (overridenSockAddr.isNull()) {
@@ -264,8 +269,14 @@ qint64 LimitedNodeList::writeDatagram(const QByteArray& datagram,
}
QByteArray datagramCopy = datagram;
- PacketType packetType = packetTypeForPacket(datagramCopy);
+ // if we're here and the connection secret is null, debug out - this could be a problem
+ if (destinationNode->getConnectionSecret().isNull()) {
+ qDebug() << "LimitedNodeList::writeDatagram called for verified datagram with null connection secret for"
+ << "destination node" << destinationNode->getUUID() << " - this is either not secure or will cause"
+ << "this packet to be unverifiable on the receiving side.";
+ }
+
// perform replacement of hash and optionally also sequence number in the header
if (SEQUENCE_NUMBERED_PACKETS.contains(packetType)) {
PacketSequenceNumber sequenceNumber = getNextSequenceNumberForPacket(destinationNode->getUUID(), packetType);
@@ -276,7 +287,7 @@ qint64 LimitedNodeList::writeDatagram(const QByteArray& datagram,
}
emit dataSent(destinationNode->getType(), datagram.size());
- auto bytesWritten = writeDatagram(datagramCopy, *destinationSockAddr, destinationNode->getConnectionSecret());
+ auto bytesWritten = writeDatagram(datagramCopy, *destinationSockAddr);
// Keep track of per-destination-node bandwidth
destinationNode->recordBytesSent(bytesWritten);
return bytesWritten;
@@ -301,8 +312,21 @@ qint64 LimitedNodeList::writeUnverifiedDatagram(const QByteArray& datagram, cons
}
}
- // don't use the node secret!
- return writeDatagram(datagram, *destinationSockAddr, QUuid());
+ PacketType packetType = packetTypeForPacket(datagram);
+
+ // optionally peform sequence number replacement in the header
+ if (SEQUENCE_NUMBERED_PACKETS.contains(packetType)) {
+
+ QByteArray datagramCopy = datagram;
+
+ PacketSequenceNumber sequenceNumber = getNextSequenceNumberForPacket(destinationNode->getUUID(), packetType);
+ replaceSequenceNumberInPacket(datagramCopy, sequenceNumber, packetType);
+
+ // send the datagram with sequence number replaced in header
+ return writeDatagram(datagramCopy, *destinationSockAddr);
+ } else {
+ return writeDatagram(datagram, *destinationSockAddr);
+ }
}
// didn't have a destinationNode to send to, return 0
@@ -310,7 +334,7 @@ qint64 LimitedNodeList::writeUnverifiedDatagram(const QByteArray& datagram, cons
}
qint64 LimitedNodeList::writeUnverifiedDatagram(const QByteArray& datagram, const HifiSockAddr& destinationSockAddr) {
- return writeDatagram(datagram, destinationSockAddr, QUuid());
+ return writeDatagram(datagram, destinationSockAddr);
}
qint64 LimitedNodeList::writeDatagram(const char* data, qint64 size, const SharedNodePointer& destinationNode,
@@ -548,11 +572,11 @@ void LimitedNodeList::removeSilentNodes() {
eachNodeHashIterator([&](NodeHash::iterator& it){
SharedNodePointer node = it->second;
node->getMutex().lock();
-
+
if ((usecTimestampNow() - node->getLastHeardMicrostamp()) > (NODE_SILENCE_THRESHOLD_MSECS * USECS_PER_MSEC)) {
// call the NodeHash erase to get rid of this node
it = _nodeHash.unsafe_erase(it);
-
+
killedNodes.insert(node);
} else {
// we didn't erase this node, push the iterator forwards
diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h
index 7c710185b1..9f8bf690bf 100644
--- a/libraries/networking/src/LimitedNodeList.h
+++ b/libraries/networking/src/LimitedNodeList.h
@@ -114,6 +114,7 @@ public:
const HifiSockAddr& overridenSockAddr = HifiSockAddr());
qint64 writeUnverifiedDatagram(const QByteArray& datagram, const HifiSockAddr& destinationSockAddr);
+
qint64 writeDatagram(const char* data, qint64 size, const SharedNodePointer& destinationNode,
const HifiSockAddr& overridenSockAddr = HifiSockAddr());
@@ -233,9 +234,7 @@ protected:
LimitedNodeList(LimitedNodeList const&); // Don't implement, needed to avoid copies of singleton
void operator=(LimitedNodeList const&); // Don't implement, needed to avoid copies of singleton
- qint64 writeDatagram(const QByteArray& datagram,
- const HifiSockAddr& destinationSockAddr,
- const QUuid& connectionSecret);
+ qint64 writeDatagram(const QByteArray& datagram, const HifiSockAddr& destinationSockAddr);
PacketSequenceNumber getNextSequenceNumberForPacket(const QUuid& nodeUUID, PacketType packetType);
diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp
index 179f054e42..7a60c51986 100644
--- a/libraries/networking/src/NodeList.cpp
+++ b/libraries/networking/src/NodeList.cpp
@@ -369,7 +369,7 @@ void NodeList::sendDomainServerCheckIn() {
}
if (!isUsingDTLS) {
- writeDatagram(domainServerPacket, _domainHandler.getSockAddr(), QUuid());
+ writeUnverifiedDatagram(domainServerPacket, _domainHandler.getSockAddr());
}
const int NUM_DOMAIN_SERVER_CHECKINS_PER_STUN_REQUEST = 5;
@@ -424,10 +424,9 @@ int NodeList::processDomainServerList(const QByteArray& packet) {
_domainHandler.setUUID(uuidFromPacketHeader(packet));
_domainHandler.setIsConnected(true);
}
-
+
int readNodes = 0;
-
-
+
QDataStream packetStream(packet);
packetStream.skipRawData(numBytesForPacketHeader(packet));