From 895023ca4b5e114ca12e3d3946aeea2f82282bb9 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Mon, 5 Mar 2018 17:43:14 -0800 Subject: [PATCH 1/2] Fix unprotected tree traversals --- .../src/entities/EntityServer.cpp | 45 ++++++++++++------- .../src/octree/OctreeSendThread.cpp | 4 +- libraries/entities/src/EntityTree.cpp | 41 +++++++++++------ .../entities/src/SimpleEntitySimulation.cpp | 9 +++- 4 files changed, 65 insertions(+), 34 deletions(-) diff --git a/assignment-client/src/entities/EntityServer.cpp b/assignment-client/src/entities/EntityServer.cpp index d2bcbf2886..e3aca52ee5 100644 --- a/assignment-client/src/entities/EntityServer.cpp +++ b/assignment-client/src/entities/EntityServer.cpp @@ -363,7 +363,9 @@ void EntityServer::nodeAdded(SharedNodePointer node) { void EntityServer::nodeKilled(SharedNodePointer node) { EntityTreePointer tree = std::static_pointer_cast(_tree); - tree->deleteDescendantsOfAvatar(node->getUUID()); + tree->withWriteLock([&] { + tree->deleteDescendantsOfAvatar(node->getUUID()); + }); tree->forgetAvatarID(node->getUUID()); OctreeServer::nodeKilled(node); } @@ -451,8 +453,6 @@ void EntityServer::domainSettingsRequestFailed() { void EntityServer::startDynamicDomainVerification() { qCDebug(entities) << "Starting Dynamic Domain Verification..."; - QString thisDomainID = DependencyManager::get()->getDomainID().remove(QRegExp("\\{|\\}")); - EntityTreePointer tree = std::static_pointer_cast(_tree); QHash localMap(tree->getEntityCertificateIDMap()); @@ -460,15 +460,19 @@ void EntityServer::startDynamicDomainVerification() { qCDebug(entities) << localMap.size() << "entities in _entityCertificateIDMap"; while (i.hasNext()) { i.next(); + const auto& certificateID = i.key(); + const auto& entityID = i.value(); - EntityItemPointer entity = tree->findEntityByEntityItemID(i.value()); + EntityItemPointer entity = tree->findEntityByEntityItemID(entityID); if (entity) { if (!entity->getProperties().verifyStaticCertificateProperties()) { - qCDebug(entities) << "During Dynamic Domain Verification, a certified entity with ID" << i.value() << "failed" + qCDebug(entities) << "During Dynamic Domain Verification, a certified entity with ID" << entityID << "failed" << "static certificate verification."; // Delete the entity if it doesn't pass static certificate verification - tree->deleteEntity(i.value(), true); + tree->withWriteLock([&] { + tree->deleteEntity(entityID, true); + }); } else { QNetworkAccessManager& networkAccessManager = NetworkAccessManager::getInstance(); QNetworkRequest networkRequest; @@ -477,39 +481,46 @@ void EntityServer::startDynamicDomainVerification() { QUrl requestURL = NetworkingConstants::METAVERSE_SERVER_URL(); requestURL.setPath("/api/v1/commerce/proof_of_purchase_status/location"); QJsonObject request; - request["certificate_id"] = i.key(); + request["certificate_id"] = certificateID; networkRequest.setUrl(requestURL); - QNetworkReply* networkReply = NULL; - networkReply = networkAccessManager.put(networkRequest, QJsonDocument(request).toJson()); + QNetworkReply* networkReply = networkAccessManager.put(networkRequest, QJsonDocument(request).toJson()); + + connect(networkReply, &QNetworkReply::finished, this, [this, entityID, networkReply] { + EntityTreePointer tree = std::static_pointer_cast(_tree); - connect(networkReply, &QNetworkReply::finished, [=]() { QJsonObject jsonObject = QJsonDocument::fromJson(networkReply->readAll()).object(); jsonObject = jsonObject["data"].toObject(); if (networkReply->error() == QNetworkReply::NoError) { + QString thisDomainID = DependencyManager::get()->getDomainID().remove(QRegExp("\\{|\\}")); if (jsonObject["domain_id"].toString() != thisDomainID) { + EntityItemPointer entity = tree->findEntityByEntityItemID(entityID); if (entity->getAge() > (_MAXIMUM_DYNAMIC_DOMAIN_VERIFICATION_TIMER_MS/MSECS_PER_SECOND)) { qCDebug(entities) << "Entity's cert's domain ID" << jsonObject["domain_id"].toString() - << "doesn't match the current Domain ID" << thisDomainID << "; deleting entity" << i.value(); - tree->deleteEntity(i.value(), true); + << "doesn't match the current Domain ID" << thisDomainID << "; deleting entity" << entityID; + tree->withWriteLock([&] { + tree->deleteEntity(entityID, true); + }); } else { - qCDebug(entities) << "Entity failed dynamic domain verification, but was created too recently to necessitate deletion:" << i.value(); + qCDebug(entities) << "Entity failed dynamic domain verification, but was created too recently to necessitate deletion:" << entityID; } } else { - qCDebug(entities) << "Entity passed dynamic domain verification:" << i.value(); + qCDebug(entities) << "Entity passed dynamic domain verification:" << entityID; } } else { - qCDebug(entities) << "Call to" << networkReply->url() << "failed with error" << networkReply->error() << "; deleting entity" << i.value() + qCDebug(entities) << "Call to" << networkReply->url() << "failed with error" << networkReply->error() << "; deleting entity" << entityID << "More info:" << jsonObject; - tree->deleteEntity(i.value(), true); + tree->withWriteLock([&] { + tree->deleteEntity(entityID, true); + }); } networkReply->deleteLater(); }); } } else { - qCWarning(entities) << "During DDV, an entity with ID" << i.value() << "was NOT found in the Entity Tree!"; + qCWarning(entities) << "During DDV, an entity with ID" << entityID << "was NOT found in the Entity Tree!"; } } diff --git a/assignment-client/src/octree/OctreeSendThread.cpp b/assignment-client/src/octree/OctreeSendThread.cpp index d5b9da7353..715e83f403 100644 --- a/assignment-client/src/octree/OctreeSendThread.cpp +++ b/assignment-client/src/octree/OctreeSendThread.cpp @@ -400,7 +400,9 @@ int OctreeSendThread::packetDistributor(SharedNodePointer node, OctreeQueryNode* if (shouldTraverseAndSend(nodeData)) { quint64 start = usecTimestampNow(); - traverseTreeAndSendContents(node, nodeData, viewFrustumChanged, isFullScene); + _myServer->getOctree()->withReadLock([&]{ + traverseTreeAndSendContents(node, nodeData, viewFrustumChanged, isFullScene); + }); // Here's where we can/should allow the server to send other data... // send the environment packet diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 2cf66911a4..03258cd52d 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -1151,7 +1151,9 @@ void EntityTree::startChallengeOwnershipTimer(const EntityItemID& entityItemID) }); connect(_challengeOwnershipTimeoutTimer, &QTimer::timeout, this, [=]() { qCDebug(entities) << "Ownership challenge timed out, deleting entity" << entityItemID; - deleteEntity(entityItemID, true); + withWriteLock([&] { + deleteEntity(entityItemID, true); + }); if (_challengeOwnershipTimeoutTimer) { _challengeOwnershipTimeoutTimer->stop(); _challengeOwnershipTimeoutTimer->deleteLater(); @@ -1259,7 +1261,9 @@ void EntityTree::sendChallengeOwnershipPacket(const QString& certID, const QStri if (text == "") { qCDebug(entities) << "CRITICAL ERROR: Couldn't compute nonce. Deleting entity..."; - deleteEntity(entityItemID, true); + withWriteLock([&] { + deleteEntity(entityItemID, true); + }); } else { qCDebug(entities) << "Challenging ownership of Cert ID" << certID; // 2. Send the nonce to the rezzing avatar's node @@ -1322,8 +1326,7 @@ void EntityTree::validatePop(const QString& certID, const EntityItemID& entityIt request["certificate_id"] = certID; networkRequest.setUrl(requestURL); - QNetworkReply* networkReply = NULL; - networkReply = networkAccessManager.put(networkRequest, QJsonDocument(request).toJson()); + QNetworkReply* networkReply = networkAccessManager.put(networkRequest, QJsonDocument(request).toJson()); connect(networkReply, &QNetworkReply::finished, [=]() { QJsonObject jsonObject = QJsonDocument::fromJson(networkReply->readAll()).object(); @@ -1332,14 +1335,20 @@ void EntityTree::validatePop(const QString& certID, const EntityItemID& entityIt if (networkReply->error() == QNetworkReply::NoError) { if (!jsonObject["invalid_reason"].toString().isEmpty()) { qCDebug(entities) << "invalid_reason not empty, deleting entity" << entityItemID; - deleteEntity(entityItemID, true); + withWriteLock([&] { + deleteEntity(entityItemID, true); + }); } else if (jsonObject["transfer_status"].toArray().first().toString() == "failed") { qCDebug(entities) << "'transfer_status' is 'failed', deleting entity" << entityItemID; - deleteEntity(entityItemID, true); + withWriteLock([&] { + deleteEntity(entityItemID, true); + }); } else if (jsonObject["transfer_status"].toArray().first().toString() == "pending") { if (isRetryingValidation) { qCDebug(entities) << "'transfer_status' is 'pending' after retry, deleting entity" << entityItemID; - deleteEntity(entityItemID, true); + withWriteLock([&] { + deleteEntity(entityItemID, true); + }); } else { if (thread() != QThread::currentThread()) { QMetaObject::invokeMethod(this, "startPendingTransferStatusTimer", @@ -1362,7 +1371,9 @@ void EntityTree::validatePop(const QString& certID, const EntityItemID& entityIt } else { qCDebug(entities) << "Call to" << networkReply->url() << "failed with error" << networkReply->error() << "; deleting entity" << entityItemID << "More info:" << jsonObject; - deleteEntity(entityItemID, true); + withWriteLock([&] { + deleteEntity(entityItemID, true); + }); } networkReply->deleteLater(); @@ -1796,9 +1807,9 @@ void EntityTree::addToNeedsParentFixupList(EntityItemPointer entity) { void EntityTree::update(bool simulate) { PROFILE_RANGE(simulation_physics, "UpdateTree"); - fixupNeedsParentFixups(); - if (simulate && _simulation) { - withWriteLock([&] { + withWriteLock([&] { + fixupNeedsParentFixups(); + if (simulate && _simulation) { _simulation->updateEntities(); { PROFILE_RANGE(simulation_physics, "Deletes"); @@ -1816,8 +1827,8 @@ void EntityTree::update(bool simulate) { deleteEntities(idsToDelete, true); } } - }); - } + } + }); } quint64 EntityTree::getAdjustedConsiderSince(quint64 sinceTime) { @@ -2286,7 +2297,9 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer QScriptEngine scriptEngine; RecurseOctreeToMapOperator theOperator(entityDescription, element, &scriptEngine, skipDefaultValues, skipThoseWithBadParents, _myAvatar); - recurseTreeWithOperator(&theOperator); + withReadLock([&] { + recurseTreeWithOperator(&theOperator); + }); return true; } diff --git a/libraries/entities/src/SimpleEntitySimulation.cpp b/libraries/entities/src/SimpleEntitySimulation.cpp index 3203c2968c..4c01269ed5 100644 --- a/libraries/entities/src/SimpleEntitySimulation.cpp +++ b/libraries/entities/src/SimpleEntitySimulation.cpp @@ -42,8 +42,13 @@ void SimpleEntitySimulation::clearOwnership(const QUuid& ownerID) { // remove ownership and dirty all the tree elements that contain the it entity->clearSimulationOwnership(); entity->markAsChangedOnServer(); - DirtyOctreeElementOperator op(entity->getElement()); - getEntityTree()->recurseTreeWithOperator(&op); + if (auto element = entity->getElement()) { + auto tree = getEntityTree(); + tree->withReadLock([&] { + DirtyOctreeElementOperator op(element); + tree->recurseTreeWithOperator(&op); + }); + } } else { ++itemItr; } From ee6e9ee81e1f6b339585652092abba1dc63de68c Mon Sep 17 00:00:00 2001 From: Ken Cooke Date: Fri, 13 Apr 2018 10:36:13 -0700 Subject: [PATCH 2/2] Remove dead code left over from Gverb --- cmake/modules/FindGverb.cmake | 25 ----------- script-archive/example/audio/audioReverbOn.js | 43 ------------------- 2 files changed, 68 deletions(-) delete mode 100644 cmake/modules/FindGverb.cmake delete mode 100644 script-archive/example/audio/audioReverbOn.js diff --git a/cmake/modules/FindGverb.cmake b/cmake/modules/FindGverb.cmake deleted file mode 100644 index 0c149a7ca1..0000000000 --- a/cmake/modules/FindGverb.cmake +++ /dev/null @@ -1,25 +0,0 @@ -# FindGVerb.cmake -# -# Try to find the Gverb library. -# -# You must provide a GVERB_ROOT_DIR which contains src and include directories -# -# Once done this will define -# -# GVERB_FOUND - system found Gverb -# GVERB_INCLUDE_DIRS - the Gverb include directory -# -# Copyright 2014 High Fidelity, Inc. -# -# Distributed under the Apache License, Version 2.0. -# See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html -# - -include("${MACRO_DIR}/HifiLibrarySearchHints.cmake") -hifi_library_search_hints("gverb") - -find_path(GVERB_INCLUDE_DIRS gverb.h PATH_SUFFIXES include HINTS ${GVERB_SEARCH_DIRS}) -find_library(GVERB_LIBRARIES gverb PATH_SUFFIXES lib HINTS ${GVERB_SEARCH_DIRS}) - -include(FindPackageHandleStandardArgs) -find_package_handle_standard_args(Gverb DEFAULT_MSG GVERB_INCLUDE_DIRS GVERB_LIBRARIES) \ No newline at end of file diff --git a/script-archive/example/audio/audioReverbOn.js b/script-archive/example/audio/audioReverbOn.js deleted file mode 100644 index 72f2fa97bc..0000000000 --- a/script-archive/example/audio/audioReverbOn.js +++ /dev/null @@ -1,43 +0,0 @@ -// -// audioReverbOn.js -// examples -// -// Copyright 2014 High Fidelity, Inc. -// -// -// Gives the ability to be set various reverb settings. -// -// -// Distributed under the Apache License, Version 2.0. -// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html - -// http://wiki.audacityteam.org/wiki/GVerb#Instant_reverb_settings -var audioOptions = new AudioEffectOptions({ - // Square Meters - maxRoomSize: 50, - roomSize: 50, - - // Seconds - reverbTime: 10, - - // Between 0 - 1 - damping: 0.50, - inputBandwidth: 0.75, - - // dB - earlyLevel: -22, - tailLevel: -28, - dryLevel: 0, - wetLevel: 6 -}); - -AudioDevice.setReverbOptions(audioOptions); -AudioDevice.setReverb(true); -print("Reverb is now on with the updated options."); - -function scriptEnding() { - AudioDevice.setReverb(false); - print("Reberb is now off."); -} - -Script.scriptEnding.connect(scriptEnding); \ No newline at end of file