[Installer] Offer to close blocking processes and retry purge on upgrade (#7664)

* [Installer] Offer to close blocking processes and retry purge on upgrade

* [Installer] Retry the purge after closing only our own applications

The first version of this fix closed every Restart Manager holder through
RmShutdown, forcing what refused to close. RmShutdown also stops services, and
the only API that brings them back is RmRestart, which would restart Cockatrice
itself into the files that are about to be replaced; the forced pass also
contradicts the running-instance guard, which never force-closes Cockatrice
because it may be holding an unsaved deck. The guard ran first anyway, so for
the case the fix exists for - a Cockatrice restarted during the update - the
probe never got to run: the guard waited 60 s and aborted.

Keep the prompt and the retry, but ask only Cockatrice, Oracle and Servatrice to
close, with WM_CLOSE and nothing else, and leave every other holder named by the
probe to the user. Repeat the prompt while the user keeps saying Yes, refresh
the names after a failed retry, and keep the /R abort message pointing at the
downloaded installer.

Also clear the read-only attribute before every purge attempt. Delete and RMDir
leave such a file in place, so the retry could never make progress, and the
diagnostics branch that named the cause had no action behind it.

The holder probe goes back to being diagnostic-only, so its Restart Manager
code and the --close argument are dropped.

* [Installer] Make an accepted close of the running client actually land

WM_CLOSE is a request, and Cockatrice answers it: while a card database
update is running, a game is open or a deck is unsaved it puts up its own
"are you sure?" confirmation. That dialog opened behind the installer's
prompt, so the user saw nothing happen; the next Yes was posted to a main
window the app's own modal dialog had disabled, so it was ignored as well,
and the identical yes/no prompt came back with no hint of why.

- Activate the matching window before posting WM_CLOSE, so the app's own
  confirmation is in front where it can be answered.
- Wait up to 30 s for cockatrice.exe to exit instead of purging after a
  fixed 2 s, so the purge measures the app gone rather than its dialog.
- On the second and later rounds ask with Retry/Cancel and say what is
  needed, instead of repeating the same offer that just did nothing.
- Stop promising Oracle and Servatrice: they are console applications, so
  CloseMainWindow() has nothing to send WM_CLOSE to. They are not
  force-stopped either - Oracle writes the card database in place.

* [Installer] Close the client from the holder probe, not PowerShell

Accepting the offer to close did nothing on a real machine: the instance stayed
up, never came forward, and the same prompt came back.

The prompt names its holders from the Restart Manager probe, but the close itself
went through powershell.exe: Get-Process -Name, a strict "path starts with
$INSTDIR" filter, then Process.CloseMainWindow() and a WScript.Shell AppActivate.
Every step of that is silent. A process whose path cannot be read is filtered
out, a process .NET finds no main window for makes CloseMainWindow() return
false without posting anything, and AppActivate is refused for a background
caller - so an attempted close, a missed close and a close request the client
ignored all looked the same in the log: nothing.

Ask the probe instead. installer-holder-probe --close-holders closes exactly the
processes Restart Manager just named as holding a file of the version being
replaced, so no executable path has to be guessed and a third party application
that merely shares the image name is never in the set. Per instance it restores
and brings the top-level window forward (AttachThreadInput, so the client's own
"really quit?" confirmation is on screen rather than behind the installer), posts
WM_CLOSE to the main window, and waits up to 30 s for the process to exit. Its
report names every holder with what happened to it, including the ones left to
the user, and goes into the installer log as one line.

That also removes the separate PowerShell polling loop that used to wait for the
close: the probe holds a handle on the process it asked, so the purge that
follows measures a settled directory.

Oracle and Servatrice are still never closed, and nothing is force-stopped.

---------

Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com>
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

tools/installer-holder-probe/CMakeLists.txt

@@ -4,7 +4,7 @@
 # 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.
-add_executable(installer-holder-probe holder_line.cpp main.cpp restart_manager_holders.cpp)
+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/close_holders.cpp

@@ -0,0 +1,212 @@
+#include "close_holders.h"
+
+// clang-format off
+// windows.h has to come before anything that pulls in a Windows type, as everywhere in
+// this tool.
+#include <windows.h>
+// clang-format on
+
+#include <cstddef>
+#include <string>
+#include <utility>
+#include <vector>
+
+namespace
+{
+constexpr wchar_t COCKATRICE_IMAGE[] = L"cockatrice.exe";
+
+/// How long one poll of the asked processes takes. One WaitForSingleObject per process
+/// and no enumeration, so polling costs nothing next to the seconds this is called on.
+constexpr unsigned long WAIT_POLL_MILLISECONDS = 200;
+
+/// How long the report line may get before it is cut. NSIS holds one string in 1024
+/// characters, and this one is written there verbatim; every holder is named, so normally
+/// the line stays far below it.
+constexpr std::size_t REPORT_CHAR_MAX = 900;
+
+bool isCockatriceImage(const std::wstring &imageName)
+{
+    return CompareStringOrdinal(imageName.c_str(), -1, COCKATRICE_IMAGE, -1, TRUE) == CSTR_EQUAL;
+}
+
+/// The holder as the log has to name it to be actionable. Deliberately not
+/// formatHolder(): that one is worded for the dialog, where ", running" says something
+/// the line here continues after.
+std::wstring processLabel(const Holder &holder)
+{
+    std::wstring label = holder.imageName.empty() ? holder.appName : holder.imageName;
+    if (label.empty()) {
+        label = L"an unnamed process";
+    }
+    label += L" (pid ";
+    label += std::to_wstring(holder.processId);
+    label += L")";
+    return label;
+}
+
+struct WindowSearch
+{
+    DWORD processId = 0;
+    HWND topmost = nullptr; ///< first visible window in Z-order: a dialog when one is up
+    HWND main = nullptr;    ///< first visible window without an owner: the one to close
+    HWND any = nullptr;     ///< first window at all, used when nothing is visible
+};
+
+BOOL CALLBACK findProcessWindow(HWND window, LPARAM parameter)
+{
+    auto &search = *reinterpret_cast<WindowSearch *>(parameter);
+    DWORD processId = 0;
+    GetWindowThreadProcessId(window, &processId);
+    if (processId != search.processId) {
+        return TRUE;
+    }
+    if (search.any == nullptr) {
+        search.any = window;
+    }
+    // A tray icon leaves the main window hidden, so a window that is not visible is still
+    // kept as a fallback. EnumWindows walks the Z-order top down, so the first visible
+    // window is the one the user would see in front.
+    if (IsWindowVisible(window) != FALSE) {
+        if (search.topmost == nullptr) {
+            search.topmost = window;
+        }
+        if (search.main == nullptr && GetWindow(window, GW_OWNER) == nullptr) {
+            search.main = window;
+        }
+    }
+    return TRUE;
+}
+
+/// Restores a minimized window and takes the foreground, so a confirmation Cockatrice
+/// answers the close with is in front of the installer instead of behind it.
+///
+/// A process in the background may not take the foreground on its own. Attaching to the
+/// thread that owns the foreground window lifts that restriction for the duration of the
+/// call; detaching afterwards leaves the two input queues as they were.
+bool bringForward(HWND window)
+{
+    if (IsIconic(window) != FALSE) {
+        ShowWindow(window, SW_RESTORE);
+    } else {
+        ShowWindow(window, SW_SHOW);
+    }
+
+    const HWND foreground = GetForegroundWindow();
+    const DWORD foregroundThread = foreground != nullptr ? GetWindowThreadProcessId(foreground, nullptr) : 0;
+    const DWORD currentThread = GetCurrentThreadId();
+    const bool attached = foregroundThread != 0 && foregroundThread != currentThread &&
+                          AttachThreadInput(currentThread, foregroundThread, TRUE) != FALSE;
+    BringWindowToTop(window);
+    SetForegroundWindow(window);
+    if (attached) {
+        AttachThreadInput(currentThread, foregroundThread, FALSE);
+    }
+    return GetForegroundWindow() == window;
+}
+
+/// Waits for @p processes to signal, up to @p waitMilliseconds. Returns whether they all
+/// exited and how long the wait took.
+bool waitForExit(const std::vector<HANDLE> &processes, unsigned long waitMilliseconds, unsigned long long &elapsed)
+{
+    const ULONGLONG start = GetTickCount64();
+    const ULONGLONG deadline = start + waitMilliseconds;
+    for (;;) {
+        bool allExited = true;
+        for (const HANDLE process : processes) {
+            if (WaitForSingleObject(process, 0) != WAIT_OBJECT_0) {
+                allExited = false;
+                break;
+            }
+        }
+        elapsed = GetTickCount64() - start;
+        if (allExited || GetTickCount64() >= deadline) {
+            return allExited;
+        }
+        Sleep(WAIT_POLL_MILLISECONDS);
+    }
+}
+} // namespace
+
+CloseHoldersResult closeCockatriceHolders(const std::vector<Holder> &holders, unsigned long waitMilliseconds)
+{
+    CloseHoldersResult result;
+    std::vector<std::wstring> lines;
+    std::vector<HANDLE> asked;
+    std::vector<std::size_t> askedLines;
+
+    for (const Holder &holder : holders) {
+        if (holder.isService || !isCockatriceImage(holder.imageName)) {
+            // Oracle and Servatrice are named by the prompt and close by hand: Oracle
+            // writes the card database in place. A service has no window to ask either.
+            lines.push_back(processLabel(holder) + L" left to you");
+            continue;
+        }
+
+        WindowSearch search;
+        search.processId = holder.processId;
+        EnumWindows(findProcessWindow, reinterpret_cast<LPARAM>(&search));
+        if (search.any == nullptr) {
+            lines.push_back(processLabel(holder) + L" has no window to close, left to you");
+            continue;
+        }
+
+        const HWND target =
+            search.main != nullptr ? search.main : (search.topmost != nullptr ? search.topmost : search.any);
+        const HWND toRaise = search.topmost != nullptr ? search.topmost : target;
+        std::wstring line = processLabel(holder);
+        line += bringForward(toRaise) ? L" brought forward" : L" could not be brought forward";
+
+        // Kept open across the wait so a pid reused after the process exits cannot be
+        // mistaken for the instance that was asked to close.
+        const HANDLE process = OpenProcess(SYNCHRONIZE, FALSE, holder.processId);
+        const DWORD openError = process == nullptr ? GetLastError() : ERROR_SUCCESS;
+        if (PostMessageW(target, WM_CLOSE, 0, 0) == FALSE) {
+            // Read the error before CloseHandle, which is free to reset it.
+            const DWORD postError = GetLastError();
+            if (process != nullptr) {
+                CloseHandle(process);
+            }
+            lines.push_back(line + L", and the close request could not be sent (win32 " + std::to_wstring(postError) +
+                            L")");
+            continue;
+        }
+        line += L" and asked to close";
+        ++result.askedToClose;
+        if (process == nullptr) {
+            line += L", but its exit could not be watched (win32 " + std::to_wstring(openError) + L")";
+        } else {
+            askedLines.push_back(lines.size());
+            asked.push_back(process);
+        }
+        lines.push_back(std::move(line));
+    }
+
+    if (!asked.empty() && waitMilliseconds > 0) {
+        unsigned long long elapsed = 0;
+        const bool allExited = waitForExit(asked, waitMilliseconds, elapsed);
+        for (std::size_t index = 0; index < asked.size(); ++index) {
+            const bool exited = allExited || WaitForSingleObject(asked[index], 0) == WAIT_OBJECT_0;
+            lines[askedLines[index]] += exited ? L", exited after " + std::to_wstring(elapsed) + L" ms"
+                                               : L", still running after " + std::to_wstring(elapsed) + L" ms";
+        }
+    }
+    for (const HANDLE process : asked) {
+        CloseHandle(process);
+    }
+
+    if (lines.empty()) {
+        result.report = L"nothing is holding the files";
+        return result;
+    }
+    for (const std::wstring &line : lines) {
+        if (!result.report.empty()) {
+            result.report += L"; ";
+        }
+        result.report += line;
+    }
+    if (result.report.size() > REPORT_CHAR_MAX) {
+        result.report.erase(REPORT_CHAR_MAX);
+        result.report += L"...";
+    }
+    return result;
+}

tools/installer-holder-probe/close_holders.h

@@ -0,0 +1,46 @@
+#ifndef CLOSE_HOLDERS_H
+#define CLOSE_HOLDERS_H
+
+#include "holder_line.h"
+
+#include <string>
+#include <vector>
+
+/**
+ * @brief What asking the holders to close did, in the form the installer logs.
+ *
+ * @c report is one line naming every holder, including the ones this deliberately
+ * leaves alone, because the installer repeats it in its log and in the second prompt.
+ */
+struct CloseHoldersResult
+{
+    unsigned int askedToClose = 0; ///< holders a WM_CLOSE was posted to
+    std::wstring report;           ///< one line, narrowed to ASCII when written
+};
+
+/**
+ * @brief Brings every running Cockatrice holding an installer file forward and asks it
+ *        to close.
+ *
+ * The set is the Restart Manager answer, so it is exactly the processes holding a file of
+ * the previous version: nothing here has to guess at an executable path, and an
+ * application that merely shares the image name is never touched. Only cockatrice.exe is
+ * asked to close. Oracle and Servatrice are left to the user - Oracle writes the card
+ * database in place - and so is every service.
+ *
+ * Two things happen per instance. Its top-level window is restored and brought to the
+ * foreground, because Cockatrice may answer the close with a confirmation of its own that
+ * would otherwise open behind the installer's prompt, and then WM_CLOSE is posted to the
+ * main window, the same request as clicking its close button. Stopping the process is
+ * never attempted.
+ *
+ * Once the requests are out, this waits up to @c waitMilliseconds for the processes to
+ * exit, so the caller can measure the purge immediately afterwards instead of sleeping
+ * for a fixed time or re-asking a question this call already answered.
+ *
+ * @param holders the Restart Manager answer, one entry per application or service
+ * @param waitMilliseconds how long to wait for the asked instances to exit, 0 to not wait
+ */
+CloseHoldersResult closeCockatriceHolders(const std::vector<Holder> &holders, unsigned long waitMilliseconds);
+
+#endif

tools/installer-holder-probe/main.cpp

@@ -1,3 +1,4 @@
+#include "close_holders.h"
 #include "holder_line.h"
 #include "restart_manager_holders.h"
 
@@ -12,8 +13,16 @@ namespace
 /// trailing backslash then never has to survive NSIS quoting or a command line.
 constexpr wchar_t INSTALL_DIR_VARIABLE[] = L"COCKATRICE_GUARD_DIR";
 
+/// How long the close mode waits for the instance it asked to exit before it reports the
+/// process as still running. The installer measures the purge right after this call, so
+/// the wait belongs here: it is the only place that holds a handle on the process whose
+/// exit it is waiting for.
+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.
 enum ExitCode
 {
     EXIT_NO_HOLDERS = 0,
@@ -22,6 +31,20 @@ enum ExitCode
     EXIT_PROBE_FAILED = 3,
 };
 
+/// 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
+///   --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.
+bool closeModeRequested(int argumentCount, char **arguments)
+{
+    return argumentCount > 1 && std::string(arguments[1]) == "--close-holders";
+}
+
 /// Writes one line to the pipe nsExec reads. Narrowed to ASCII on purpose: the bytes
 /// come back to the installer as an ANSI string, so a non-ASCII process name would be
 /// mojibake in the log; a '?' at least stays honest. Control characters are dropped
@@ -54,7 +77,7 @@ std::wstring installDirFromEnvironment()
 }
 } // namespace
 
