Commit 38713121 authored by Igor Kushnir's avatar Igor Kushnir Committed by Milian Wolff
Browse files

DocumentParsePlan: don't cache often-invalidated cend()

The loop in DocumentParsePlan::removeTargetsForListener() caches
m_targets.cend(), which can become invalidated when a target is erased
inside this loop. Comparing to the invalidated end iterator leads to
undefined behavior.

This undefined behavior makes QmlJS's test_files crash every other time,
both on my system and on the CI. The alternating crashes of this test
can be seen by clicking Next Build repeatedly starting from the first
build since the bug was introduced:
https://build.kde.org/job/KDevelop/job/kdevelop/job/kf5-qt5%20SUSEQt5.15/165/testReport/junit/projectroot.plugins.qmljs/tests/test_files/

KDevelop often crashes because of this undefined behavior too (see the
bug report referenced below).

m_targets is a QSet, which is implemented in terms of QHash. QHash's end
iterator `e` equals the d-pointer `d` via anonymous union. So the only
way cend() can be invalidated is if QSet::erase() detaches. That's
possible, because an entire DocumentParsePlan is copied in
BackgroundParserPrivate::parseDocumentsInternal():
    const DocumentParsePlan parsePlan = *parsePlanConstIt;
The involvement of multithreading explains why test_files crashes every
other time rather than each time. Examining call stacks of both
test_files and kdevelop crashes confirms this hypothesis: a background
thread crashes in DocumentParsePlan::removeTargetsForListener() while
the main thread waits on the `m_mutex.lock()` line in
parseDocumentsInternal().

This bug was introduced in 5ee9b9fe.

BUG: 445699
parent 78e40953
Pipeline #101061 passed with stage
in 32 minutes and 15 seconds
......@@ -164,7 +164,7 @@ public:
void removeTargetsForListener(QObject* notifyWhenReady)
{
m_priority = BackgroundParser::WorstPriority;
for (auto it = m_targets.cbegin(), end = m_targets.cend(); it != end;) {
for (auto it = m_targets.cbegin(); it != m_targets.cend();) {
if (it->notifyWhenReady.data() == notifyWhenReady) {
it = m_targets.erase(it);
} else {
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment