Merge pull request #14147 from birarda/bug/large-traits

suppress traits larger than INT16_MAX bytes

assignment-client/src/avatars/AvatarMixerClientData.cpp

@@ -112,6 +112,11 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message,
             AvatarTraits::TraitWireSize traitSize;
             message.readPrimitive(&traitSize);
 
+            if (traitSize < -1 || traitSize > message.getBytesLeftToRead()) {
+                qWarning() << "Refusing to process simple trait of size" << traitSize << "from" << message.getSenderSockAddr();
+                break;
+            }
+
             if (packetTraitVersion > _lastReceivedTraitVersions[traitType]) {
                 _avatar->processTrait(traitType, message.read(traitSize));
                 _lastReceivedTraitVersions[traitType] = packetTraitVersion;
@@ -128,26 +133,41 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message,
         } else {
             AvatarTraits::TraitInstanceID instanceID = QUuid::fromRfc4122(message.readWithoutCopy(NUM_BYTES_RFC4122_UUID));
 
+            if (message.getBytesLeftToRead() == 0) {
+                qWarning () << "Received an instanced trait with no size from" << message.getSenderSockAddr();
+                break;
+            }
+
             AvatarTraits::TraitWireSize traitSize;
             message.readPrimitive(&traitSize);
 
-            auto& instanceVersionRef = _lastReceivedTraitVersions.getInstanceValueRef(traitType, instanceID);
+            if (traitSize < -1 || traitSize > message.getBytesLeftToRead()) {
+                qWarning() << "Refusing to process instanced trait of size" << traitSize << "from" << message.getSenderSockAddr();
+                break;
+            }
 
-            if (packetTraitVersion > instanceVersionRef) {
+            if (traitType == AvatarTraits::AvatarEntity) {
-                if (traitSize == AvatarTraits::DELETED_TRAIT_SIZE) {
+                auto& instanceVersionRef = _lastReceivedTraitVersions.getInstanceValueRef(traitType, instanceID);
-                    _avatar->processDeletedTraitInstance(traitType, instanceID);
 
-                    // to track a deleted instance but keep version information
+                if (packetTraitVersion > instanceVersionRef) {
-                    // the avatar mixer uses the negative value of the sent version
+                    if (traitSize == AvatarTraits::DELETED_TRAIT_SIZE) {
-                    instanceVersionRef = -packetTraitVersion;
+                        _avatar->processDeletedTraitInstance(traitType, instanceID);
+
+                        // to track a deleted instance but keep version information
+                        // the avatar mixer uses the negative value of the sent version
+                        instanceVersionRef = -packetTraitVersion;
+                    } else {
+                        _avatar->processTraitInstance(traitType, instanceID, message.read(traitSize));
+                        instanceVersionRef = packetTraitVersion;
+                    }
+
+                    anyTraitsChanged = true;
                 } else {
-                    _avatar->processTraitInstance(traitType, instanceID, message.read(traitSize));
+                    message.seek(message.getPosition() + traitSize);
-                    instanceVersionRef = packetTraitVersion;
                 }
-
-                anyTraitsChanged = true;
             } else {
-                message.seek(message.getPosition() + traitSize);
+                qWarning() << "Refusing to process traits packet with instanced trait of unprocessable type from" << message.getSenderSockAddr();
+                break;
             }
         }
     }

assignment-client/src/avatars/AvatarMixerSlave.cpp

@@ -152,6 +152,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis
                                                    });
 
                 if (!isDeleted && (sentInstanceIt == sentIDValuePairs.end() || receivedVersion > sentInstanceIt->value)) {
+
                     // this instance version exists and has never been sent or is newer so we need to send it
                     bytesWritten += sendingAvatar->packTraitInstance(traitType, instanceID, traitsPacketList, receivedVersion);
 
@@ -161,6 +162,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis
                         sentIDValuePairs.emplace_back(instanceID, receivedVersion);
                     }
                 } else if (isDeleted && sentInstanceIt != sentIDValuePairs.end() && absoluteReceivedVersion > sentInstanceIt->value) {
+
                     // this instance version was deleted and we haven't sent the delete to this client yet
                     bytesWritten += AvatarTraits::packInstancedTraitDelete(traitType, instanceID, traitsPacketList, absoluteReceivedVersion);
 
@@ -180,6 +182,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis
         listeningNodeData->setLastOtherAvatarTraitsSendPoint(otherNodeLocalID, timeOfLastTraitsChange);
     }
 
+
     return bytesWritten;
 }
 

libraries/avatars/src/AvatarData.cpp

