fix(ui): harden docset directory cleanup on install (#1938)

src/libs/ui/docsetsdialog.cpp

@@ -20,6 +20,7 @@
 #include <QClipboard>
 #include <QDateTime>
 #include <QDir>
+#include <QFileInfo>
 #include <QInputDialog>
 #include <QJsonArray>
 #include <QJsonDocument>
@@ -77,6 +78,17 @@ DownloadType downloadType(const QNetworkReply *reply)
 {
     return static_cast<DownloadType>(reply->property(DownloadTypeProperty).toInt());
 }
+
+// An empty name, or one with path separators, could escape the cache and storage directories.
+bool isDocsetNameSafe(const QString &docsetName)
+{
+    if (docsetName.isEmpty() || docsetName.contains(QLatin1Char('/')) || docsetName.contains(QLatin1Char('\\'))) {
+        qCWarning(log, "Refusing docset with an unsafe name '%s'.", qPrintable(docsetName));
+        return false;
+    }
+
+    return true;
+}
 } // namespace
 
 DocsetsDialog::DocsetsDialog(Core::Application *app, QWidget *parent)
@@ -264,9 +276,7 @@ void DocsetsDialog::downloadSelectedDocsets()
 
 QTemporaryFile *DocsetsDialog::docsetTemporaryFile(const QString &docsetName)
 {
-    // A name with path separators could escape the cache and storage directories.
-    if (docsetName.contains(QLatin1Char('/')) || docsetName.contains(QLatin1Char('\\'))) {
-        qCWarning(log, "Refusing docset with an unsafe name '%s'.", qPrintable(docsetName));
+    if (!isDocsetNameSafe(docsetName)) {
         return nullptr;
     }
 
@@ -382,10 +392,43 @@ void DocsetsDialog::downloadCompleted()
 
     case DownloadType::Docset: {
         const QString docsetName = reply->property(DocsetNameProperty).toString();
+        if (!isDocsetNameSafe(docsetName)) {
+            QListWidgetItem *listItem = findDocsetListItem(docsetName);
+            if (listItem != nullptr) {
+                listItem->setData(DocsetListItemDelegate::ShowProgressRole, false);
+            }
+            break;
+        }
+
         const QString docsetDirectoryName = docsetName + QLatin1String(".docset");
 
-        if (QDir(m_application->settings()->docsetPath).exists(docsetDirectoryName)) {
-            removeDocset(docsetName);
+        // A directory owned by no loaded docset is stale; refuse to merge into it.
+        const QDir dataDir(m_application->settings()->docsetPath);
+        if (dataDir.exists(docsetDirectoryName)) {
+            const QString docsetPath = dataDir.filePath(docsetDirectoryName);
+            const QString installedName = docsetNameForPath(docsetPath);
+            bool removed = false;
+            if (!installedName.isEmpty()) {
+                // removeDocset() reports its own failure.
+                removed = removeDocset(installedName);
+            } else {
+                removed = Core::FileManager::removeRecursively(docsetPath);
+                if (!removed) {
+                    QMessageBox::warning(this,
+                                         QStringLiteral("Zeal"),
+                                         tr("Cannot remove directory <b>%1</b>! It might be in use"
+                                            " by another process.")
+                                             .arg(docsetPath.toHtmlEscaped()));
+                }
+            }
+
+            if (!removed) {
+                QListWidgetItem *listItem = findDocsetListItem(docsetName);
+                if (listItem != nullptr) {
+                    listItem->setData(DocsetListItemDelegate::ShowProgressRole, false);
+                }
+                break;
+            }
         }
 
         QTemporaryFile *tmpFile = docsetTemporaryFile(docsetName);
@@ -1083,26 +1126,31 @@ void DocsetsDialog::installDownloadedDocset(const QString &docsetName)
                                         docsetName + QLatin1String(".docset"));
 }
 
-void DocsetsDialog::removeDocset(const QString &name)
+bool DocsetsDialog::removeDocset(const QString &name)
 {
     if (!m_docsetRegistry->contains(name)) {
-        return;
+        return true;
     }
 
     const QString docsetPath = m_docsetRegistry->docset(name)->path();
     m_docsetRegistry->unloadDocset(name);
     if (!Core::FileManager::removeRecursively(docsetPath)) {
+        // The directory survived, so keep the docset usable for the rest of the session.
+        m_docsetRegistry->loadDocset(docsetPath);
+
         const QString error = tr("Cannot remove directory <b>%1</b>! It might be in use"
                                  " by another process.")
                                   .arg(docsetPath.toHtmlEscaped());
         QMessageBox::warning(this, QStringLiteral("Zeal"), error);
-        return;
+        return false;
     }
 
     QListWidgetItem *listItem = findDocsetListItem(name);
     if (listItem != nullptr) {
         listItem->setHidden(false);
     }
+
+    return true;
 }
 
 void DocsetsDialog::updateStatus()
@@ -1123,6 +1171,25 @@ void DocsetsDialog::updateStatus()
     enableControls();
 }
 
+// Matched by canonical path, because the registry is keyed on the docset's metadata
+// name, which can differ from the directory name.
+QString DocsetsDialog::docsetNameForPath(const QString &path) const
+{
+    const QString canonicalPath = QFileInfo(path).canonicalFilePath();
+    if (canonicalPath.isEmpty()) {
+        return {};
+    }
+
+    const QList<Registry::Docset *> docsets = m_docsetRegistry->docsets();
+    for (const Registry::Docset *docset : docsets) {
+        if (QFileInfo(docset->path()).canonicalFilePath() == canonicalPath) {
+            return docset->name();
+        }
+    }
+
+    return {};
+}
+
 QString DocsetsDialog::docsetNameForTmpFilePath(const QString &filePath) const
 {
     for (auto it = m_tmpFiles.cbegin(), end = m_tmpFiles.cend(); it != end; ++it) {

src/libs/ui/docsetsdialog.h

@@ -103,7 +103,8 @@ private:
     void downloadTarixIndex(const QString &docsetName, const QUrl &indexUrl, int attempt);
     void onTarixIndexFailed(QNetworkReply *reply);
     void installDownloadedDocset(const QString &docsetName);
-    void removeDocset(const QString &name);
+    // Returns false if the docset directory could not be removed.
+    bool removeDocset(const QString &name);
 
     void updateStatus();
 
@@ -111,6 +112,8 @@ private:
     // Returns nullptr if the temporary file cannot be opened.
     QTemporaryFile *docsetTemporaryFile(const QString &docsetName);
 
+    QString docsetNameForPath(const QString &path) const;
+
     // FIXME: Come up with a better approach
     QString docsetNameForTmpFilePath(const QString &filePath) const;