[Installer] Stop calling the payload free while an executable is locked

The guard answered "nothing is holding the payload" from two lookups that can
both miss a running instance: Restart Manager does not reliably report a
program's own image or the modules it mapped, and the image path of a process
running under another account cannot be read from the probe at all. A user log
showed exactly that: the first guard passed while cockatrice.exe was running
from the directory, the old uninstaller silently skipped the file it could not
delete, and the update aborted half an uninstall later with "cancelled by user"
after a close attempt the running window ignored.

Ask the file itself. A running image is opened without write sharing, so the
write open replacing it needs is refused with ERROR_SHARING_VIOLATION for as
long as the program runs, while a file that is merely being read still accepts
it. When neither lookup named anybody and a payload executable refuses that
open, the probe answers that the file is in use and that the program holding it
could not be identified - the guard's "could not confirm" dialog, whose Retry
works once the user has closed it - instead of passing the payload as free.

Only the payload executables are tested, not every file the purge removes: a
locked executable proves a program is running and a mapped DLL belongs to a
running executable, while a locked DLL or qt.conf can be the asking process
itself, which is the loop the guard already had. The open is retried once after
300 ms, and only for the files that refused, so a scanner holding a file for a
moment cannot stop an install for nothing.

The Windows probe check now covers that answer: a payload executable held by
the asking process, so that it cannot be named, has to come back as in use
rather than as free.

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

tools/installer-holder-probe/CMakeLists.txt

@@ -6,7 +6,8 @@
 # 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 payload_paths.cpp restart_manager_holders.cpp
+  installer-holder-probe close_holders.cpp holder_line.cpp main.cpp payload_locks.cpp payload_paths.cpp
+                         restart_manager_holders.cpp
 )
 
 target_link_libraries(installer-holder-probe PRIVATE rstrtmgr)

tools/installer-holder-probe/main.cpp

@@ -1,5 +1,7 @@
 #include "close_holders.h"
 #include "holder_line.h"
+#include "payload_locks.h"
+#include "payload_paths.h"
 #include "restart_manager_holders.h"
 
 #include <algorithm>
@@ -33,11 +35,12 @@ constexpr unsigned long END_WAIT_MILLISECONDS = 5000;
 /// 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. 3 is only for
-/// a lookup that failed and named nobody: one that failed but still named a payload program
-/// running from the directory is answered with that process. 4 is only the launcher mode:
-/// the process the installer waits on has no window a user could answer, which is the one
-/// answer the installer acts on by itself instead of asking the user.
+/// to the user, which is what the report line next to the code spells out. 3 is for an
+/// answer that could not name what it found: a lookup that failed and named nobody, or the
+/// lookups naming nobody while a payload program refuses to be replaced, which is proof one
+/// runs from the directory. 4 is only the launcher mode: the process the installer waits on
+/// has no window a user could answer, which is the one answer the installer acts on by
+/// itself instead of asking the user.
 enum ExitCode
 {
     EXIT_NO_HOLDERS = 0,
@@ -60,12 +63,15 @@ enum ExitCode
 ///                     whether there was a window to ask at all, because an instance with
 ///                     no window has answered its own close and is stuck in shutdown
 ///
-/// The guard's three modes answer from two lookups that cover each other's blind spot: the
-/// Restart Manager names whoever has a payload file open, and every payload program running
+/// The guard's three modes answer from three questions that cover each other's blind spots:
+/// the Restart Manager names whoever has a payload file open, every payload program running
 /// from the directory itself 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.
+/// not always report those, and a payload executable that refuses to be written is running
+/// even when neither lookup could name it (see payloadExecutablesInUse). No one of them is
+/// the guard's authority: an instance the process lookups do not name, while its executable
+/// is locked, is exactly the case that left a half-replaced install behind, so it is reported
+/// as a check that could not name what it found instead of as a free payload.
 /// A program in a folder below the directory is not named by the second lookup: it is not an
 /// instance of this payload, and the guard could neither close nor end it.
 enum class Mode
@@ -109,6 +115,24 @@ void writeLine(const std::wstring &text)
     WriteFile(GetStdHandle(STD_OUTPUT_HANDLE), narrow.data(), static_cast<DWORD>(narrow.size()), &written, nullptr);
 }
 
