mirror of
https://invent.kde.org/network/kdeconnect-kde.git
synced 2026-10-17 01:49:08 +02:00
Thomas Whitecotton
0dc666ad95
handleRequestConversation() logs a warning when the requested conversation has zero messages, but then falls through into replyForConversation(), which does:
```
for (auto it = conversation.crbegin() + start; it < conversation.crend(); ++it) {
...
Q_EMIT conversationMessageRead(QDBusVariant(QVariant::fromValue(*it)));
}
```
On an empty QList, `crbegin() + start` for any non-zero `start` is undefined behaviour: the iterator is past-the-end, the loop body runs, `*it` dereferences garbage memory, and QVariant::fromValue copies through the bad pointer. In practice this segfaults inside QVariant::fromMetaType while constructing a ConversationMessage copy.
When the empty conversation comes from the phone repeatedly (a deleted SMS thread the phone hasn't dropped from its index, a junk ID, or a thread that has not been synced yet), kdeconnectd's D-Bus auto-activation restarts the daemon after each crash, it re-fetches the same bad conversation, and re-crashes. The result is a roughly 10-second crash loop that takes down kdeconnectd (not just the SMS plugin) until the phone's state changes.
Hit on kdeconnect 26.04.1 against a Galaxy Z Fold7: 20 SIGSEGVs in 3 minutes from a single conversationID. Matches the unresolved Fedora ABRT report #1026772 against 24.08.0 (https://retrace.fedoraproject.org/faf/reports/1026772/), so this has been around for at least a couple of years across multiple KDE Gear releases.
The existing comment ("it's likely that it is a junk ID, but go ahead anyway") shows the empty-list case was already known. There just isn't anything useful to do with it, so return after the warning instead of continuing into the broken loop. Q_EMIT finished() is needed so the worker's thread quits and the object self-destructs per the constructor's signal wiring. BUG: 519587
95 lines
3.7 KiB
C++
95 lines
3.7 KiB
C++
/**
|
|
* SPDX-FileCopyrightText: 2018 Simon Redman <simon@ergotech.com>
|
|
*
|
|
* SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
|
|
*/
|
|
|
|
#include "requestconversationworker.h"
|
|
|
|
#include "conversationsdbusinterface.h"
|
|
|
|
#include <QObject>
|
|
|
|
RequestConversationWorker::RequestConversationWorker(const qint64 &conversationID, int start, int end, ConversationsDbusInterface *interface)
|
|
: // QObject(interface)
|
|
conversationID(conversationID)
|
|
, start(start)
|
|
, parent(interface)
|
|
, m_thread(new QThread)
|
|
{
|
|
Q_ASSERT(end >= start && "Not allowed to have a negative-length range");
|
|
howMany = end - start;
|
|
|
|
this->moveToThread(m_thread);
|
|
connect(m_thread, &QThread::started, this, &RequestConversationWorker::handleRequestConversation);
|
|
connect(m_thread, &QThread::finished, m_thread, &QObject::deleteLater);
|
|
connect(this, &RequestConversationWorker::finished, m_thread, &QThread::quit);
|
|
connect(this, &RequestConversationWorker::finished, this, &QObject::deleteLater);
|
|
}
|
|
|
|
void RequestConversationWorker::handleRequestConversation()
|
|
{
|
|
auto messagesList = parent->getConversation(conversationID);
|
|
|
|
if (messagesList.isEmpty()) {
|
|
// The phone advertised a conversation with no messages (likely a junk ID,
|
|
// a thread the user just deleted, or a not-yet-synced thread). Bail out
|
|
// here: falling through into replyForConversation() does
|
|
// `conversation.crbegin() + start` on an empty QList, which is UB and
|
|
// segfaults inside QVariant::fromValue when *it is dereferenced.
|
|
qCWarning(KDECONNECT_CONVERSATIONS) << "Got a conversationID for a conversation with no messages!" << conversationID;
|
|
Q_EMIT finished();
|
|
return;
|
|
}
|
|
|
|
// In case the remote takes awhile to respond, we should go ahead and do anything we can from the cache
|
|
size_t numHandled = replyForConversation(messagesList, start, howMany);
|
|
|
|
if (numHandled < howMany) {
|
|
// In this case, the cache wasn't able to satisfy the request fully. Get more.
|
|
|
|
size_t numRemaining = howMany - numHandled;
|
|
parent->updateConversation(conversationID);
|
|
messagesList = parent->getConversation(conversationID);
|
|
// ConversationsDbusInterface::updateConversation blocks until it sees new messages in the requested conversation
|
|
replyForConversation(messagesList, start + numHandled, numRemaining);
|
|
} else {
|
|
// The cache was able to fully satisfy the request but we need to check that it isn't running dry
|
|
|
|
size_t numCachedMessages = messagesList.count();
|
|
size_t requestEnd = start + numHandled;
|
|
size_t numRemainingMessages = numCachedMessages - requestEnd;
|
|
double percentRemaining = ((double)numRemainingMessages / numCachedMessages) * 100;
|
|
|
|
if (percentRemaining < CACHE_LOW_WATER_MARK_PERCENT || numRemainingMessages < MIN_NUMBER_TO_REQUEST) {
|
|
parent->updateConversation(conversationID);
|
|
}
|
|
}
|
|
|
|
Q_EMIT finished();
|
|
}
|
|
|
|
size_t RequestConversationWorker::replyForConversation(const QList<ConversationMessage> &conversation, size_t start, size_t howMany)
|
|
{
|
|
// Messages are sorted in ascending order of keys, meaning the front of the list has the oldest
|
|
// messages (smallest timestamp number)
|
|
// Therefore, return the end of the list first (most recent messages)
|
|
size_t i = 0;
|
|
for (auto it = conversation.crbegin() + start; it < conversation.crend(); ++it) {
|
|
if (i >= howMany) {
|
|
break;
|
|
}
|
|
Q_EMIT conversationMessageRead(QDBusVariant(QVariant::fromValue(*it)));
|
|
i++;
|
|
}
|
|
|
|
return i;
|
|
}
|
|
|
|
void RequestConversationWorker::work()
|
|
{
|
|
m_thread->start();
|
|
}
|
|
|
|
#include "moc_requestconversationworker.cpp"
|