From 85b4c3b4229ea3cf975790c123d760fe0808c39b Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Fri, 2 Mar 2018 17:10:14 -0800 Subject: [PATCH] Protect session UUID from concurrent read/writes --- domain-server/src/DomainServer.cpp | 2 +- domain-server/src/DomainServer.h | 2 +- libraries/networking/src/LimitedNodeList.cpp | 28 +++++++++----------- libraries/networking/src/LimitedNodeList.h | 17 +++++++----- libraries/networking/src/NodeList.cpp | 12 ++++----- 5 files changed, 31 insertions(+), 30 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index d2ef1a4156..c27ac09d15 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -405,7 +405,7 @@ void DomainServer::restart() { exit(DomainServer::EXIT_CODE_REBOOT); } -const QUuid& DomainServer::getID() { +QUuid DomainServer::getID() { return DependencyManager::get()->getSessionUUID(); } diff --git a/domain-server/src/DomainServer.h b/domain-server/src/DomainServer.h index afe2a1cc7c..6e9fe28c0a 100644 --- a/domain-server/src/DomainServer.h +++ b/domain-server/src/DomainServer.h @@ -135,7 +135,7 @@ signals: void userDisconnected(); private: - const QUuid& getID(); + QUuid getID(); void parseCommandLine(); QString getContentBackupDir(); diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 9dbbc570dd..0803e380f2 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -49,19 +49,8 @@ const std::set SOLO_NODE_TYPES = { }; LimitedNodeList::LimitedNodeList(int socketListenPort, int dtlsListenPort) : - _sessionUUID(), - _nodeHash(), - _nodeMutex(QReadWriteLock::Recursive), _nodeSocket(this), - _dtlsSocket(NULL), - _localSockAddr(), - _publicSockAddr(), - _stunSockAddr(STUN_SERVER_HOSTNAME, STUN_SERVER_PORT), - _packetReceiver(new PacketReceiver(this)), - _numCollectedPackets(0), - _numCollectedBytes(0), - _packetStatTimer(), - _permissions(NodePermissions()) + _packetReceiver(new PacketReceiver(this)) { qRegisterMetaType("ConnectionStep"); auto port = (socketListenPort != INVALID_PORT) ? socketListenPort : LIMITED_NODELIST_LOCAL_PORT.get(); @@ -122,13 +111,22 @@ LimitedNodeList::LimitedNodeList(int socketListenPort, int dtlsListenPort) : } } +QUuid LimitedNodeList::getSessionUUID() const { + QReadLocker lock { &_sessionUUIDLock }; + return _sessionUUID; +} + void LimitedNodeList::setSessionUUID(const QUuid& sessionUUID) { - QUuid oldUUID = _sessionUUID; - _sessionUUID = sessionUUID; + QUuid oldUUID; + { + QWriteLocker lock { &_sessionUUIDLock }; + oldUUID = _sessionUUID; + _sessionUUID = sessionUUID; + } if (sessionUUID != oldUUID) { qCDebug(networking) << "NodeList UUID changed from" << uuidStringWithoutCurlyBraces(oldUUID) - << "to" << uuidStringWithoutCurlyBraces(_sessionUUID); + << "to" << uuidStringWithoutCurlyBraces(sessionUUID); emit uuidChanged(sessionUUID, oldUUID); } } diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 1b1c809279..7165b3dd63 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -104,7 +104,7 @@ public: }; Q_ENUM(ConnectionStep); - const QUuid& getSessionUUID() const { return _sessionUUID; } + QUuid getSessionUUID() const; void setSessionUUID(const QUuid& sessionUUID); void setPermissions(const NodePermissions& newPermissions); @@ -380,20 +380,19 @@ protected: bool sockAddrBelongsToNode(const HifiSockAddr& sockAddr) { return findNodeWithAddr(sockAddr) != SharedNodePointer(); } - QUuid _sessionUUID; NodeHash _nodeHash; - mutable QReadWriteLock _nodeMutex; + mutable QReadWriteLock _nodeMutex { QReadWriteLock::Recursive }; udt::Socket _nodeSocket; - QUdpSocket* _dtlsSocket; + QUdpSocket* _dtlsSocket { nullptr }; HifiSockAddr _localSockAddr; HifiSockAddr _publicSockAddr; - HifiSockAddr _stunSockAddr; + HifiSockAddr _stunSockAddr { STUN_SERVER_HOSTNAME, STUN_SERVER_PORT }; bool _hasTCPCheckedLocalSocket { false }; PacketReceiver* _packetReceiver; - std::atomic _numCollectedPackets; - std::atomic _numCollectedBytes; + std::atomic _numCollectedPackets { 0 }; + std::atomic _numCollectedBytes { 0 }; QElapsedTimer _packetStatTimer; NodePermissions _permissions; @@ -424,6 +423,10 @@ private slots: void flagTimeForConnectionStep(ConnectionStep connectionStep, quint64 timestamp); void possiblyTimeoutSTUNAddressLookup(); void addSTUNHandlerToUnfiltered(); // called once STUN socket known + +private: + mutable QReadWriteLock _sessionUUIDLock; + QUuid _sessionUUID; }; #endif // hifi_LimitedNodeList_h diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 9ad0cb5e79..71d448ede9 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -798,7 +798,7 @@ void NodeList::sendIgnoreRadiusStateToNode(const SharedNodePointer& destinationN void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { // enumerate the nodes to send a reliable ignore packet to each that can leverage it - if (!nodeID.isNull() && _sessionUUID != nodeID) { + if (!nodeID.isNull() && getSessionUUID() != nodeID) { eachMatchingNode([](const SharedNodePointer& node)->bool { if (node->getType() == NodeType::AudioMixer || node->getType() == NodeType::AvatarMixer) { return true; @@ -851,7 +851,7 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { void NodeList::removeFromIgnoreMuteSets(const QUuid& nodeID) { // don't remove yourself, or nobody - if (!nodeID.isNull() && _sessionUUID != nodeID) { + if (!nodeID.isNull() && getSessionUUID() != nodeID) { { QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; _ignoredNodeIDs.unsafe_erase(nodeID); @@ -870,7 +870,7 @@ bool NodeList::isIgnoringNode(const QUuid& nodeID) const { void NodeList::personalMuteNodeBySessionID(const QUuid& nodeID, bool muteEnabled) { // cannot personal mute yourself, or nobody - if (!nodeID.isNull() && _sessionUUID != nodeID) { + if (!nodeID.isNull() && getSessionUUID() != nodeID) { auto audioMixer = soloNodeOfType(NodeType::AudioMixer); if (audioMixer) { if (isIgnoringNode(nodeID)) { @@ -970,7 +970,7 @@ void NodeList::maybeSendIgnoreSetToNode(SharedNodePointer newNode) { void NodeList::setAvatarGain(const QUuid& nodeID, float gain) { // cannot set gain of yourself - if (_sessionUUID != nodeID) { + if (getSessionUUID() != nodeID) { auto audioMixer = soloNodeOfType(NodeType::AudioMixer); if (audioMixer) { // setup the packet @@ -1013,7 +1013,7 @@ void NodeList::kickNodeBySessionID(const QUuid& nodeID) { // send a request to domain-server to kick the node with the given session ID // the domain-server will handle the persistence of the kick (via username or IP) - if (!nodeID.isNull() && _sessionUUID != nodeID ) { + if (!nodeID.isNull() && getSessionUUID() != nodeID ) { if (getThisNodeCanKick()) { // setup the packet auto kickPacket = NLPacket::create(PacketType::NodeKickRequest, NUM_BYTES_RFC4122_UUID, true); @@ -1036,7 +1036,7 @@ void NodeList::kickNodeBySessionID(const QUuid& nodeID) { void NodeList::muteNodeBySessionID(const QUuid& nodeID) { // cannot mute yourself, or nobody - if (!nodeID.isNull() && _sessionUUID != nodeID ) { + if (!nodeID.isNull() && getSessionUUID() != nodeID ) { if (getThisNodeCanKick()) { auto audioMixer = soloNodeOfType(NodeType::AudioMixer); if (audioMixer) {