[Installer] Name only the payload programs installed in the directory (#7684)

The running-instance lookup named every process whose image path began with the
install directory, subfolders included. Installing into a folder that holds
another application in a subfolder therefore reported that application as
holding the previous version, and nothing the guard offers could clear it: the
close asks cockatrice.exe only, and the end answer refuses anything that is not
a payload program. Both answers were no-ops, so the update could only abort and
tell the user to close a program that was never touching the files.

Name only the programs the payload installs, and only when the image lies in the
install directory itself. A program below the directory that really does hold a
payload file is still named by the Restart Manager lookup, which is the lookup
for that question.

The Windows probe check now covers a payload-named program in a subfolder and a
program the payload does not install; both have to come back as nothing.

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

tools/installer-holder-probe/CMakeLists.txt

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

tools/installer-holder-probe/close_holders.cpp

@@ -1,5 +1,6 @@
 #include "close_holders.h"
 
+#include "payload_paths.h"
 #include "restart_manager_holders.h"
 
 // clang-format off

tools/installer-holder-probe/main.cpp

@@ -34,7 +34,7 @@ constexpr unsigned long END_WAIT_MILLISECONDS = 5000;
 /// 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 process
+/// 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.
@@ -61,11 +61,13 @@ enum ExitCode
 ///                     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 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.
+/// Restart Manager names whoever has a payload file open, and 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.
+/// 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
 {
     Name,          ///< name the holders
@@ -201,8 +203,8 @@ int main(int argumentCount, char **arguments)
         return closeLauncher(installDir);
     }
 
-    // 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
+    // The payload programs 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.
     // Neither lookup may name the probe or the process that started it, whichever one found
@@ -225,8 +227,8 @@ int main(int argumentCount, char **arguments)
     }
 
     // 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.
+    // 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;

tools/installer-holder-probe/payload_paths.cpp

@@ -0,0 +1,52 @@
+#include "payload_paths.h"
+
+#include <algorithm>
+#include <cstddef>
+#include <iterator>
+
+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
+/// the ASCII letters is the whole answer. The Windows ordinal comparison is not used here
+/// on purpose: this file is compiled into a test that runs everywhere, and the question
+/// "is this image a program the payload installs?" must be answerable without Windows.
+bool equalsIgnoringAsciiCase(const std::wstring &left, const std::wstring &right)
+{
+    if (left.size() != right.size()) {
+        return false;
+    }
+    return std::equal(left.begin(), left.end(), right.begin(), [](wchar_t a, wchar_t b) {
+        const wchar_t foldedA = a >= L'A' && a <= L'Z' ? static_cast<wchar_t>(a - L'A' + L'a') : a;
+        const wchar_t foldedB = b >= L'A' && b <= L'Z' ? static_cast<wchar_t>(b - L'A' + L'a') : b;
+        return foldedA == foldedB;
+    });
+}
+} // namespace
+
+std::wstring fileNameOf(const std::wstring &path)
+{
+    const std::size_t separator = path.find_last_of(L"\\/");
+    return separator == std::wstring::npos ? path : path.substr(separator + 1);
+}
+
+std::wstring parentDirectoryOf(const std::wstring &path)
+{
+    const std::size_t separator = path.find_last_of(L"\\/");
+    return separator == std::wstring::npos ? std::wstring() : path.substr(0, separator);
+}
+
+bool isPayloadProgram(const std::wstring &imageName)
+{
+    for (const wchar_t *program : PAYLOAD_PROGRAMS) {
+        if (equalsIgnoringAsciiCase(imageName, program)) {
+            return true;
+        }
+    }
+    return false;
+}

tools/installer-holder-probe/payload_paths.h

@@ -0,0 +1,39 @@
+#ifndef PAYLOAD_PATHS_H
+#define PAYLOAD_PATHS_H
+
+#include <string>
+
+/**
+ * @brief The last name in @p path: what follows its last separator, or @p path itself when
+ *        it has none.
+ *
+ * Empty for an empty path and for one that ends in a separator, which has no name of its
+ * own - a caller that hands in a directory there gets no name rather than the directory's
+ * own last component.
+ */
+std::wstring fileNameOf(const std::wstring &path);
+
+/**
+ * @brief The directory @p path lies in, without a trailing separator.
+ *
+ * Empty when @p path has no directory part. Used to tell where a process was started from:
+ * an image running directly in the install directory has that directory as its parent,
+ * while one started from a folder below it has that folder, and the two are not the same
+ * answer to "does this process belong to the installation being replaced?".
+ */
+std::wstring parentDirectoryOf(const std::wstring &path);
+
+/**
+ * @brief Whether @p imageName is one of the programs the payload installs.
+ *
+ * The single place that answers which images are Cockatrice's own, so the guard acts on the
+ * same set it names, and so a program a user happens to keep in the install directory is
+ * never treated as an instance of the application. Matched on the image name alone, without
+ * case: the names are the payload's own, and the comparison is deliberately free of Windows
+ * API so it can be exercised off Windows.
+ *
+ * @param imageName the file name of a process image, empty when it could not be read
+ */
+bool isPayloadProgram(const std::wstring &imageName);
+
+#endif

tools/installer-holder-probe/restart_manager_holders.cpp

@@ -30,10 +30,6 @@ 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"};
 