-int main()
+int main(int argumentCount, char **arguments)
 {
     const std::wstring installDir = installDirFromEnvironment();
     if (installDir.empty()) {
@@ -68,11 +91,23 @@ int main()
         writeLine(L"the Restart Manager lookup failed with win32 error " + std::to_wstring(error));
         return EXIT_PROBE_FAILED;
     }
+
+    const bool closeMode = closeModeRequested(argumentCount, arguments);
     if (holders.empty()) {
-        // No output: the installer words the "nothing is holding them" case, and it
-        // owns the message either way.
+        // No output in the naming mode: the installer words the "nothing is holding them"
+        // case, and it owns the message either way. The close mode says it itself, because
+        // its line is logged verbatim next to what it was asked to do.
+        if (closeMode) {
+            writeLine(L"nothing is holding the files");
+        }
         return EXIT_NO_HOLDERS;
     }
-    writeLine(formatHolders(holders));
-    return EXIT_HOLDERS_FOUND;
+    if (!closeMode) {
+        writeLine(formatHolders(holders));
+        return EXIT_HOLDERS_FOUND;
+    }
+
+    const CloseHoldersResult result = closeCockatriceHolders(holders, CLOSE_WAIT_MILLISECONDS);
+    writeLine(result.report);
+    return result.askedToClose > 0 ? EXIT_HOLDERS_FOUND : EXIT_NO_HOLDERS;
 }