Merge pull request #12050 from highfidelity/RC61.1

Beta Release 61.1 - Includes up to Developer Release 7593

assignment-client/src/avatars/AvatarMixerSlave.cpp

@@ -214,7 +214,7 @@ void AvatarMixerSlave::broadcastAvatarDataToAgent(const SharedNodePointer& node)
         uint64_t getTimestamp() const override {
             return _lastEncodeTime;
         }
-        const AvatarSharedPointer& getAvatar() const { return _avatar; }
+        AvatarSharedPointer getAvatar() const { return _avatar; }
 
     private:
         AvatarSharedPointer _avatar;
@@ -326,7 +326,7 @@ void AvatarMixerSlave::broadcastAvatarDataToAgent(const SharedNodePointer& node)
 
     int remainingAvatars = (int)sortedAvatars.size();
     while (!sortedAvatars.empty()) {
-        const auto& avatarData = sortedAvatars.top().getAvatar();
+        const auto avatarData = sortedAvatars.top().getAvatar();
         sortedAvatars.pop();
         remainingAvatars--;
 

interface/src/avatar/AvatarManager.cpp

@@ -150,7 +150,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
         glm::vec3 getPosition() const override { return _avatar->getWorldPosition(); }
         float getRadius() const override { return std::static_pointer_cast<Avatar>(_avatar)->getBoundingRadius(); }
         uint64_t getTimestamp() const override { return std::static_pointer_cast<Avatar>(_avatar)->getLastRenderUpdateTime(); }
-        const AvatarSharedPointer& getAvatar() const { return _avatar; }
+        AvatarSharedPointer getAvatar() const { return _avatar; }
     private:
         AvatarSharedPointer _avatar;
     };