@@ -1804,15 +1804,24 @@ QUrl AvatarData::getWireSafeSkeletonModelURL() const {
 
 qint64 AvatarData::packTrait(AvatarTraits::TraitType traitType, ExtendedIODevice& destination,
                            AvatarTraits::TraitVersion traitVersion) {
-    qint64 bytesWritten = 0;
-    bytesWritten += destination.writePrimitive(traitType);
 
-    if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) {
+    qint64 bytesWritten = 0;
-        bytesWritten += destination.writePrimitive(traitVersion);
-    }
 
     if (traitType == AvatarTraits::SkeletonModelURL) {
+
         QByteArray encodedSkeletonURL = getWireSafeSkeletonModelURL().toEncoded();
+
+        if (encodedSkeletonURL.size() > AvatarTraits::MAXIMUM_TRAIT_SIZE) {
+            qWarning() << "Refusing to pack simple trait" << traitType << "of size" << encodedSkeletonURL.size()
+                << "bytes since it exceeds the maximum size" << AvatarTraits::MAXIMUM_TRAIT_SIZE << "bytes";
+            return 0;
+        }
+
+        bytesWritten += destination.writePrimitive(traitType);
+
+        if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) {
+            bytesWritten += destination.writePrimitive(traitVersion);
+        }
         
         AvatarTraits::TraitWireSize encodedURLSize = encodedSkeletonURL.size();
         bytesWritten += destination.writePrimitive(encodedURLSize);
@@ -1827,14 +1836,6 @@ qint64 AvatarData::packTraitInstance(AvatarTraits::TraitType traitType, AvatarTr
                                    ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion) {
     qint64 bytesWritten = 0;
 
-    bytesWritten += destination.writePrimitive(traitType);
-
-    if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) {
-        bytesWritten += destination.writePrimitive(traitVersion);
-    }
-
-    bytesWritten += destination.write(traitInstanceID.toRfc4122());
-
     if (traitType == AvatarTraits::AvatarEntity) {
         // grab a read lock on the avatar entities and check for entity data for the given ID
         QByteArray entityBinaryData;
@@ -1845,6 +1846,20 @@ qint64 AvatarData::packTraitInstance(AvatarTraits::TraitType traitType, AvatarTr
             }
         });
 
+        if (entityBinaryData.size() > AvatarTraits::MAXIMUM_TRAIT_SIZE) {
+            qWarning() << "Refusing to pack instanced trait" << traitType << "of size" << entityBinaryData.size()
+                << "bytes since it exceeds the maximum size " << AvatarTraits::MAXIMUM_TRAIT_SIZE << "bytes";
+            return 0;
+        }
+
+        bytesWritten += destination.writePrimitive(traitType);
+
+        if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) {
+            bytesWritten += destination.writePrimitive(traitVersion);
+        }
+
+        bytesWritten += destination.write(traitInstanceID.toRfc4122());
+
         if (!entityBinaryData.isNull()) {
             AvatarTraits::TraitWireSize entityBinarySize = entityBinaryData.size();
 

libraries/avatars/src/AvatarHashMap.cpp

@@ -339,7 +339,7 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> mess
         // grab the last trait versions for this avatar
         auto& lastProcessedVersions = _processedTraitVersions[avatarID];
 
-        while (traitType != AvatarTraits::NullTrait) {
+        while (traitType != AvatarTraits::NullTrait && message->getBytesLeftToRead() > 0) {
             AvatarTraits::TraitVersion packetTraitVersion;
             message->readPrimitive(&packetTraitVersion);
 
@@ -380,7 +380,7 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> mess
                 }
             }
 
-            if (skipBinaryTrait) {
+            if (skipBinaryTrait && traitBinarySize > 0) {
                 // we didn't read this trait because it was older or because we didn't have an avatar to process it for
                 message->seek(message->getPosition() + traitBinarySize);
             }

libraries/avatars/src/AvatarTraits.h

@@ -39,6 +39,7 @@ namespace AvatarTraits {
 
     using TraitWireSize = int16_t;
     const TraitWireSize DELETED_TRAIT_SIZE = -1;
+    const TraitWireSize MAXIMUM_TRAIT_SIZE = INT16_MAX;
 
     inline qint64 packInstancedTraitDelete(TraitType traitType, TraitInstanceID instanceID, ExtendedIODevice& destination,
                                          TraitVersion traitVersion = NULL_TRAIT_VERSION) {

libraries/networking/src/NodeList.cpp

@@ -332,7 +332,6 @@ void NodeList::sendDomainServerCheckIn() {
                 qCDebug(networking) << "Local domain-server port read from shared memory (or default) is" << domainPort;
                 _domainHandler.setPort(domainPort);
             }
-
         }
 
         // check if we're missing a keypair we need to verify ourselves with the domain-server