-/// 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"};
-
 void collectFilesOf(const std::wstring &directory, int depth, int maxDepth, std::vector<std::wstring> &files)
 {
     if (files.size() >= MAX_PROBED_FILES) {
@@ -84,15 +80,9 @@ std::wstring imagePathOf(unsigned long processId)
     return imagePath;
 }
 
-std::wstring fileNameOf(const std::wstring &path)
-{
-    const std::size_t separator = path.find_last_of(L"\\/");
-    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.
+/// @p directory with exactly one trailing separator, so that two spellings of the same
+/// directory compare equal and a sibling whose name merely begins with the same letters
+/// ("Cockatrice-old" next to "Cockatrice") cannot pass for it.
 std::wstring directoryPrefixOf(const std::wstring &directory)
 {
     std::wstring prefix = directory;
@@ -103,15 +93,17 @@ std::wstring directoryPrefixOf(const std::wstring &directory)
     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)
+/// Whether @p left and @p right name the same directory, compared the way the file system
+/// does: a difference in case is not a difference in directory, and a trailing separator
+/// changes nothing.
+bool isSameDirectory(const std::wstring &left, const std::wstring &right)
 {
-    if (path.size() < prefix.size()) {
+    if (left.empty() || right.empty()) {
         return false;
     }
-    const int length = static_cast<int>(prefix.size());
-    return CompareStringOrdinal(path.c_str(), length, prefix.c_str(), length, TRUE) == CSTR_EQUAL;
+    const std::wstring leftPrefix = directoryPrefixOf(left);
+    const std::wstring rightPrefix = directoryPrefixOf(right);
+    return CompareStringOrdinal(leftPrefix.c_str(), -1, rightPrefix.c_str(), -1, TRUE) == CSTR_EQUAL;
 }
 
 /// Copies a fixed-size, NUL-terminated WCHAR array the API filled in.
@@ -134,16 +126,6 @@ Holder holderOf(const RM_PROCESS_INFO &info)
 }
 } // namespace
 
-bool isPayloadProgram(const std::wstring &imageName)
-{
-    for (const wchar_t *program : PAYLOAD_PROGRAMS) {
-        if (CompareStringOrdinal(imageName.c_str(), -1, program, -1, TRUE) == CSTR_EQUAL) {
-            return true;
-        }
-    }
-    return false;
-}
-
 std::vector<std::wstring> collectPayloadFiles(const std::wstring &installDir)
 {
     std::vector<std::wstring> files;
@@ -279,7 +261,6 @@ std::vector<Holder> findProcessesRunningFrom(const std::wstring &installDir)
         return holders;
     }
 
-    const std::wstring prefix = directoryPrefixOf(installDir);
     // Never the probe, and never the installer or uninstaller that started it: either can
     // run from the directory being replaced, and naming one asks the user to close the
     // program that is asking (see askingProcessIds).
@@ -300,12 +281,20 @@ std::vector<Holder> findProcessesRunningFrom(const std::wstring &installDir)
             // 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)) {
+            const std::wstring imageName = fileNameOf(imagePath);
+            // Only the programs the payload installs, and only those started from the
+            // directory itself: a folder a user installs into can hold other applications,
+            // each in its own subfolder, and one of those is not an instance of this
+            // payload. Naming it would ask for a close the probe cannot send and an end it
+            // would refuse to perform, so the guard could only ever abort. A program below
+            // the directory that really does hold a payload file is named by
+            // findProcessesUsingFiles, which is the lookup for that question.
+            if (!isPayloadProgram(imageName) || !isSameDirectory(parentDirectoryOf(imagePath), installDir)) {
                 continue;
             }
             Holder holder;
             holder.processId = entry.th32ProcessID;
-            holder.imageName = fileNameOf(imagePath);
+            holder.imageName = imageName;
             holders.push_back(std::move(holder));
         } while (Process32NextW(snapshot, &entry) != FALSE);
     }

tools/installer-holder-probe/restart_manager_holders.h

@@ -2,6 +2,7 @@
 #define RESTART_MANAGER_HOLDERS_H
 
 #include "holder_line.h"
+#include "payload_paths.h"
 
 #include <string>
 #include <vector>
@@ -21,18 +22,6 @@
  */
 std::vector<std::wstring> collectPayloadFiles(const std::wstring &installDir);
 
-/**
- * @brief Whether @p imageName is one of the programs the payload installs.
- *
- * The single place that answers which images are Cockatrice's own, so the guard ends the
- * same set it names. Matched case-insensitively on the image name alone: only holders are
- * ever tested against it, and a holder is a process that has a payload file open or runs
- * from the install directory.
- *
- * @param imageName the file name of a process image, empty when it could not be read.
- */
-bool isPayloadProgram(const std::wstring &imageName);
-
 /**
  * @brief Names the applications and services that have one of @p files open or mapped.
  *
@@ -47,8 +36,7 @@ bool isPayloadProgram(const std::wstring &imageName);
 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.
+ * @brief Lists every payload program whose image lies in @p installDir itself.
  *
  * 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
@@ -57,16 +45,23 @@ unsigned long findProcessesUsingFiles(const std::vector<std::wstring> &files, st
  * 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.
  *
+ * Only the programs the payload installs are named, and only when the image lies directly
+ * in @p installDir. A folder a user installs into can hold other applications, each in its
+ * own subfolder, and those are not instances of this one: they hold none of the payload,
+ * neither closing nor ending them would free a file the purge removes, and naming them
+ * leaves the guard with an answer that can never be carried out. A program below the
+ * directory that does hold a payload file is still named, by findProcessesUsingFiles.
+ *
  * 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.
+ * so a 64-bit instance is named from this 32-bit stub. The comparison is on the directory,
+ * not on an image name alone, so a program that merely shares a name with a payload program
+ * 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 processes
- *         named by askingProcessIds(); empty when the directory is empty or no process
- *         runs from it.
+ * @return one entry per payload program running from it, in snapshot order, without the
+ *         processes named by askingProcessIds(); empty when the directory is empty or no
+ *         payload program runs from it.
  */
 std::vector<Holder> findProcessesRunningFrom(const std::wstring &installDir);