@@ -185,7 +185,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
     render::Transaction transaction;
     while (!sortedAvatars.empty()) {
         const SortableAvatar& sortData = sortedAvatars.top();
-        const auto& avatar = std::static_pointer_cast<Avatar>(sortData.getAvatar());
+        const auto avatar = std::static_pointer_cast<Avatar>(sortData.getAvatar());
 
         bool ignoring = DependencyManager::get<NodeList>()->isPersonalMutingNode(avatar->getID());
         if (ignoring) {
@@ -239,7 +239,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
             sortedAvatars.pop();
             while (inView && !sortedAvatars.empty()) {
                 const SortableAvatar& newSortData = sortedAvatars.top();
-                const auto& newAvatar = std::static_pointer_cast<Avatar>(newSortData.getAvatar());
+                const auto newAvatar = std::static_pointer_cast<Avatar>(newSortData.getAvatar());
                 inView = newSortData.getPriority() > OUT_OF_VIEW_THRESHOLD;
                 if (inView && newAvatar->hasNewJointData()) {
                     numAVatarsNotUpdated++;

interface/src/commerce/Ledger.cpp

@@ -61,7 +61,7 @@ void Ledger::send(const QString& endpoint, const QString& success, const QString
 
 void Ledger::signedSend(const QString& propertyName, const QByteArray& text, const QString& key, const QString& endpoint, const QString& success, const QString& fail, const bool controlled_failure) {
     auto wallet = DependencyManager::get<Wallet>();
-    QString signature = key.isEmpty() ? "" : wallet->signWithKey(text, key);
+    QString signature = wallet->signWithKey(text, key);
     QJsonObject request;
     request[propertyName] = QString(text);
     if (!controlled_failure) {

interface/src/commerce/Wallet.cpp

@@ -547,13 +547,16 @@ QStringList Wallet::listPublicKeys() {
 // the horror of code pages and so on (changing the bytes) by just returning a base64
 // encoded string representing the signature (suitable for http, etc...)
 QString Wallet::signWithKey(const QByteArray& text, const QString& key) {
-    qCInfo(commerce) << "Signing text" << text << "with key" << key;
     EC_KEY* ecPrivateKey = NULL;
+
+    auto keyFilePathString = keyFilePath().toStdString();
     if ((ecPrivateKey = readPrivateKey(keyFilePath().toStdString().c_str()))) {
         unsigned char* sig = new unsigned char[ECDSA_size(ecPrivateKey)];
 
         unsigned int signatureBytes = 0;
 
+        qCInfo(commerce) << "Hashing and signing plaintext" << text << "with key at address" << ecPrivateKey;
+
         QByteArray hashedPlaintext = QCryptographicHash::hash(text, QCryptographicHash::Sha256);
 
 
@@ -746,12 +749,10 @@ void Wallet::handleChallengeOwnershipPacket(QSharedPointer<ReceivedMessage> pack
     }
 
     EC_KEY_free(ec);
-    QByteArray ba = sig.toLocal8Bit();
-    const char *sigChar = ba.data();
 
     QByteArray textByteArray;
     if (status > -1) {
-        textByteArray = QByteArray(sigChar, (int) strlen(sigChar));
+        textByteArray = sig.toUtf8();
     }
     textByteArraySize = textByteArray.size();
     int certIDSize = certID.size();

libraries/entities-renderer/src/EntityTreeRenderer.cpp

@@ -349,7 +349,7 @@ void EntityTreeRenderer::updateChangedEntities(const render::ScenePointer& scene
             float getRadius() const override { return 0.5f * _renderer->getEntity()->getQueryAACube().getScale(); }
             uint64_t getTimestamp() const override { return _renderer->getUpdateTime(); }
 
-            const EntityRendererPointer& getRenderer() const { return _renderer; }
+            EntityRendererPointer getRenderer() const { return _renderer; }
         private:
             EntityRendererPointer _renderer;
         };
@@ -382,7 +382,7 @@ void EntityTreeRenderer::updateChangedEntities(const render::ScenePointer& scene
             std::unordered_map<EntityItemID, EntityRendererPointer>::iterator itr;
             size_t numSorted = sortedRenderables.size();
             while (!sortedRenderables.empty() && usecTimestampNow() < expiry) {
-                const EntityRendererPointer& renderable = sortedRenderables.top().getRenderer();
+                const auto renderable = sortedRenderables.top().getRenderer();
                 renderable->updateInScene(scene, transaction);
                 _renderablesToUpdate.erase(renderable->getEntity()->getID());
                 sortedRenderables.pop();

libraries/entities/src/EntityItemProperties.cpp

@@ -2513,7 +2513,8 @@ bool EntityItemProperties::verifySignature(const QString& publicKey, const QByte
         return false;
     }
 
-    const unsigned char* key = reinterpret_cast<const unsigned char*>(publicKey.toUtf8().constData());
+    auto keyByteArray = publicKey.toUtf8();
+    auto key = keyByteArray.constData();
     int keyLength = publicKey.length();
 
     BIO *bio = BIO_new_mem_buf((void*)key, keyLength);
@@ -2531,19 +2532,23 @@ bool EntityItemProperties::verifySignature(const QString& publicKey, const QByte
             // ECSDA verification prototype: note that type is currently ignored
             // int ECDSA_verify(int type, const unsigned char *dgst, int dgstlen,
             // const unsigned char *sig, int siglen, EC_KEY *eckey);
-            bool answer = ECDSA_verify(0,
+            int answer = ECDSA_verify(0,
                 digest,
                 digestLength,
                 signature,
                 signatureLength,
                 ec);
             long error = ERR_get_error();
-            if (error != 0) {
+            if (error != 0 || answer == -1) {
-                const char* error_str = ERR_error_string(error, NULL);
+                qCWarning(entities) << "ERROR while verifying signature!"
-                qCWarning(entities) << "ERROR while verifying signature! EC error:" << error_str
                     << "\nKey:" << publicKey << "\nutf8 Key Length:" << keyLength
                     << "\nDigest:" << digest << "\nDigest Length:" << digestLength
                     << "\nSignature:" << signature << "\nSignature Length:" << signatureLength;
+                while (error != 0) {
+                    const char* error_str = ERR_error_string(error, NULL);
+                    qCWarning(entities) << "EC error:" << error_str;
+                    error = ERR_get_error();
+                }
             }
             EC_KEY_free(ec);
             if (bio) {
@@ -2552,7 +2557,7 @@ bool EntityItemProperties::verifySignature(const QString& publicKey, const QByte
             if (evp_key) {
                 EVP_PKEY_free(evp_key);
             }
-            return answer;
+            return (answer == 1);
         } else {
             if (bio) {
                 BIO_free(bio);

libraries/entities/src/EntityTree.cpp

@@ -1189,13 +1189,15 @@ bool EntityTree::verifyNonce(const QString& certID, const QString& nonce, Entity
         key = sent.second;
     }
 
-    QString annotatedKey = "-----BEGIN PUBLIC KEY-----\n" + key.insert(64, "\n") + "\n-----END PUBLIC KEY-----";
+    QString annotatedKey = "-----BEGIN PUBLIC KEY-----\n" + key.insert(64, "\n") + "\n-----END PUBLIC KEY-----\n"; 
-    bool verificationSuccess = EntityItemProperties::verifySignature(annotatedKey.toUtf8(), actualNonce.toUtf8(), nonce.toUtf8());
+    QByteArray hashedActualNonce = QCryptographicHash::hash(QByteArray(actualNonce.toUtf8()), QCryptographicHash::Sha256);
+    bool verificationSuccess = EntityItemProperties::verifySignature(annotatedKey.toUtf8(), hashedActualNonce, QByteArray::fromBase64(nonce.toUtf8()));
 
     if (verificationSuccess) {
         qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "succeeded.";
     } else {
-        qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "failed for nonce" << actualNonce << "key" << key << "signature" << nonce;
+        qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "failed. Actual nonce:" << actualNonce <<
+            "\nHashed actual nonce (digest):" << hashedActualNonce << "\nSent nonce (signature)" << nonce << "\nKey" << key;
     }
 
     return verificationSuccess;

libraries/networking/src/udt/Connection.cpp

@@ -191,6 +191,8 @@ void Connection::queueReceivedMessagePacket(std::unique_ptr<Packet> packet) {
 
     pendingMessage.enqueuePacket(std::move(packet));
 
+    bool processedLastOrOnly = false;
+
     while (pendingMessage.hasAvailablePackets()) {
         auto packet = pendingMessage.removeNextPacket();
 
@@ -201,9 +203,13 @@ void Connection::queueReceivedMessagePacket(std::unique_ptr<Packet> packet) {
         // if this was the last or only packet, then we can remove the pending message from our hash
         if (packetPosition == Packet::PacketPosition::LAST ||
             packetPosition == Packet::PacketPosition::ONLY) {
-            _pendingReceivedMessages.erase(messageNumber);
+            processedLastOrOnly = true;
         }
     }
+
+    if (processedLastOrOnly) {
+        _pendingReceivedMessages.erase(messageNumber);
+    }
 }
 
 void Connection::sync() {

libraries/networking/src/udt/PacketHeaders.cpp

@@ -30,7 +30,7 @@ PacketVersion versionForPacketType(PacketType packetType) {
         case PacketType::EntityEdit:
         case PacketType::EntityData:
         case PacketType::EntityPhysics:
-            return static_cast<PacketVersion>(EntityVersion::HazeEffect);
+            return static_cast<PacketVersion>(EntityVersion::OwnershipChallengeFix);
 
         case PacketType::EntityQuery:
             return static_cast<PacketVersion>(EntityQueryPacketVersion::ConnectionIdentifier);

libraries/networking/src/udt/PacketHeaders.h

@@ -197,9 +197,10 @@ uint qHash(const PacketType& key, uint seed);
 QDebug operator<<(QDebug debug, const PacketType& type);
 
 enum class EntityVersion : PacketVersion {
-    StrokeColorProperty = 77,
+    StrokeColorProperty = 0,
     HasDynamicOwnershipTests,
-    HazeEffect
+    HazeEffect,
+    OwnershipChallengeFix
 };
 
 enum class EntityScriptCallMethodVersion : PacketVersion {

libraries/shared/src/PrioritySortUtil.h

@@ -28,7 +28,7 @@
         glm::vec3 getPosition() const override { return _thing->getPosition(); }
         float getRadius() const override { return 0.5f * _thing->getBoundingRadius(); }
         uint64_t getTimestamp() const override { return _thing->getLastTime(); }
-        const Thing& getThing() const { return _thing; }
+        Thing getThing() const { return _thing; }
     private:
         Thing _thing;
     };
@@ -43,6 +43,13 @@
 
 (3) Loop over your priority queue and do timeboxed work:
 
+    NOTE: Be careful using references to members of instances of T from std::priority_queue<T>.
+          Under the hood std::priority_queue<T> may re-use instances of T.
+          For example, after a pop() or a push() the top T may have the same memory address
+          as the top T before the pop() or push() (but point to a swapped instance of T).
+          This causes a reference to member variable of T to point to a different value
+          when operations taken on std::priority_queue<T> shuffle around the instances of T.
+
     uint64_t cutoffTime = usecTimestampNow() + TIME_BUDGET;
     while (!sortedThings.empty()) {
         const Thing& thing = sortedThings.top();