From dfe4b2d2759e5a1af43f5f2a74b6416ce5dab229 Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Tue, 14 Mar 2023 20:40:06 +0100 Subject: MamManager: Create task before sending --- src/client/QXmppMamManager.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/client/QXmppMamManager.cpp') diff --git a/src/client/QXmppMamManager.cpp b/src/client/QXmppMamManager.cpp index c39a3411..a63c997d 100644 --- a/src/client/QXmppMamManager.cpp +++ b/src/client/QXmppMamManager.cpp @@ -243,6 +243,9 @@ QXmppTask QXmppMamManager::retrieveMessages(con auto [itr, _] = d->ongoingRequests.insert({ queryIq.queryId().toStdString(), RetrieveRequestState() }); + // create task here; promise could finish immediately after client()->sendIq() + auto task = itr->second.promise.task(); + // retrieve messages client()->sendIq(std::move(queryIq)).then(this, [this, queryId = queryIq.queryId()](QXmppClient::IqResult result) { auto itr = d->ongoingRequests.find(queryId.toStdString()); @@ -309,5 +312,5 @@ QXmppTask QXmppMamManager::retrieveMessages(con } }); - return itr->second.promise.task(); + return task; } -- cgit v1.2.3 From 567b58a7a6dabf105a4d013b07c9937fbd68fd24 Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Tue, 14 Mar 2023 20:42:50 +0100 Subject: MamManager: Avoid possible use-after-move --- src/client/QXmppMamManager.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src/client/QXmppMamManager.cpp') diff --git a/src/client/QXmppMamManager.cpp b/src/client/QXmppMamManager.cpp index a63c997d..a5b1ce5c 100644 --- a/src/client/QXmppMamManager.cpp +++ b/src/client/QXmppMamManager.cpp @@ -240,14 +240,16 @@ QString QXmppMamManager::retrieveArchivedMessages(const QString &to, QXmppTask QXmppMamManager::retrieveMessages(const QString &to, const QString &node, const QString &jid, const QDateTime &start, const QDateTime &end, const QXmppResultSetQuery &resultSetQuery) { auto queryIq = buildRequest(to, node, jid, start, end, resultSetQuery); + auto queryId = queryIq.queryId(); - auto [itr, _] = d->ongoingRequests.insert({ queryIq.queryId().toStdString(), RetrieveRequestState() }); + auto [itr, inserted] = d->ongoingRequests.insert({ queryIq.queryId().toStdString(), RetrieveRequestState() }); + Q_ASSERT(inserted); // create task here; promise could finish immediately after client()->sendIq() auto task = itr->second.promise.task(); // retrieve messages - client()->sendIq(std::move(queryIq)).then(this, [this, queryId = queryIq.queryId()](QXmppClient::IqResult result) { + client()->sendIq(std::move(queryIq)).then(this, [this, queryId](QXmppClient::IqResult result) { auto itr = d->ongoingRequests.find(queryId.toStdString()); if (itr == d->ongoingRequests.end()) { return; -- cgit v1.2.3 From fa45d75aaadfe9e1d07911cd9483932abaa55117 Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Tue, 14 Mar 2023 20:52:40 +0100 Subject: MamManager: Move parsing from handleStanza() into new function --- src/client/QXmppMamManager.cpp | 55 +++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 22 deletions(-) (limited to 'src/client/QXmppMamManager.cpp') diff --git a/src/client/QXmppMamManager.cpp b/src/client/QXmppMamManager.cpp index a5b1ce5c..d96b2791 100644 --- a/src/client/QXmppMamManager.cpp +++ b/src/client/QXmppMamManager.cpp @@ -20,6 +20,37 @@ using namespace QXmpp::Private; +std::optional> parseMamMessageResult(const QDomElement &messageEl) +{ + auto resultElement = messageEl.firstChildElement("result"); + if (resultElement.isNull() || resultElement.namespaceURI() != ns_mam) { + return {}; + } + + auto forwardedElement = resultElement.firstChildElement("forwarded"); + if (forwardedElement.isNull() || forwardedElement.namespaceURI() != ns_forwarding) { + return {}; + } + + auto queryId = resultElement.attribute("queryid"); + + auto messageElement = forwardedElement.firstChildElement("message"); + if (messageElement.isNull()) { + return {}; + } + + QXmppMessage message; + message.parse(messageElement); + + auto delayElement = forwardedElement.firstChildElement("delay"); + if (!delayElement.isNull() && delayElement.namespaceURI() == ns_delayed_delivery) { + const auto stamp = delayElement.attribute("stamp"); + message.setStamp(QXmppUtils::datetimeFromString(stamp)); + } + + return { { message, queryId } }; +} + struct RetrieveRequestState { QXmppPromise promise; @@ -87,28 +118,8 @@ QStringList QXmppMamManager::discoveryFeatures() const bool QXmppMamManager::handleStanza(const QDomElement &element) { if (element.tagName() == "message") { - QDomElement resultElement = element.firstChildElement("result"); - if (!resultElement.isNull() && resultElement.namespaceURI() == ns_mam) { - QDomElement forwardedElement = resultElement.firstChildElement("forwarded"); - QString queryId = resultElement.attribute("queryid"); - - if (forwardedElement.isNull() || forwardedElement.namespaceURI() != ns_forwarding) { - return false; - } - - auto messageElement = forwardedElement.firstChildElement("message"); - auto delayElement = forwardedElement.firstChildElement("delay"); - - if (messageElement.isNull()) { - return false; - } - - QXmppMessage message; - message.parse(messageElement); - if (!delayElement.isNull() && delayElement.namespaceURI() == ns_delayed_delivery) { - const QString stamp = delayElement.attribute("stamp"); - message.setStamp(QXmppUtils::datetimeFromString(stamp)); - } + if (auto result = parseMamMessageResult(element)) { + auto &[message, queryId] = *result; auto itr = d->ongoingRequests.find(queryId.toStdString()); if (itr != d->ongoingRequests.end()) { -- cgit v1.2.3 From 672af91550cbfbe622a058de32df58e805468f1e Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Tue, 14 Mar 2023 21:12:43 +0100 Subject: MamManager: Flatten retrieveMessages code --- src/client/QXmppMamManager.cpp | 108 ++++++++++++++++++++++------------------- 1 file changed, 58 insertions(+), 50 deletions(-) (limited to 'src/client/QXmppMamManager.cpp') diff --git a/src/client/QXmppMamManager.cpp b/src/client/QXmppMamManager.cpp index d96b2791..2bd0db23 100644 --- a/src/client/QXmppMamManager.cpp +++ b/src/client/QXmppMamManager.cpp @@ -265,64 +265,72 @@ QXmppTask QXmppMamManager::retrieveMessages(con if (itr == d->ongoingRequests.end()) { return; } + auto &state = itr->second; - if (std::holds_alternative(result)) { - auto &iq = itr->second.iq; - iq.parse(std::get(result)); + // handle IQ sending errors + if (std::holds_alternative(result)) { + state.promise.finish(std::get(result)); + d->ongoingRequests.erase(itr); + return; + } - if (iq.type() == QXmppIq::Error) { - itr->second.promise.finish(QXmppError { iq.error().text(), iq.error() }); - d->ongoingRequests.erase(itr); - return; - } + // parse IQ + auto &iq = state.iq; + iq.parse(std::get(result)); + + // handle MAM error result IQ + if (iq.type() == QXmppIq::Error) { + state.promise.finish(QXmppError { iq.error().text(), iq.error() }); + d->ongoingRequests.erase(itr); + return; + } - // decrypt encrypted messages - if (auto *e2eeExt = client()->encryptionExtension()) { - auto &messages = itr->second.messages; - auto running = std::make_shared(0); - // handle case when no message is encrypted - auto hasEncryptedMessages = false; + // decrypt encrypted messages + if (auto *e2eeExt = client()->encryptionExtension()) { + auto &messages = state.messages; + auto running = std::make_shared(0); - for (auto i = 0; i < messages.size(); i++) { - if (!e2eeExt->isEncrypted(messages.at(i))) { - continue; - } - hasEncryptedMessages = true; - - auto message = messages.at(i); - (*running)++; - e2eeExt->decryptMessage(std::move(message)).then(this, [this, i, running, queryId](auto result) { - (*running)--; - auto itr = d->ongoingRequests.find(queryId.toStdString()); - if (itr == d->ongoingRequests.end()) { - return; - } - - if (std::holds_alternative(result)) { - itr->second.messages[i] = std::get(std::move(result)); - } else { - warning(QStringLiteral("Error decrypting message.")); - } - if (*running == 0) { - itr->second.finish(); - d->ongoingRequests.erase(itr); - } - }); - } + // handle case when no message is encrypted + auto hasEncryptedMessages = false; - if (!hasEncryptedMessages) { - // finish here, no decryptMessage callback will do it - itr->second.finish(); - d->ongoingRequests.erase(itr); + for (auto i = 0; i < messages.size(); i++) { + if (!e2eeExt->isEncrypted(messages.at(i))) { + continue; } - } else { - itr->second.finish(); - d->ongoingRequests.erase(itr); + hasEncryptedMessages = true; + + auto message = messages.at(i); + (*running)++; + e2eeExt->decryptMessage(std::move(message)).then(this, [this, i, running, queryId](auto result) { + (*running)--; + + auto itr = d->ongoingRequests.find(queryId.toStdString()); + if (itr == d->ongoingRequests.end()) { + return; + } + auto &state = itr->second; + + if (std::holds_alternative(result)) { + state.messages[i] = std::get(std::move(result)); + } else { + warning(QStringLiteral("Error decrypting message.")); + } + + if (*running == 0) { + state.finish(); + d->ongoingRequests.erase(itr); + } + }); + } + + // finishing the promise is done after decryptMessage() + if (hasEncryptedMessages) { + return; } - } else { - itr->second.promise.finish(std::get(result)); - d->ongoingRequests.erase(itr); } + + state.finish(); + d->ongoingRequests.erase(itr); }); return task; -- cgit v1.2.3 From 154ac6b989aeee66eb2d3b87802faf009b90a92b Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Tue, 14 Mar 2023 21:51:52 +0100 Subject: MamManager: Avoid shared_ptr for counting running jobs --- src/client/QXmppMamManager.cpp | 48 ++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 14 deletions(-) (limited to 'src/client/QXmppMamManager.cpp') diff --git a/src/client/QXmppMamManager.cpp b/src/client/QXmppMamManager.cpp index 2bd0db23..d7f63eaf 100644 --- a/src/client/QXmppMamManager.cpp +++ b/src/client/QXmppMamManager.cpp @@ -20,6 +20,22 @@ using namespace QXmpp::Private; +template +auto transform(const T &input, Converter convert) +{ + using Output = std::decay_t; + QVector output; + output.reserve(input.size()); + std::transform(input.begin(), input.end(), std::back_inserter(output), std::move(convert)); + return output; +} + +template +auto sum(const T &c) +{ + return std::accumulate(c.begin(), c.end(), 0); +} + std::optional> parseMamMessageResult(const QDomElement &messageEl) { auto resultElement = messageEl.firstChildElement("result"); @@ -56,6 +72,7 @@ struct RetrieveRequestState QXmppPromise promise; QXmppMamResultIq iq; QVector messages; + uint runningDecryptionJobs = 0; void finish() { @@ -288,26 +305,28 @@ QXmppTask QXmppMamManager::retrieveMessages(con // decrypt encrypted messages if (auto *e2eeExt = client()->encryptionExtension()) { auto &messages = state.messages; - auto running = std::make_shared(0); - // handle case when no message is encrypted - auto hasEncryptedMessages = false; + // check for encrypted messages (once) + auto messagesEncrypted = transform(state.messages, [&](const auto &m) { + return e2eeExt->isEncrypted(m); + }); + auto encryptedCount = sum(messagesEncrypted); + + // We can't do this on the fly (with ++ and --) in the for loop + // because some decryptMessage() jobs could finish instantly + state.runningDecryptionJobs = encryptedCount; for (auto i = 0; i < messages.size(); i++) { - if (!e2eeExt->isEncrypted(messages.at(i))) { + if (!messagesEncrypted[i]) { continue; } - hasEncryptedMessages = true; auto message = messages.at(i); - (*running)++; - e2eeExt->decryptMessage(std::move(message)).then(this, [this, i, running, queryId](auto result) { - (*running)--; - + state.runningDecryptionJobs++; + e2eeExt->decryptMessage(std::move(message)).then(this, [this, i, queryId](auto result) { auto itr = d->ongoingRequests.find(queryId.toStdString()); - if (itr == d->ongoingRequests.end()) { - return; - } + Q_ASSERT(itr != d->ongoingRequests.end()); + auto &state = itr->second; if (std::holds_alternative(result)) { @@ -316,7 +335,8 @@ QXmppTask QXmppMamManager::retrieveMessages(con warning(QStringLiteral("Error decrypting message.")); } - if (*running == 0) { + state.runningDecryptionJobs--; + if (state.runningDecryptionJobs == 0) { state.finish(); d->ongoingRequests.erase(itr); } @@ -324,7 +344,7 @@ QXmppTask QXmppMamManager::retrieveMessages(con } // finishing the promise is done after decryptMessage() - if (hasEncryptedMessages) { + if (encryptedCount > 0) { return; } } -- cgit v1.2.3 From 054b35de3ea9251ca713209ea73b2b814fb6c0bc Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Tue, 14 Mar 2023 22:26:50 +0100 Subject: MamManager: Only parse ScePublic when decrypting messages Message that are end-to-end-encrypted need to be parsed in ScePublic mode and should then be decrypted. In case the decryption fails, the messages are parsed in SceAll (the normal mode). --- src/client/QXmppMamManager.cpp | 70 +++++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 21 deletions(-) (limited to 'src/client/QXmppMamManager.cpp') diff --git a/src/client/QXmppMamManager.cpp b/src/client/QXmppMamManager.cpp index d7f63eaf..fe0735c7 100644 --- a/src/client/QXmppMamManager.cpp +++ b/src/client/QXmppMamManager.cpp @@ -9,15 +9,16 @@ #include "QXmppConstants_p.h" #include "QXmppDataForm.h" #include "QXmppE2eeExtension.h" -#include "QXmppFutureUtils_p.h" #include "QXmppMamIq.h" #include "QXmppMessage.h" +#include "QXmppPromise.h" #include "QXmppUtils.h" #include #include +using namespace QXmpp; using namespace QXmpp::Private; template @@ -36,7 +37,26 @@ auto sum(const T &c) return std::accumulate(c.begin(), c.end(), 0); } -std::optional> parseMamMessageResult(const QDomElement &messageEl) +struct MamMessage +{ + QDomElement element; + std::optional delay; +}; + +enum EncryptedType { Unencrypted, + Encrypted }; + +QXmppMessage parseMamMessage(const MamMessage &mamMessage, EncryptedType encrypted) +{ + QXmppMessage m; + m.parse(mamMessage.element, encrypted == Encrypted ? ScePublic : SceAll); + if (mamMessage.delay) { + m.setStamp(*mamMessage.delay); + } + return m; +} + +std::optional> parseMamMessageResult(const QDomElement &messageEl) { auto resultElement = messageEl.firstChildElement("result"); if (resultElement.isNull() || resultElement.namespaceURI() != ns_mam) { @@ -55,31 +75,32 @@ std::optional> parseMamMessageResult(const QDo return {}; } - QXmppMessage message; - message.parse(messageElement); - - auto delayElement = forwardedElement.firstChildElement("delay"); - if (!delayElement.isNull() && delayElement.namespaceURI() == ns_delayed_delivery) { - const auto stamp = delayElement.attribute("stamp"); - message.setStamp(QXmppUtils::datetimeFromString(stamp)); - } + auto parseDelay = [](const auto &forwardedEl) -> std::optional { + auto delayEl = forwardedEl.firstChildElement("delay"); + if (!delayEl.isNull() && delayEl.namespaceURI() == ns_delayed_delivery) { + return QXmppUtils::datetimeFromString(delayEl.attribute("stamp")); + } + return {}; + }; - return { { message, queryId } }; + return { { MamMessage { messageElement, parseDelay(forwardedElement) }, queryId } }; } struct RetrieveRequestState { QXmppPromise promise; QXmppMamResultIq iq; - QVector messages; + QVector messages; + QVector processedMessages; uint runningDecryptionJobs = 0; void finish() { + Q_ASSERT(messages.count() == processedMessages.count()); promise.finish( QXmppMamManager::RetrievedMessages { std::move(iq), - std::move(messages) }); + std::move(processedMessages) }); } }; @@ -144,7 +165,7 @@ bool QXmppMamManager::handleStanza(const QDomElement &element) itr->second.messages.append(std::move(message)); } else { // signal-based API - Q_EMIT archivedMessageReceived(queryId, message); + Q_EMIT archivedMessageReceived(queryId, parseMamMessage(message, Unencrypted)); } return true; } @@ -304,11 +325,13 @@ QXmppTask QXmppMamManager::retrieveMessages(con // decrypt encrypted messages if (auto *e2eeExt = client()->encryptionExtension()) { - auto &messages = state.messages; + // initialize processed messages (we need random access because + // decryptMessage() may finish in random order) + state.processedMessages.resize(state.messages.size()); // check for encrypted messages (once) auto messagesEncrypted = transform(state.messages, [&](const auto &m) { - return e2eeExt->isEncrypted(m); + return e2eeExt->isEncrypted(m.element); }); auto encryptedCount = sum(messagesEncrypted); @@ -316,25 +339,26 @@ QXmppTask QXmppMamManager::retrieveMessages(con // because some decryptMessage() jobs could finish instantly state.runningDecryptionJobs = encryptedCount; - for (auto i = 0; i < messages.size(); i++) { + for (auto i = 0; i < state.messages.size(); i++) { if (!messagesEncrypted[i]) { continue; } - auto message = messages.at(i); - state.runningDecryptionJobs++; - e2eeExt->decryptMessage(std::move(message)).then(this, [this, i, queryId](auto result) { + e2eeExt->decryptMessage(parseMamMessage(state.messages.at(i), Encrypted)).then(this, [this, i, queryId](auto result) { auto itr = d->ongoingRequests.find(queryId.toStdString()); Q_ASSERT(itr != d->ongoingRequests.end()); auto &state = itr->second; + // store decrypted message, fallback to encrypted message if (std::holds_alternative(result)) { - state.messages[i] = std::get(std::move(result)); + state.processedMessages[i] = std::get(std::move(result)); } else { warning(QStringLiteral("Error decrypting message.")); + state.processedMessages[i] = parseMamMessage(state.messages[i], Unencrypted); } + // finish promise if this was the last job state.runningDecryptionJobs--; if (state.runningDecryptionJobs == 0) { state.finish(); @@ -349,6 +373,10 @@ QXmppTask QXmppMamManager::retrieveMessages(con } } + // for the case without decryption, finish here + state.processedMessages = transform(state.messages, [](const auto &m) { + return parseMamMessage(m, Unencrypted); + }); state.finish(); d->ongoingRequests.erase(itr); }); -- cgit v1.2.3