[Installer] Check for running instances with the Restart Manager probe (#7671)

* [Installer] Check for running instances with the Restart Manager probe

The running-instance guard was built on powershell.exe started from the
32-bit NSIS stub, which is the 32-bit Windows PowerShell: its Get-Process
cannot read the path of a 64-bit process, so a running 64-bit Cockatrice
was reported as not running, the update went ahead, and the purge then
failed on the DLLs that instance still had mapped.

Use tools/installer-holder-probe instead - the same Restart Manager lookup
the abort path already had - before anything is replaced and in the
uninstaller as well. It is bitness-agnostic, so the Sysnative and WMI
fallbacks are gone.

The guard looks only at the payload, asks Cockatrice to close once by
itself (WM_CLOSE, the same request as the window's close button) and then
offers close again / retry after closing by hand / cancel for whatever the
close cannot reach - Oracle writes the card database in place, Servatrice
has no window - without force-stopping anything. A failed purge offers the
same close-and-retry, tries one last purge when nobody is named, and every
dialog shows in every mode: a silent reinstall stays silent until
something goes wrong.

* [Installer] Name processes running from the install directory

The running-instance guard answers from the Restart Manager, which reports
who has a file open. A running instance does not have to have one open: its
own image and the modules it loaded are mapped into it rather than opened by
it, and a mapping is not reliably reported. That is the state the probe was
added for - every DLL still on disk, an instance running from the directory,
and "nothing is holding them any more" as the answer.

A guard built on file handles alone is blind to exactly that instance, so it
passes, the purge fails, the holder lookup after it names nobody either, and
the install ends in a warning that no running program was found: no close is
attempted and no retry is offered. A manual install with another instance
open never asks it to close at all.

Name every process whose image lives in the directory being replaced as
well, with the same bitness-safe QueryFullProcessImageNameW the probe
already uses to label its holders. The image lookup is listed first - it is
the instance the user is asked to close - and the Restart Manager entries
follow with the pids already named dropped, so the close still reaches
everything that holds a file. A Restart Manager call that fails only makes
the probe unable to answer when the image lookup named nobody either.

Ask instead of dead-ending when the probe cannot answer at all: Retry checks
again, Ignore carries on, Abort cancels, in the installer and the uninstaller
alike, and the abort dialog says the check failed instead of claiming no
running program was found.

The Windows CI probe check covers the new lookup with a process started from
the directory that holds no file open.

* [Installer] Offer to wait again when the launcher has not exited

A client that is still closing after 60 s is usually not hung: it is showing
its own "quit anyway?" confirmation for an unsaved deck, an open game or a
card database update, and the user is the one who has to answer it. The
installer answered that with a plain warning and stopped, so the only way
forward was to close Cockatrice by hand and run the update again - which on
an in-app update means finding the downloaded installer.

Ask instead: Retry brings the client to the front, asks it to close again
(WM_CLOSE, nothing forced) and keeps waiting on its handle, Cancel stops as
before. There is no "install anyway" answer because a live process has its
image and Qt DLLs mapped, so writing the payload over them is the broken
install this wait exists to prevent - and the guard that runs right after
names the same process by its image path and stops there, so ignoring would
only move the same question a moment later.

* [Installer] Ask about a launcher that has not exited after ten seconds

A minute of silence is a long time to sit in front of an installer that has no
window yet, and the case it is waiting for is not a slow flush: a client that
started an update exits in well under a second, so a minute means it is almost
always showing its own "quit anyway?" confirmation with nobody looking at it.

Wait ten seconds for the first question instead, and the full grace for the
ones after it. The normal path is untouched - WaitForSingleObject returns as
soon as the handle signals, so a client that exits in 200 ms still costs 200
ms - and answering the first question once does not turn into a stream of
prompts while the user is in Cockatrice's window.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

.ci/compile.sh

@@ -515,7 +515,27 @@ try {
     New-Item -ItemType Directory -Force $empty | Out-Null
     $nothing = Invoke-Probe $empty
     if ($nothing.Code -ne 0) { throw "expected exit 0 for a directory with nothing open, got $($nothing.Code) : $($nothing.Output)" }
-    Write-Host "holder lookup named pid $PID for a locked file and named nothing for an empty directory"
+    # A process running from the directory is a holder even when it has no file open: an
+    # instance holds its payload as mapped images, not as handles, and that is the state
+    # the guard used to miss - it passed, the purge failed, and the install ended in a
+    # warning with no offer to close and try again. The image is a copy of a native
+    # Windows program, so it starts without its usual directory next to it.
+    $copy = Join-Path $dir "fake-cockatrice.exe"
+    Copy-Item -LiteralPath (Join-Path $env:SystemRoot "System32\cmd.exe") -Destination $copy
+    $running = Start-Process -FilePath $copy -ArgumentList "/c", "ping -n 20 127.0.0.1 >nul" -PassThru
+    try {
+        Start-Sleep -Milliseconds 1500
+        $byImage = Invoke-Probe $dir
+    } finally {
+        try { $running.Kill() } catch { }
+        # Wait for the image to be released, or the cleanup below leaves the copy behind.
+        try { $running.WaitForExit(5000) | Out-Null } catch { }
+    }
+    if ($byImage.Code -ne 1) { throw "expected exit 1 for a process running from the directory, got $($byImage.Code) : $($byImage.Output)" }
+    if ($byImage.Output -notmatch "fake-cockatrice\.exe \(pid\s+$($running.Id)") {
+        throw "the lookup did not name the process running from the directory: $($byImage.Output)"
+    }
+    Write-Host "holder lookup named pid $PID for a locked file, named nothing for an empty directory, and named pid $($running.Id) for a process running from the directory"
 } finally {
     Remove-Item -Recurse -Force $dir -ErrorAction SilentlyContinue
 }

tools/installer-holder-probe/CMakeLists.txt

@@ -1,9 +1,10 @@
-# Windows-only. The installer runs this on the abort path of a failed purge to name the
-# applications and services holding the install directory open (Restart Manager); see
-# the LogProcessesUsingInstallDir function in cmake/NSIS.template.in. It ships inside
-# the installer and is extracted to $PLUGINSDIR there, never installed, so it links
-# nothing from this project: no Qt, and a static C runtime because the installed runtime
-# is exactly what the purge may have failed to replace.
+# Windows-only. The installer runs this before anything is replaced (the running-instance
+# guard in .onInit) and on the abort path of a failed purge, to name the applications and
+# services holding the payload open (Restart Manager); see the GuardAppsNotRunning and
+# LogProcessesUsingInstallDir functions in cmake/NSIS.template.in. It ships inside the
+# installer and the uninstaller and is extracted to $PLUGINSDIR there, never installed, so
+# it links nothing from this project: no Qt, and a static C runtime because the installed
+# runtime is exactly what the purge may have failed to replace.
 add_executable(installer-holder-probe close_holders.cpp holder_line.cpp main.cpp restart_manager_holders.cpp)
 
 target_link_libraries(installer-holder-probe PRIVATE rstrtmgr)

tools/installer-holder-probe/main.cpp

@@ -2,6 +2,7 @@
 #include "holder_line.h"
 #include "restart_manager_holders.h"
 
+#include <algorithm>
 #include <string>
 #include <vector>
 #include <windows.h>
@@ -22,7 +23,9 @@ constexpr unsigned long CLOSE_WAIT_MILLISECONDS = 30000;
 /// Exit codes the installer reads. "Something was found" is deliberately not 0, so an
 /// answer of "nothing holds the files" stays distinguishable from a probe that failed.
 /// In the close mode 1 means a close was actually sent and 0 that every holder was left
-/// to the user, which is what the report line next to the code spells out.
+/// to the user, which is what the report line next to the code spells out. 3 is only for
+/// a lookup that failed and named nobody: one that failed but still named a process
+/// running from the directory is answered with that process.
 enum ExitCode
 {
     EXIT_NO_HOLDERS = 0,
@@ -33,13 +36,17 @@ enum ExitCode
 
 /// The probe has two modes, selected by the first argument:
 ///
-///   (none)           name what holds the install directory, one line, for the abort
-///                     dialog and the log
+///   (none)           name what holds the payload, one line, for the guard prompt and the
+///                     abort dialog/log
 ///   --close-holders  bring the running Cockatrice forward, ask it to close, and wait for
 ///                     it to exit, for the offer to close after a failed purge
 ///
-/// Both answer from the same Restart Manager query, so the close can only ever reach a
-/// process that is holding a file of the version being replaced.
+/// Both answer from two lookups that cover each other's blind spot: the Restart Manager
+/// names whoever has a payload file open, and every process running from the directory is
+/// named as well, because a running instance holds its own image and the modules it
+/// loaded as mappings rather than open files and the Restart Manager does not always
+/// report those. Neither lookup alone is the guard's authority: an instance the file
+/// handles do not name is exactly the case that left a half-replaced install behind.
 bool closeModeRequested(int argumentCount, char **arguments)
 {
     return argumentCount > 1 && std::string(arguments[1]) == "--close-holders";
@@ -85,9 +92,26 @@ int main(int argumentCount, char **arguments)
         return EXIT_NOT_CONFIGURED;
     }
 
-    std::vector<Holder> holders;
-    const unsigned long error = findProcessesUsingFiles(collectProbeFiles(installDir), holders);
-    if (error != ERROR_SUCCESS) {
+    // Processes running from the directory first: they are the instance the user is asked
+    // to close, and the Restart Manager may not name any of them (see
+    // findProcessesRunningFrom). The file holders follow, deduplicated by process, so an
+    // instance that both runs from the directory and holds a payload file is named once.
+    std::vector<Holder> holders = findProcessesRunningFrom(installDir);
+    std::vector<Holder> fileHolders;
+    const unsigned long error = findProcessesUsingFiles(collectPayloadFiles(installDir), fileHolders);
+    for (const Holder &holder : fileHolders) {
+        const bool alreadyNamed = std::any_of(holders.begin(), holders.end(), [&holder](const Holder &named) {
+            return named.processId == holder.processId;
+        });
+        if (!alreadyNamed) {
+            holders.push_back(holder);
+        }
+    }
+
+    // A failed lookup only makes the probe unable to answer when it also named nobody: with
+    // a process running from the directory the installer still knows what to close, and
+    // that is worth more to the guard than the code of the query that missed it.
+    if (error != ERROR_SUCCESS && holders.empty()) {
         writeLine(L"the Restart Manager lookup failed with win32 error " + std::to_wstring(error));
         return EXIT_PROBE_FAILED;
     }

tools/installer-holder-probe/restart_manager_holders.cpp

@@ -5,10 +5,12 @@
 // here is a requirement; Regroup would sort them the other way round.
 #include <windows.h>
 #include <restartmanager.h>
+#include <tlhelp32.h>
 // clang-format on
 
 #include <algorithm>
 #include <iterator>
+#include <utility>
 
 namespace
 {
@@ -20,6 +22,9 @@ constexpr std::size_t MAX_PROBED_FILES = 2000;
 /// How deep the Plugins walk goes. Its real shape is Plugins\<category>\<plugin>.dll.
 constexpr int MAX_PLUGIN_DEPTH = 4;
 
+/// The payload files the installer replaces by name; the DLLs are matched by mask below.
+constexpr const wchar_t *PAYLOAD_FILES[] = {L"cockatrice.exe", L"oracle.exe", L"servatrice.exe", L"qt.conf"};
+
 void collectFilesOf(const std::wstring &directory, int depth, int maxDepth, std::vector<std::wstring> &files)
 {
     if (files.size() >= MAX_PROBED_FILES) {
@@ -76,6 +81,30 @@ std::wstring fileNameOf(const std::wstring &path)
     return separator == std::wstring::npos ? path : path.substr(separator + 1);
 }
 
+/// @p directory with exactly one trailing separator, so that a sibling whose name merely
+/// begins with the same letters ("Cockatrice-old" next to "Cockatrice") cannot pass for a
+/// subdirectory of it.
+std::wstring directoryPrefixOf(const std::wstring &directory)
+{
+    std::wstring prefix = directory;
+    while (!prefix.empty() && (prefix.back() == L'\\' || prefix.back() == L'/')) {
+        prefix.pop_back();
+    }
+    prefix += L'\\';
+    return prefix;
+}
+
+/// Whether @p path lies inside @p prefix, which is a directory with a trailing separator.
+/// Ordinal and case-insensitive, as Windows compares paths.
+bool isUnderDirectory(const std::wstring &path, const std::wstring &prefix)
+{
+    if (path.size() < prefix.size()) {
+        return false;
+    }
+    const int length = static_cast<int>(prefix.size());
+    return CompareStringOrdinal(path.c_str(), length, prefix.c_str(), length, TRUE) == CSTR_EQUAL;
+}
+
 /// Copies a fixed-size, NUL-terminated WCHAR array the API filled in.
 template <std::size_t N> std::wstring fixedString(const wchar_t (&text)[N])
 {
@@ -96,13 +125,39 @@ Holder holderOf(const RM_PROCESS_INFO &info)
 }
 } // namespace
 
-std::vector<std::wstring> collectProbeFiles(const std::wstring &installDir)
+std::vector<std::wstring> collectPayloadFiles(const std::wstring &installDir)
 {
     std::vector<std::wstring> files;
-    // depth 0, maxDepth 0: the install root's files and nothing below it. Its data
-    // directory holds downloaded pictures, which the purge does not touch and which
-    // would turn this into a walk over thousands of files.
-    collectFilesOf(installDir, 0, 0, files);
+    for (const wchar_t *name : PAYLOAD_FILES) {
+        const std::wstring path = installDir + L"\\" + name;
+        const DWORD attributes = GetFileAttributesW(path.c_str());
+        if (attributes != INVALID_FILE_ATTRIBUTES && (attributes & FILE_ATTRIBUTE_DIRECTORY) == 0) {
+            files.push_back(path);
+        }
+    }
+
+    // Every top-level DLL: the set the purge deletes and therefore the set a running
+    // instance can have mapped. Matched by mask rather than by a list of expected names,
+    // so a transitive dependency nobody thought of is covered as well.
+    WIN32_FIND_DATAW entry{};
+    const HANDLE dlls = FindFirstFileW((installDir + L"\\*.dll").c_str(), &entry);
+    if (dlls != INVALID_HANDLE_VALUE) {
+        do {
+            if (files.size() >= MAX_PROBED_FILES) {
+                break;
+            }
+            if ((entry.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 ||
+                (entry.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
+                continue;
+            }
+            files.push_back(installDir + L"\\" + entry.cFileName);
+        } while (FindNextFileW(dlls, &entry) != 0);
+        FindClose(dlls);
+    }
+
+    // Plugins is the other tree the purge removes. Its data directory is not: it holds
+    // downloaded pictures, which the purge does not touch and which would turn this into
+    // a walk over thousands of files.
     if (files.size() < MAX_PROBED_FILES) {
         collectFilesOf(installDir + L"\\Plugins", 0, MAX_PLUGIN_DEPTH, files);
     }
@@ -158,3 +213,44 @@ unsigned long findProcessesUsingFiles(const std::vector<std::wstring> &files, st
     RmEndSession(session);
     return error;
 }
+
+std::vector<Holder> findProcessesRunningFrom(const std::wstring &installDir)
+{
+    std::vector<Holder> holders;
+    if (installDir.empty()) {
+        return holders;
+    }
+
+    const std::wstring prefix = directoryPrefixOf(installDir);
+    // Never the probe itself: it runs from the directory being replaced when the
+    // uninstaller asks in place or the installer was copied next to the payload, and
+    // naming it would ask the user to close the process that is doing the asking.
+    const DWORD self = GetCurrentProcessId();
+
+    const HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+    if (snapshot == INVALID_HANDLE_VALUE) {
+        return holders;
+    }
+    PROCESSENTRY32W entry{};
+    entry.dwSize = sizeof(entry);
+    if (Process32FirstW(snapshot, &entry) != FALSE) {
+        do {
+            if (entry.th32ProcessID == self) {
+                continue;
+            }
+            // One process at a time, because the path is per process; a process that
+            // refuses to be read is not named here and is still named by Restart
+            // Manager when it holds a handle. The idle process has no image at all.
+            const std::wstring imagePath = imagePathOf(entry.th32ProcessID);
+            if (!isUnderDirectory(imagePath, prefix)) {
+                continue;
+            }
+            Holder holder;
+            holder.processId = entry.th32ProcessID;
+            holder.imageName = fileNameOf(imagePath);
+            holders.push_back(std::move(holder));
+        } while (Process32NextW(snapshot, &entry) != FALSE);
+    }
+    CloseHandle(snapshot);
+    return holders;
+}

tools/installer-holder-probe/restart_manager_holders.h

@@ -7,17 +7,19 @@
 #include <vector>
 
 /**
- * @brief Lists the files a failed purge can be holding up, as full paths.
+ * @brief Lists the payload files, as full paths.
  *
- * Everything directly in @p installDir plus everything below its Plugins directory:
- * those are the two things the purge removes, so they are the two sets of files a
- * holder can be holding. Restart Manager refuses a directory with ERROR_ACCESS_DENIED,
- * which is why the probe registers files and this has to name them first.
+ * The set is what the installer replaces and the purge removes: the three executables,
+ * qt.conf, every top-level DLL and everything below Plugins. A document, a log or a
+ * screenshot the user happens to have open in the install directory is not part of it,
+ * so it cannot be mistaken for a running instance and block an upgrade.
  *
- * Missing or unreadable directories are skipped rather than reported, and the list is
- * capped: this runs on the abort path and must not walk a tree of downloaded pictures.
+ * Restart Manager refuses a directory with ERROR_ACCESS_DENIED, which is why the probe
+ * registers files and this has to name them first. Missing files and unreadable
+ * directories are skipped rather than reported, and the list is capped: this runs before
+ * the first page and on the abort path, and must not walk a tree of downloaded pictures.
  */
-std::vector<std::wstring> collectProbeFiles(const std::wstring &installDir);
+std::vector<std::wstring> collectPayloadFiles(const std::wstring &installDir);
 
 /**
  * @brief Names the applications and services that have one of @p files open or mapped.
@@ -32,4 +34,27 @@ std::vector<std::wstring> collectProbeFiles(const std::wstring &installDir);
  */
 unsigned long findProcessesUsingFiles(const std::vector<std::wstring> &files, std::vector<Holder> &holders);
 
+/**
+ * @brief Lists every process whose image lives in @p installDir, whether or not it has a
+ *        payload file open.
+ *
+ * Restart Manager answers the question "who has this file open", and a running instance
+ * does not necessarily have one open: its own image and the modules it loaded are mapped
+ * into it, not opened by it, and RmGetList does not reliably report a mapping. That is
+ * the state the failed updates this probe exists for were in - every DLL still on disk,
+ * an instance running from the directory, and "nothing is holding them any more" as the
+ * answer - so an answer built on file handles alone cannot be the guard's authority.
+ *
+ * Reading the image path needs PROCESS_QUERY_LIMITED_INFORMATION and
+ * QueryFullProcessImageNameW, neither of which depends on the bitness of either process,
+ * so a 64-bit instance is named from this 32-bit stub. The comparison is on the
+ * directory, not on an image name, so a process that merely shares a name (an Oracle
+ * database server next to a Cockatrice install) is never named.
+ *
+ * @param installDir the directory being replaced, as a full path.
+ * @return one entry per process running from it, in snapshot order, without the probe
+ *         itself; empty when the directory is empty or no process runs from it.
+ */
+std::vector<Holder> findProcessesRunningFrom(const std::wstring &installDir);
+
 #endif