From 50b56ec7611ab6b1432578908eafa7c8756ec9de Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 14 Jun 2017 14:02:22 -0700 Subject: [PATCH] don't unpack avatar identity that is old --- assignment-client/src/avatars/AvatarMixer.cpp | 5 +- libraries/avatars/src/AvatarData.cpp | 131 ++++++++++-------- libraries/avatars/src/AvatarData.h | 22 ++- libraries/avatars/src/AvatarHashMap.cpp | 16 ++- .../networking/src/udt/PacketHeaders.cpp | 2 +- libraries/networking/src/udt/PacketHeaders.h | 3 +- libraries/networking/src/udt/SequenceNumber.h | 4 +- 7 files changed, 98 insertions(+), 85 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 337faf2027..ffeba6a2e8 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -510,12 +510,9 @@ void AvatarMixer::handleAvatarIdentityPacket(QSharedPointer mes AvatarData& avatar = nodeData->getAvatar(); // parse the identity packet and update the change timestamp if appropriate - AvatarData::Identity identity; - AvatarData::parseAvatarIdentityPacket(message->getMessage(), identity); - bool identityChanged = false; bool displayNameChanged = false; - avatar.processAvatarIdentity(identity, identityChanged, displayNameChanged); + avatar.processAvatarIdentity(message->getMessage(), identityChanged, displayNameChanged); if (identityChanged) { QMutexLocker nodeDataLocker(&nodeData->getMutex()); nodeData->flagIdentityChange(); diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 4407e12295..d523d1f5dc 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1473,27 +1473,6 @@ QStringList AvatarData::getJointNames() const { return _jointNames; } -void AvatarData::parseAvatarIdentityPacket(const QByteArray& data, Identity& identityOut) { - QDataStream packetStream(data); - - packetStream >> identityOut.uuid - >> identityOut.skeletonModelURL - >> identityOut.attachmentData - >> identityOut.displayName - >> identityOut.sessionDisplayName - >> identityOut.avatarEntityData - >> identityOut.sequenceId; - -#ifdef WANT_DEBUG - qCDebug(avatars) << __FUNCTION__ - << "identityOut.uuid:" << identityOut.uuid - << "identityOut.skeletonModelURL:" << identityOut.skeletonModelURL - << "identityOut.displayName:" << identityOut.displayName - << "identityOut.sessionDisplayName:" << identityOut.sessionDisplayName; -#endif - -} - glm::quat AvatarData::getOrientationOutbound() const { return (getLocalOrientation()); } @@ -1504,46 +1483,80 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { return _skeletonModelURL.scheme() == "file" ? emptyURL : _skeletonModelURL; } -void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged) { +void AvatarData::processAvatarIdentity(const QByteArray& identityData, bool& identityChanged, bool& displayNameChanged) { - if (identity.sequenceId < _identitySequenceId) { - qCDebug(avatars) << "Ignoring older identity packet for avatar" << getSessionUUID() - << "_identitySequenceId (" << _identitySequenceId << ") is greater than" << identity.sequenceId; - return; + QDataStream packetStream(identityData); + + QUuid avatarSessionID; + + // peek the sequence number, this will tell us if we should be processing this identity packet at all + udt::SequenceNumber::Type incomingSequenceNumberType; + packetStream >> avatarSessionID >> incomingSequenceNumberType; + udt::SequenceNumber incomingSequenceNumber(incomingSequenceNumberType); + + if (!_hasProcessedFirstIdentity) { + _identitySequenceNumber = incomingSequenceNumber - 1; + _hasProcessedFirstIdentity = true; + qCDebug(avatars) << "Processing first identity packet for" << avatarSessionID << "-" + << (udt::SequenceNumber::Type) incomingSequenceNumber; } - // otherwise, set the identitySequenceId to match the incoming identity - _identitySequenceId = identity.sequenceId; - if (_firstSkeletonCheck || (identity.skeletonModelURL != cannonicalSkeletonModelURL(emptyURL))) { - setSkeletonModelURL(identity.skeletonModelURL); - identityChanged = true; - if (_firstSkeletonCheck) { + if (incomingSequenceNumber > _identitySequenceNumber) { + Identity identity; + + packetStream >> identity.skeletonModelURL + >> identity.attachmentData + >> identity.displayName + >> identity.sessionDisplayName + >> identity.avatarEntityData; + + // set the store identity sequence number to match the incoming identity + _identitySequenceNumber = incomingSequenceNumber; + + if (_firstSkeletonCheck || (identity.skeletonModelURL != cannonicalSkeletonModelURL(emptyURL))) { + setSkeletonModelURL(identity.skeletonModelURL); + identityChanged = true; + if (_firstSkeletonCheck) { + displayNameChanged = true; + } + _firstSkeletonCheck = false; + } + + if (identity.displayName != _displayName) { + _displayName = identity.displayName; + identityChanged = true; displayNameChanged = true; } - _firstSkeletonCheck = false; - } + maybeUpdateSessionDisplayNameFromTransport(identity.sessionDisplayName); - if (identity.displayName != _displayName) { - _displayName = identity.displayName; - identityChanged = true; - displayNameChanged = true; - } - maybeUpdateSessionDisplayNameFromTransport(identity.sessionDisplayName); + if (identity.attachmentData != _attachmentData) { + setAttachmentData(identity.attachmentData); + identityChanged = true; + } - if (identity.attachmentData != _attachmentData) { - setAttachmentData(identity.attachmentData); - identityChanged = true; - } + bool avatarEntityDataChanged = false; + _avatarEntitiesLock.withReadLock([&] { + avatarEntityDataChanged = (identity.avatarEntityData != _avatarEntityData); + }); + + if (avatarEntityDataChanged) { + setAvatarEntityData(identity.avatarEntityData); + identityChanged = true; + } - bool avatarEntityDataChanged = false; - _avatarEntitiesLock.withReadLock([&] { - avatarEntityDataChanged = (identity.avatarEntityData != _avatarEntityData); - }); - if (avatarEntityDataChanged) { - setAvatarEntityData(identity.avatarEntityData); - identityChanged = true; - } +#ifdef WANT_DEBUG + qCDebug(avatars) << __FUNCTION__ + << "identity.uuid:" << identity.uuid + << "identity.skeletonModelURL:" << identity.skeletonModelURL + << "identity.displayName:" << identity.displayName + << "identity.sessionDisplayName:" << identity.sessionDisplayName; +#endif + } else { + qCDebug(avatars) << "Refusing to process identity for" << uuidStringWithoutCurlyBraces(avatarSessionID) << "since" + << (udt::SequenceNumber::Type) _identitySequenceNumber + << "is later than" << (udt::SequenceNumber::Type) incomingSequenceNumber; + } } QByteArray AvatarData::identityByteArray() const { @@ -1553,12 +1566,12 @@ QByteArray AvatarData::identityByteArray() const { _avatarEntitiesLock.withReadLock([&] { identityStream << getSessionUUID() - << urlToSend - << _attachmentData - << _displayName - << getSessionDisplayNameForTransport() // depends on _sessionDisplayName - << _avatarEntityData - << _identitySequenceId; + << (udt::SequenceNumber::Type) _identitySequenceNumber + << urlToSend + << _attachmentData + << _displayName + << getSessionDisplayNameForTransport() // depends on _sessionDisplayName + << _avatarEntityData; }); return identityData; @@ -2340,6 +2353,8 @@ void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) { _avatarEntityData = avatarEntityData; setAvatarEntityDataChanged(true); + qDebug() << "Current avatar entity data is" << _avatarEntityData.keys(); + foreach (auto entityID, previousAvatarEntityIDs) { if (!_avatarEntityData.contains(entityID)) { _avatarEntityDetached.insert(entityID); diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 4104615cfe..74aececa66 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -52,15 +52,16 @@ typedef unsigned long long quint64; #include #include #include -#include -#include -#include #include #include -#include +#include #include -#include +#include +#include +#include #include +#include +#include #include "AABox.h" #include "HeadData.h" @@ -525,20 +526,16 @@ public: const HeadData* getHeadData() const { return _headData; } struct Identity { - QUuid uuid; QUrl skeletonModelURL; QVector attachmentData; QString displayName; QString sessionDisplayName; AvatarEntityMap avatarEntityData; - quint64 sequenceId; }; - static void parseAvatarIdentityPacket(const QByteArray& data, Identity& identityOut); - // identityChanged returns true if identity has changed, false otherwise. // displayNameChanged returns true if displayName has changed, false otherwise. - void processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged); + void processAvatarIdentity(const QByteArray& identityData, bool& identityChanged, bool& displayNameChanged); QByteArray identityByteArray() const; @@ -626,7 +623,7 @@ public: bool getIdentityDataChanged() const { return _identityDataChanged; } // has the identity data changed since the last time sendIdentityPacket() was called void markIdentityDataChanged() { _identityDataChanged = true; - _identitySequenceId++; + ++_identitySequenceNumber; } float getDensity() const { return _density; } @@ -786,7 +783,8 @@ protected: float _audioAverageLoudness { 0.0f }; bool _identityDataChanged { false }; - quint64 _identitySequenceId { 0 }; + udt::SequenceNumber _identitySequenceNumber { 0 }; + bool _hasProcessedFirstIdentity { false }; float _density; private: diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 2ccc64fee2..cfbf2a8806 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -126,8 +126,9 @@ AvatarSharedPointer AvatarHashMap::parseAvatarData(QSharedPointer message, SharedNodePointer sendingNode) { - AvatarData::Identity identity; - AvatarData::parseAvatarIdentityPacket(message->getMessage(), identity); + + // peek the avatar UUID from the incoming packet + QUuid identityUUID = message->peek(NUM_BYTES_RFC4122_UUID); // make sure this isn't for an ignored avatar auto nodeList = DependencyManager::get(); @@ -136,20 +137,21 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer { QReadLocker locker(&_hashLock); auto me = _avatarHash.find(EMPTY); - if ((me != _avatarHash.end()) && (identity.uuid == me.value()->getSessionUUID())) { + if ((me != _avatarHash.end()) && (identityUUID == me.value()->getSessionUUID())) { // We add MyAvatar to _avatarHash with an empty UUID. Code relies on this. In order to correctly handle an // identity packet for ourself (such as when we are assigned a sessionDisplayName by the mixer upon joining), // we make things match here. - identity.uuid = EMPTY; + identityUUID = EMPTY; } } - if (!nodeList->isIgnoringNode(identity.uuid) || nodeList->getRequestsDomainListData()) { + + if (!nodeList->isIgnoringNode(identityUUID) || nodeList->getRequestsDomainListData()) { // mesh URL for a UUID, find avatar in our list - auto avatar = newOrExistingAvatar(identity.uuid, sendingNode); + auto avatar = newOrExistingAvatar(identityUUID, sendingNode); bool identityChanged = false; bool displayNameChanged = false; // In this case, the "sendingNode" is the Avatar Mixer. - avatar->processAvatarIdentity(identity, identityChanged, displayNameChanged); + avatar->processAvatarIdentity(message->getMessage(), identityChanged, displayNameChanged); } } diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index fc6251566e..7985df58bf 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -59,7 +59,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::AvatarData: case PacketType::BulkAvatarData: case PacketType::KillAvatar: - return static_cast(AvatarMixerPacketVersion::AvatarIdentitySequenceId); + return static_cast(AvatarMixerPacketVersion::AvatarIdentitySequenceFront); case PacketType::MessagesData: return static_cast(MessageDataVersion::TextOrBinaryData); case PacketType::ICEServerHeartbeat: diff --git a/libraries/networking/src/udt/PacketHeaders.h b/libraries/networking/src/udt/PacketHeaders.h index 9b37a7d76d..23cd3dd3e5 100644 --- a/libraries/networking/src/udt/PacketHeaders.h +++ b/libraries/networking/src/udt/PacketHeaders.h @@ -243,7 +243,8 @@ enum class AvatarMixerPacketVersion : PacketVersion { AvatarAsChildFixes, StickAndBallDefaultAvatar, IdentityPacketsIncludeUpdateTime, - AvatarIdentitySequenceId + AvatarIdentitySequenceId, + AvatarIdentitySequenceFront }; enum class DomainConnectRequestVersion : PacketVersion { diff --git a/libraries/networking/src/udt/SequenceNumber.h b/libraries/networking/src/udt/SequenceNumber.h index 3abc80bdd8..2c82eccfa3 100644 --- a/libraries/networking/src/udt/SequenceNumber.h +++ b/libraries/networking/src/udt/SequenceNumber.h @@ -35,8 +35,8 @@ public: explicit SequenceNumber(char* value) { _value = (*reinterpret_cast(value)) & MAX; } explicit SequenceNumber(Type value) { _value = (value <= MAX) ? ((value >= 0) ? value : 0) : MAX; } explicit SequenceNumber(UType value) { _value = (value <= MAX) ? value : MAX; } - explicit operator Type() { return _value; } - explicit operator UType() { return static_cast(_value); } + explicit operator Type() const { return _value; } + explicit operator UType() const { return static_cast(_value); } inline SequenceNumber& operator++() { _value = (_value + 1) % (MAX + 1);