Merge pull request #12571 from Atlante45/fix/es-concurrent-traversal

Fix unprotected tree traversals
This commit is contained in:
Seth Alves 2018-04-14 14:33:25 -07:00 committed by GitHub
commit 7f285c5f90
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 65 additions and 34 deletions

View file

@ -363,7 +363,9 @@ void EntityServer::nodeAdded(SharedNodePointer node) {
void EntityServer::nodeKilled(SharedNodePointer node) {
EntityTreePointer tree = std::static_pointer_cast<EntityTree>(_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<AddressManager>()->getDomainID().remove(QRegExp("\\{|\\}"));
EntityTreePointer tree = std::static_pointer_cast<EntityTree>(_tree);
QHash<QString, EntityItemID> 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<EntityTree>(_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<AddressManager>()->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!";
}
}

View file

@ -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

View file

@ -1155,7 +1155,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();
@ -1263,7 +1265,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
@ -1326,8 +1330,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();
@ -1336,14 +1339,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",
@ -1366,7 +1375,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();
@ -1800,9 +1811,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");
@ -1820,8 +1831,8 @@ void EntityTree::update(bool simulate) {
deleteEntities(idsToDelete, true);
}
}
});
}
}
});
}
quint64 EntityTree::getAdjustedConsiderSince(quint64 sinceTime) {
@ -2290,7 +2301,9 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer
QScriptEngine scriptEngine;
RecurseOctreeToMapOperator theOperator(entityDescription, element, &scriptEngine, skipDefaultValues,
skipThoseWithBadParents, _myAvatar);
recurseTreeWithOperator(&theOperator);
withReadLock([&] {
recurseTreeWithOperator(&theOperator);
});
return true;
}

View file

@ -37,8 +37,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;
}