From 9edf0056aed137466a2de7a1ebfaf6bd7b4d95db Mon Sep 17 00:00:00 2001
From: Nikolai Kosjar <nikolai.kosjar@qt.io>
Date: Thu, 14 May 2020 09:28:47 +0200
Subject: [PATCH] ClangTools: Fix crash when starting the analyzer again

Address a nullptr dereference of m_runWorker in
ClangTool::updateForCurrentState() for the case described as (C2) below,
reproducible with "./qtcreator -test ClangTools".

Two use cases are connected to this:

 (C1) Run the analyzer twice with clearing app output pane in-between
 (C2) Run the analyzer twice without clearing app output pane in-between

Relevant observations in this context are:

 (O1) Closing the app output pane destroys the RunControl/RunWorker.
 (O2) Running the analyzer a second time will first create a new
 RunControl/RunWorker, then destroy the old one.

Now, the change

    ClangTools: Avoid accessing deleted run worker
    This reverts commit d02f5664e53df41ff7156eb46069e339479074d2.

fixed a use-after-free-crash for (C1), but introduced a
nullptr-deref-crash for (C2) as it resets m_runWorker to nullptr on
RunControl destruction, which conflicts with the order mentioned in
(O2).

To fix both use cases, revert the mentioned change and access
m_runWorker only when we know that it exists for sure - right after
signal emission.

Change-Id: I034f0905d635b15c0c6bbe499648b62d5a058c04
Reviewed-by: Christian Stenger <christian.stenger@qt.io>
---
 src/plugins/clangtools/clangtool.cpp | 27 ++++++++++++++++-----------
 src/plugins/clangtools/clangtool.h   |  3 +++
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/src/plugins/clangtools/clangtool.cpp b/src/plugins/clangtools/clangtool.cpp
index e08b73bae60..0928e40e6bd 100644
--- a/src/plugins/clangtools/clangtool.cpp
+++ b/src/plugins/clangtools/clangtool.cpp
@@ -696,9 +696,12 @@ void ClangTool::startTool(ClangTool::FileSelection fileSelection,
     connect(m_runWorker, &ClangToolRunWorker::buildFailed,this, &ClangTool::onBuildFailed);
     connect(m_runWorker, &ClangToolRunWorker::startFailed, this, &ClangTool::onStartFailed);
     connect(m_runWorker, &ClangToolRunWorker::started, this, &ClangTool::onStarted);
-    connect(m_runWorker, &ClangToolRunWorker::runnerFinished,
-            this, &ClangTool::updateForCurrentState);
-    connect(m_runControl, &RunControl::destroyed, [this](){ m_runWorker = nullptr; });
+    connect(m_runWorker, &ClangToolRunWorker::runnerFinished, this, [this]() {
+        m_filesCount = m_runWorker->totalFilesToAnalyze();
+        m_filesSucceeded = m_runWorker->filesAnalyzed();
+        m_filesFailed = m_runWorker->filesNotAnalyzed();
+        updateForCurrentState();
+    });
 
     // More init and UI update
     m_diagnosticFilterModel->setProject(project);
@@ -857,6 +860,10 @@ void ClangTool::reset()
     m_state = State::Initial;
     m_runControl = nullptr;
     m_runWorker = nullptr;
+
+    m_filesCount = 0;
+    m_filesSucceeded = 0;
+    m_filesFailed = 0;
 }
 
 static bool canAnalyzeProject(Project *project)
@@ -1039,8 +1046,6 @@ void ClangTool::onRunControlStopped()
 void ClangTool::update()
 {
     updateForInitialState();
-    if (!m_runWorker)
-        return;
     updateForCurrentState();
 }
 
@@ -1162,9 +1167,9 @@ void ClangTool::updateForCurrentState()
 
     // Info bar: errors
     const bool hasErrorText = !m_infoBarWidget->errorText().isEmpty();
-    const bool hasErrors = m_runWorker && m_runWorker->filesNotAnalyzed() > 0;
+    const bool hasErrors = m_filesFailed > 0;
     if (hasErrors && !hasErrorText) {
-        const QString text = makeLink( tr("Failed to analyze %1 files.").arg(m_runWorker->filesNotAnalyzed()));
+        const QString text = makeLink( tr("Failed to analyze %1 files.").arg(m_filesFailed));
         m_infoBarWidget->setError(InfoBarWidget::Warning, text, [this]() { showOutputPane(); });
     }
 
@@ -1177,12 +1182,12 @@ void ClangTool::updateForCurrentState()
         break;
     case State::AnalyzerRunning:
         showProgressIcon = true;
-        if (m_runWorker->totalFilesToAnalyze() == 0) {
+        if (m_filesCount == 0) {
             infoText = tr("Analyzing..."); // Not yet fully started/initialized
         } else {
             infoText = tr("Analyzing... %1 of %2 files processed.")
-                           .arg(m_runWorker->filesAnalyzed() + m_runWorker->filesNotAnalyzed())
-                           .arg(m_runWorker->totalFilesToAnalyze());
+                           .arg(m_filesSucceeded + m_filesFailed)
+                           .arg(m_filesCount);
         }
         break;
     case State::PreparationStarted:
@@ -1195,7 +1200,7 @@ void ClangTool::updateForCurrentState()
         infoText = tr("Analysis stopped by user.");
         break;
     case State::AnalyzerFinished:
-        infoText = tr("Finished processing %1 files.").arg(m_runWorker->totalFilesToAnalyze());
+        infoText = tr("Finished processing %1 files.").arg(m_filesCount);
         break;
     case State::ImportFinished:
         infoText = tr("Diagnostics imported.");
diff --git a/src/plugins/clangtools/clangtool.h b/src/plugins/clangtools/clangtool.h
index 3ee72da83c7..a32bb50c2c1 100644
--- a/src/plugins/clangtools/clangtool.h
+++ b/src/plugins/clangtools/clangtool.h
@@ -162,6 +162,9 @@ private:
     QAction *m_stopAction = nullptr;
 
     State m_state = State::Initial;
+    int m_filesCount = 0;
+    int m_filesSucceeded = 0;
+    int m_filesFailed = 0;
 
     DiagnosticFilterModel *m_diagnosticFilterModel = nullptr;