+/// The line for a payload executable that is in use while no lookup could name the program
+/// holding it: the answer the guard shows, so it names the file the user has to free and
+/// says that the program doing it is still to be closed by hand.
+std::wstring inUseLine(const std::vector<std::wstring> &files)
+{
+    std::wstring names;
+    for (const std::wstring &file : files) {
+        if (!names.empty()) {
+            names += L", ";
+        }
+        names += fileNameOf(file);
+    }
+    if (files.size() == 1) {
+        return names + L" is in use and the program holding it could not be identified";
+    }
+    return names + L" are in use and the programs holding them could not be identified";
+}
+
 std::wstring environmentVariable(const wchar_t *name)
 {
     for (DWORD size = MAX_PATH; size <= 32768U; size *= 2) {
@@ -226,15 +250,30 @@ int main(int argumentCount, char **arguments)
         }
     }
 
-    // A failed lookup only makes the probe unable to answer when it also named nobody: with
-    // a payload program 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;
-    }
-
+    // Nobody was named. That is not yet the answer that nothing holds the payload: a lookup
+    // that failed only makes the probe unable to answer when it also named nobody, and a
+    // payload executable that refuses to be replaced is running even when neither lookup
+    // could name it.
     if (holders.empty()) {
+        // Neither process lookup is proof that the payload is free: the Restart Manager does
+        // not report every mapping, and a program running under another account cannot be
+        // read from here, so "nobody was named" is not the answer "nothing is running". The
+        // file cannot be missed - a running image refuses the write the replacement needs -
+        // and an answer of "nothing is holding them" over that locked file is what sent
+        // updates into an uninstall that could only fail halfway. Reported as a check that
+        // could not name what it found, so the guard asks the user to close it and retry
+        // instead of replacing a payload that is still running.
+        const std::vector<std::wstring> inUse = payloadExecutablesInUse(installDir);
+        if (!inUse.empty()) {
+            writeLine(inUseLine(inUse));
+            return EXIT_PROBE_FAILED;
+        }
+
+        if (error != ERROR_SUCCESS) {
+            writeLine(L"the Restart Manager lookup failed with win32 error " + std::to_wstring(error));
+            return EXIT_PROBE_FAILED;
+        }
+
         // No output in the naming mode: the installer words the "nothing is holding them"
         // case, and it owns the message either way. The acting modes say it themselves,
         // because their line is logged verbatim next to what they were asked to do.

tools/installer-holder-probe/payload_locks.cpp

@@ -0,0 +1,67 @@
+#include "payload_locks.h"
+
+#include "payload_paths.h"
+
+#include <windows.h>
+
+namespace
+{
+/// How long the second look waits before it re-tests the files that refused the write open.
+/// Only those are re-tested, so the one answer costs this at most once: a scanner, or a
+/// program that is only starting, holds a file without write sharing for a moment, and that
+/// must not turn a free payload into a run the user has to answer.
+constexpr DWORD RECHECK_DELAY_MILLISECONDS = 300;
+
+/// Whether @p path refuses an open that asks to write it.
+///
+/// Windows opens a program's image, and every module loaded into it, with read and delete
+/// sharing but not write sharing, so this is refused for as long as the program runs and by
+/// nothing else that merely reads the file. A read-only file is skipped: it refuses a write
+/// open for its own reason and would answer yes for a payload nobody is running, and the
+/// purge clears that attribute before it deletes anything. Any other error is no answer at
+/// all - access this probe does not have says nothing about whether the file is in use, and
+/// still answering "in use" would stop installs that could have finished.
+bool refusesWrite(const std::wstring &path)
+{
+    const DWORD attributes = GetFileAttributesW(path.c_str());
+    if (attributes == INVALID_FILE_ATTRIBUTES ||
+        (attributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_READONLY)) != 0) {
+        return false;
+    }
+    const HANDLE file = CreateFileW(path.c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                                    nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
+    if (file == INVALID_HANDLE_VALUE) {
+        return GetLastError() == ERROR_SHARING_VIOLATION;
+    }
+    // Nothing was written: the open is the whole test, and closing the handle leaves the
+    // file exactly as it was.
+    CloseHandle(file);
+    return false;
+}
+} // namespace
+
+std::vector<std::wstring> payloadExecutablesInUse(const std::wstring &installDir)
+{
+    std::vector<std::wstring> refused;
+    for (const wchar_t *name : PAYLOAD_PROGRAMS) {
+        const std::wstring path = installDir + L"\\" + name;
+        if (refusesWrite(path)) {
+            refused.push_back(path);
+        }
+    }
+    if (refused.empty()) {
+        return refused;
+    }
+
+    // One re-test of the files that refused, and only those: the answer is shown to a user
+    // who then has to close a program and run the installer again, so it has to be the
+    // settled state rather than a moment in which something happened to hold the file.
+    Sleep(RECHECK_DELAY_MILLISECONDS);
+    std::vector<std::wstring> settled;
+    for (const std::wstring &path : refused) {
+        if (refusesWrite(path)) {
+            settled.push_back(path);
+        }
+    }
+    return settled;
+}

tools/installer-holder-probe/payload_locks.h

@@ -0,0 +1,36 @@
+#ifndef PAYLOAD_LOCKS_H
+#define PAYLOAD_LOCKS_H
+
+#include <string>
+#include <vector>
+
+/**
+ * @brief The payload executables in @p installDir that refuse to be written, as full paths:
+ *        the programs that are running from it right now.
+ *
+ * The guard asks two questions and both can come back empty for a running instance: the
+ * Restart Manager does not reliably report a program's own image or the modules it mapped,
+ * and the image path of a process running under another account cannot be read from here,
+ * so the process is not named either. The file itself is the evidence neither question
+ * depends on: a program's image is opened without write sharing for as long as it runs, so
+ * the write access replacing it needs is refused for exactly that long, while a file that is
+ * merely being read still accepts it.
+ *
+ * An installer that answers "nothing is holding the payload" from two lookups that both
+ * missed is what sent updates into an uninstall that could only fail halfway, leaving the
+ * previous version half removed. This answer is what keeps that from being said: a payload
+ * executable that refuses the write is running, whether or not this process may name it.
+ *
+ * Only the programs the payload installs are tested, not every file the purge removes. A
+ * payload executable refusing the write proves that it is running; a mapped module belongs
+ * to a running executable, which is tested. A DLL or qt.conf on its own can be held by the
+ * program that is asking - the uninstaller runs in place from the directory it empties - and
+ * naming that as a holder is the loop the guard already had.
+ *
+ * @return one full path per executable that still refused after the re-test, in the order
+ *         PAYLOAD_PROGRAMS names them; empty when every one of them can be written, or none
+ *         is installed.
+ */
+std::vector<std::wstring> payloadExecutablesInUse(const std::wstring &installDir);
+
+#endif

tools/installer-holder-probe/payload_paths.cpp

@@ -6,10 +6,6 @@
 
 namespace
 {
-/// The payload programs, by image name. A process running one of these is what the guard
-/// exists to name, so askingProcessIds() stops its walk up the parent chain at one.
-constexpr const wchar_t *PAYLOAD_PROGRAMS[] = {L"cockatrice.exe", L"oracle.exe", L"servatrice.exe"};
-
 /// Compares two file names the way the file system does, without a locale.
 ///
 /// Only the names above are ever compared against, and all of them are ASCII, so folding

tools/installer-holder-probe/payload_paths.h

@@ -3,6 +3,18 @@
 
 #include <string>
 
+/**
+ * @brief The file names of the programs the payload installs, in the order the guard names
+ *        them.
+ *
+ * The single list behind isPayloadProgram(), so the guard acts on exactly the programs it
+ * names, and so a payload program added later is closed, ended and tested for a lock
+ * without a second list to update. A payload executable that refuses to be written is
+ * running, which is what makes this list the evidence the two process lookups cannot
+ * provide - see payloadExecutablesInUse().
+ */
+inline constexpr const wchar_t *PAYLOAD_PROGRAMS[] = {L"cockatrice.exe", L"oracle.exe", L"servatrice.exe"};
+
 /**
  * @brief The last name in @p path: what follows its last separator, or @p path itself when
  *        it has none.

tools/installer-holder-probe/restart_manager_holders.cpp

@@ -27,9 +27,6 @@ constexpr int MAX_PLUGIN_DEPTH = 4;
 /// only keeps a snapshot with a cycle in it from walking forever.
 constexpr int MAX_ASKER_LEVELS = 8;
 
-/// 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) {
@@ -129,7 +126,12 @@ Holder holderOf(const RM_PROCESS_INFO &info)
 std::vector<std::wstring> collectPayloadFiles(const std::wstring &installDir)
 {
     std::vector<std::wstring> files;
-    for (const wchar_t *name : PAYLOAD_FILES) {
+    // The programs the payload installs, by name, plus the configuration file next to
+    // them: what the purge deletes by name, and therefore what an instance can have
+    // mapped.
+    std::vector<std::wstring> replaced(std::begin(PAYLOAD_PROGRAMS), std::end(PAYLOAD_PROGRAMS));
+    replaced.emplace_back(L"qt.conf");
+    for (const std::wstring &name : replaced) {
         const std::wstring path = installDir + L"\\" + name;
         const DWORD attributes = GetFileAttributesW(path.c_str());
         if (attributes != INVALID_FILE_ATTRIBUTES && (attributes & FILE_ATTRIBUTE_DIRECTORY) == 0) {