Merge pull request #10680 from chrip/fix/7009-installer-cleanup-on-migration

fix(updater): remove leftover installer during config version migration

Author: mgallien

src/gui/updater/ocupdater.cpp

@@ -26,7 +26,6 @@ const auto updateAvailableC = QStringLiteral("Updater/updateAvailable");
 const auto updateTargetVersionC = QStringLiteral("Updater/updateTargetVersion");
 const auto updateTargetVersionStringC = QStringLiteral("Updater/updateTargetVersionString");
 const auto autoUpdateAttemptedC = QStringLiteral("Updater/autoUpdateAttempted");
-const auto msiLogFileNameC = QStringLiteral("msi.log");
 }
 
 UpdaterScheduler::UpdaterScheduler(QObject *parent)
@@ -223,7 +222,7 @@ void OCUpdater::slotStartInstaller()
             return QDir::toNativeSeparators(path);
         };
 
-        QString msiLogFile = cfg.configPath() + msiLogFileNameC;
+        const auto msiLogFile = cfg.msiLogFilePath();
         QString command = QStringLiteral("&{msiexec /i '%1' /L*V '%2'| Out-Null ; &'%3'}")
              .arg(preparePathForPowershell(updateFile))
              .arg(preparePathForPowershell(msiLogFile))
@@ -305,30 +304,11 @@ void NSISUpdater::slotWriteFile()
 
 void NSISUpdater::wipeUpdateData()
 {
-    ConfigFile cfg;
-    QSettings settings(cfg.configFile(), QSettings::IniFormat);
-    QString updateFileName = settings.value(updateAvailableC).toString();
-    if (!updateFileName.isEmpty()) {
-        if (QFile::remove(updateFileName)) {
-            qCInfo(lcUpdater) << "Removed updater file:" << updateFileName;
-        } else {
-            qCWarning(lcUpdater) << "Failed to remove updater file:" << updateFileName;
-        }
-    }
-    // Also try to remove the msi log file (created when running msiexec)
-    const auto msiLogFileName = QString{cfg.configPath() + msiLogFileNameC};
-    if (QFile::exists(msiLogFileName)) {
-        if (QFile::remove(msiLogFileName)) {
-            qCInfo(lcUpdater) << "Removed msi log file:" << msiLogFileName;
-        } else {
-            qCWarning(lcUpdater) << "Failed to remove msi log file:" << msiLogFileName;
-        }
-    }
-
-    settings.remove(updateAvailableC);
-    settings.remove(updateTargetVersionC);
-    settings.remove(updateTargetVersionStringC);
-    settings.remove(autoUpdateAttemptedC);
+    // Deliberately delegated: ConfigFile::cleanUpdaterConfiguration() is also
+    // reached from Application::configVersionMigration() on the first start of
+    // a newly installed version, and both paths must remove the installer, not
+    // just the keys that point at it.
+    ConfigFile().cleanUpdaterConfiguration();
 }
 
 void NSISUpdater::slotDownloadFinished()

src/libsync/configfile.cpp

@@ -434,10 +434,38 @@ QString ConfigFile::excludeFileFromSystem()
     return fi.absoluteFilePath();
 }
 
+namespace {
+void removeUpdaterArtifact(const QString &path)
+{
+    if (path.isEmpty() || !QFile::exists(path)) {
+        return;
+    }
+
+    if (QFile::remove(path)) {
+        qCInfo(lcConfigFile) << "Removed leftover updater file:" << path;
+    } else {
+        qCWarning(lcConfigFile) << "Failed to remove leftover updater file:" << path;
+    }
+}
+}
+
+QString ConfigFile::msiLogFilePath() const
+{
+    return configPath() + QStringLiteral("msi.log");
+}
+
 void OCC::ConfigFile::cleanUpdaterConfiguration()
 {
     QSettings settings(configFile(), QSettings::IniFormat);
     settings.beginGroup("Updater");
+
+    // The config is the only record of where the downloaded installer lives.
+    // Delete it before dropping the keys, or every version change orphans
+    // another installer in the config folder with nothing left to find it by.
+    // See https://github.com/nextcloud/desktop/issues/7009
+    removeUpdaterArtifact(settings.value("updateAvailable").toString());
+    removeUpdaterArtifact(msiLogFilePath());
+
     settings.remove("autoUpdateAttempted");
     settings.remove("updateTargetVersion");
     settings.remove("updateTargetVersionString");

src/libsync/configfile.h

@@ -42,6 +42,8 @@ class OWNCLOUDSYNC_EXPORT ConfigFile
     static QString excludeFileFromSystem(); // doesn't access config dir
 
     void cleanUpdaterConfiguration();
+    /** Path of the log msiexec writes while installing an update (Windows). */
+    [[nodiscard]] QString msiLogFilePath() const;
     void cleanupGlobalNetworkConfiguration();
 
     /**

test/testupdater.cpp

@@ -51,6 +51,53 @@ private Q_SLOTS:
         QVERIFY(currVersion < highVersion);
     }
 
+    // Reproduces #7009: on the first start of a newly installed version the
+    // leftover installer must be gone. Application::configVersionMigration()
+    // (src/gui/application.cpp:161-164) drops the Updater/* keys before
+    // main() calls handleStartup() (src/gui/main.cpp:104 vs 142), so the
+    // cleanup in NSISUpdater::handleStartup() never sees the artifact.
+    void testLeftoverInstallerIsRemovedAfterVersionChange()
+    {
+        QTemporaryDir tempDir;
+        QVERIFY(tempDir.isValid());
+        QVERIFY(ConfigFile::setConfDir(tempDir.path()));
+
+        ConfigFile cfg;
+        const auto writeFile = [](const QString &path, const QByteArray &contents) {
+            QFile f(path);
+            return f.open(QIODevice::WriteOnly) && f.write(contents) == contents.size();
+        };
+
+        // a previous run downloaded an installer and msiexec left its log behind
+        const auto installer = FileSystem::joinPath(cfg.configPath(), "Nextcloud-1.0.0-x64.msi"_L1);
+        const auto msiLog = FileSystem::joinPath(cfg.configPath(), "msi.log"_L1);
+        QVERIFY(writeFile(installer, "this would be the installer"_ba));
+        QVERIFY(writeFile(msiLog, "this would be the msiexec log"_ba));
+
+        {
+            QSettings settings(cfg.configFile(), QSettings::IniFormat);
+            settings.setValue("Updater/updateAvailable"_L1, installer);
+            settings.setValue("Updater/updateTargetVersion"_L1, "1.0.0"_L1);
+            settings.setValue("Updater/updateTargetVersionString"_L1, "1.0.0"_L1);
+            settings.setValue("Updater/autoUpdateAttempted"_L1, true);
+            settings.sync();
+        }
+
+        // the update was installed: the running binary is newer than the target
+        QVERIFY(Updater::Helper::currentVersionToInt() > Updater::Helper::stringVersionToInt("1.0.0"_L1));
+
+        // what Application::configVersionMigration() does on that very first
+        // start, before the updater gets a chance to look at its own state
+        cfg.setClientVersionString("1.0.0"_L1);
+        cfg.cleanUpdaterConfiguration();
+
+        NSISUpdater updater(QUrl("http://localhost:1/updateinfo.xml"_L1));
+        updater.handleStartup();
+
+        QVERIFY(!QFileInfo::exists(installer));
+        QVERIFY(!QFileInfo::exists(msiLog));
+    }
+
 #ifdef HAVE_QHTTPSERVER
     void testUpdaterDownloadRedirect()
     {