From 0369aaffa973b5820d3b6270a8eceadaf94bc9c5 Mon Sep 17 00:00:00 2001 From: "sentry[bot]" <39604003+sentry[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 13:16:50 +0100 Subject: [PATCH] [Installer] Name only the payload programs installed in the directory (#7684) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .ci/compile.sh | 48 ++++++++++++++-- cmake/NSIS.template.in | 43 +++++++------- tests/CMakeLists.txt | 14 ++++- tests/payload_paths_test.cpp | 57 +++++++++++++++++++ tools/installer-holder-probe/CMakeLists.txt | 4 +- .../installer-holder-probe/close_holders.cpp | 1 + tools/installer-holder-probe/main.cpp | 22 +++---- .../installer-holder-probe/payload_paths.cpp | 52 +++++++++++++++++ tools/installer-holder-probe/payload_paths.h | 39 +++++++++++++ .../restart_manager_holders.cpp | 53 +++++++---------- .../restart_manager_holders.h | 35 +++++------- 11 files changed, 279 insertions(+), 89 deletions(-) create mode 100644 tests/payload_paths_test.cpp create mode 100644 tools/installer-holder-probe/payload_paths.cpp create mode 100644 tools/installer-holder-probe/payload_paths.h diff --git a/.ci/compile.sh b/.ci/compile.sh index 1a30a0fd3..cea837528 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -567,8 +567,9 @@ Start-Sleep -Seconds 120 # 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" + # Windows program, so it starts without its usual directory next to it, and it carries + # one of the payload's own names because that is what the lookup names. + $copy = Join-Path $dir "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 { @@ -580,10 +581,49 @@ Start-Sleep -Seconds 120 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)") { + if ($byImage.Output -notmatch "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 $($holder.Id) for a file another process holds, named nothing for a file only the asking process holds, named nothing for an empty directory, and named pid $($running.Id) for a process running from the directory" + + # A program in a folder below the directory is not an instance of the installation, even + # when it carries a payload program's name: the install directory can be a folder the + # user keeps other applications in, each in its own subfolder, and naming one of those + # asked for a close the probe never sends and an end it refuses to perform, so the guard + # could only abort the update. A program that really does hold a payload file is still + # named, by the Restart Manager lookup exercised above. + $nested = Join-Path $dir "other" + New-Item -ItemType Directory -Force $nested | Out-Null + $nestedImage = Join-Path $nested "cockatrice.exe" + Copy-Item -LiteralPath (Join-Path $env:SystemRoot "System32\cmd.exe") -Destination $nestedImage + $nestedRunning = Start-Process -FilePath $nestedImage -ArgumentList "/c", "ping -n 20 127.0.0.1 >nul" -PassThru + try { + Start-Sleep -Milliseconds 1500 + $inSubfolder = Invoke-Probe $dir + } finally { + try { $nestedRunning.Kill() } catch { } + try { $nestedRunning.WaitForExit(5000) | Out-Null } catch { } + } + if ($inSubfolder.Code -ne 0) { + throw "expected exit 0 for a program in a folder below the directory, got $($inSubfolder.Code) : $($inSubfolder.Output)" + } + + # A program the user keeps in the directory itself is not a payload program either: the + # guard ends only what the payload installs, and naming anything else asks for an answer + # that cannot free a single file. + $foreign = Join-Path $dir "tool.exe" + Copy-Item -LiteralPath (Join-Path $env:SystemRoot "System32\cmd.exe") -Destination $foreign + $foreignRunning = Start-Process -FilePath $foreign -ArgumentList "/c", "ping -n 20 127.0.0.1 >nul" -PassThru + try { + Start-Sleep -Milliseconds 1500 + $ignored = Invoke-Probe $dir + } finally { + try { $foreignRunning.Kill() } catch { } + try { $foreignRunning.WaitForExit(5000) | Out-Null } catch { } + } + if ($ignored.Code -ne 0) { + throw "expected exit 0 for a program the payload does not install, got $($ignored.Code) : $($ignored.Output)" + } + Write-Host "holder lookup named pid $($holder.Id) for a file another process holds, named nothing for a file only the asking process holds, named nothing for an empty directory, named pid $($running.Id) for a payload program running from the directory, and named nothing for a payload program in a folder below it or for a program the payload does not install" } finally { Remove-Item -Recurse -Force $dir -ErrorAction SilentlyContinue } diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index 8d7a82f81..d7a6e82c6 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -998,7 +998,8 @@ FunctionEnd ; repository and embedded in the installer and the uninstaller alike. It names ; every process and service that has one of the payload files open or mapped ; (the Restart Manager, RmRegisterResources/RmGetList) and, separately, every -; process whose image lives in the directory being replaced. +; payload program whose image lies in the directory being replaced rather than +; in a folder below it. ; ; The second lookup is not redundant. Restart Manager answers "who has this ; file open", and a running instance does not have to have one open: its own @@ -1084,10 +1085,10 @@ FunctionEnd ; usage: set $GuardDir to the directory to check, then call this. Sets ; $GuardExit to the probe's exit code and $GuardOutput to its one-line answer. -; 0 = nothing holds the payload and nothing runs from the directory, 1 = the +; 0 = nothing holds the payload and no payload program runs from the directory, 1 = the ; line names what does, anything else = the probe could not answer (2 = no -; directory was given, 3 = the Restart Manager call failed and no process runs -; from the directory either, "timeout" = it did not return in time). +; directory was given, 3 = the Restart Manager call failed and no payload program +; runs from the directory either, "timeout" = it did not return in time). Function ${prefix}RunHolderProbe Call ${prefix}ExtractHolderProbe ; The directory travels in an environment variable, never on the command @@ -1106,7 +1107,7 @@ FunctionEnd ; Cockatrice to close, and waits for it to exit. ; ; The set is what the probe names: the processes holding a file of the version being -; replaced, plus the processes running from the directory. tools\installer-holder-probe +; replaced, plus the payload programs running from the directory itself. The probe ; closes that set by pid, which is why nothing here has to match an executable path. Only ; cockatrice.exe is asked: Oracle writes the card database in place and Servatrice has no ; window to ask, so both are left alone here and only ended by the explicit answer. @@ -1248,14 +1249,15 @@ Function GuardAppsNotRunning Call AbortInstall ${EndIf} ; Not an answer at all: the probe was not configured, the Restart Manager - ; call failed and nothing runs from the directory, or it timed out. Carrying - ; on from that is a decision, so it is asked for rather than assumed - the - ; stale-runtime check after the purge still stands between a failed guard - ; and a mismatched install, but it only fires after files have been - ; replaced, and on an upgrade the launcher asked for nobody is watching the - ; installer's exit code. Retry asks the probe again (a Restart Manager that - ; was busy answers the second time), Ignore installs anyway, Abort cancels - ; instead of writing a payload next to files the guard could not rule out. + ; call failed and no payload program runs from the directory, or it timed + ; out. Carrying on from that is a decision, so it is asked for rather than + ; assumed - the stale-runtime check after the purge still stands between a + ; failed guard and a mismatched install, but it only fires after files have + ; been replaced, and on an upgrade the launcher asked for nobody is watching + ; the installer's exit code. Retry asks the probe again (a Restart Manager + ; that was busy answers the second time), Ignore installs anyway, Abort + ; cancels instead of writing a payload next to files the guard could not + ; rule out. !insertmacro CockLog "guard: the holder probe could not answer ($GuardExit): $GuardOutput" DetailPrint "Could not check whether Cockatrice is still running: $GuardOutput" MessageBox MB_ABORTRETRYIGNORE|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ @@ -1862,12 +1864,13 @@ FunctionEnd ; anything is replaced; here it answers the other question, about files a purge could not ; remove. It asks the Restart Manager (RmRegisterResources/RmGetList), which names a ; process holding a plain handle on a file - an antivirus scanner - and names services as -; well as user programs, and it adds every process running from the directory, because a -; running instance holds its payload as mapped images rather than as open handles and the -; Restart Manager does not reliably report those. Its remaining blind spot is a handle -; taken in kernel mode, where there is no process to name; then the per-file codes logged -; above are the whole story, and the probe says so by naming nothing rather than by -; failing. +; well as user programs, and it adds every payload program running from the directory +; itself - not one in a folder below it, which is another application the user installed +; there - because a running instance holds its payload as mapped images rather than as open +; handles and the Restart Manager does not reliably report those. Its remaining blind spot +; is a handle taken in kernel mode, where there is no process to name; then the per-file +; codes logged above are the whole story, and the probe says so by naming nothing rather +; than by failing. ; ; $INSTDIR travels through the same COCKATRICE_GUARD_DIR environment variable the guard ; uses, so a path with spaces or apostrophes never has to be embedded in a command line. @@ -1889,7 +1892,7 @@ Function LogProcessesUsingInstallDir ${If} $HolderExit == "1" StrCpy $HolderFound 1 ${ElseIf} $HolderExit == "0" - StrCpy $HolderOutput "nothing is holding them and no program is running from the directory" + StrCpy $HolderOutput "nothing is holding them and no payload program is running from the directory" ${Else} StrCpy $HolderOutput "the holder probe could not run: $HolderOutput" ${EndIf} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 62b735a50..1750a448e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -178,8 +178,9 @@ cockatrice_add_test(NAME user_data_backup_test SOURCES user_data_backup_test.cpp cockatrice_add_test(NAME password_hash_test SOURCES password_hash_test.cpp LIBS libcockatrice_utility) cockatrice_add_test(NAME report_categories_test SOURCES report_categories_test.cpp LIBS libcockatrice_utility) cockatrice_add_test(NAME warning_categories_test SOURCES warning_categories_test.cpp LIBS libcockatrice_utility) -# The installer's holder probe is Windows-only, but the wording it produces is plain C++ -# and is what a stuck user has to act on, so it is tested everywhere. +# The installer's holder probe is Windows-only, but the wording it produces and the +# question which programs are the payload's own are plain C++ and are what a stuck user +# has to act on, so they are tested everywhere. cockatrice_add_test( NAME holder_line_test @@ -189,6 +190,15 @@ cockatrice_add_test( INCLUDE_DIRS ${CMAKE_SOURCE_DIR}/tools/installer-holder-probe ) +cockatrice_add_test( + NAME + payload_paths_test + SOURCES + ${CMAKE_SOURCE_DIR}/tools/installer-holder-probe/payload_paths.cpp + payload_paths_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/tools/installer-holder-probe +) cockatrice_add_test( NAME playmat_resolver_test diff --git a/tests/payload_paths_test.cpp b/tests/payload_paths_test.cpp new file mode 100644 index 000000000..c4576d751 --- /dev/null +++ b/tests/payload_paths_test.cpp @@ -0,0 +1,57 @@ +#include "payload_paths.h" + +#include +#include + +namespace +{ +/// The directory part of a path spelled with the same case as the install directory, which +/// is the comparison the probe makes once both sides are folded - see isSameDirectory in +/// restart_manager_holders.cpp. Kept as a plain string here so the shape of a path can be +/// tested without Windows. +bool runsFrom(const std::wstring &imagePath, const std::wstring &installDir) +{ + return parentDirectoryOf(imagePath) == installDir; +} +} // namespace + +TEST(PayloadPaths, SplitsAnImagePathIntoItsDirectoryAndName) +{ + EXPECT_EQ(fileNameOf(L"D:\\Games\\Cockatrice\\cockatrice.exe"), std::wstring(L"cockatrice.exe")); + EXPECT_EQ(parentDirectoryOf(L"D:\\Games\\Cockatrice\\cockatrice.exe"), std::wstring(L"D:\\Games\\Cockatrice")); + EXPECT_EQ(fileNameOf(L"cockatrice.exe"), std::wstring(L"cockatrice.exe")); + EXPECT_EQ(parentDirectoryOf(L"cockatrice.exe"), std::wstring()); + EXPECT_EQ(parentDirectoryOf(L"D:/Games/Cockatrice/cockatrice.exe"), std::wstring(L"D:/Games/Cockatrice")); +} + +TEST(PayloadPaths, OnlyAnImageInTheInstallDirectoryRunsFromIt) +{ + const std::wstring installDir = L"D:\\Games\\Cockatrice"; + EXPECT_TRUE(runsFrom(L"D:\\Games\\Cockatrice\\cockatrice.exe", installDir)); + // A folder the user keeps other applications in is not the installation, and neither is + // a sibling whose name merely begins with the same letters. + EXPECT_FALSE(runsFrom(L"D:\\Games\\Cockatrice\\old\\cockatrice.exe", installDir)); + EXPECT_FALSE(runsFrom(L"D:\\Games\\Cockatrice-old\\cockatrice.exe", installDir)); + EXPECT_FALSE(runsFrom(L"D:\\Games\\cockatrice.exe", installDir)); +} + +TEST(PayloadPaths, KnowsTheProgramsThePayloadInstalls) +{ + EXPECT_TRUE(isPayloadProgram(L"cockatrice.exe")); + EXPECT_TRUE(isPayloadProgram(L"oracle.exe")); + EXPECT_TRUE(isPayloadProgram(L"servatrice.exe")); + EXPECT_TRUE(isPayloadProgram(L"COCKATRICE.EXE")); + EXPECT_TRUE(isPayloadProgram(L"Servatrice.Exe")); +} + +TEST(PayloadPaths, LeavesEveryOtherProgramAlone) +{ + // A program the user keeps in the install directory, the uninstaller, a service host and + // a name that only looks like a payload program are all not instances of the payload. + EXPECT_FALSE(isPayloadProgram(L"uninstall.exe")); + EXPECT_FALSE(isPayloadProgram(L"tool.exe")); + EXPECT_FALSE(isPayloadProgram(L"svchost.exe")); + EXPECT_FALSE(isPayloadProgram(L"cockatrice2.exe")); + EXPECT_FALSE(isPayloadProgram(L"mycockatrice.exe")); + EXPECT_FALSE(isPayloadProgram(L"")); +} diff --git a/tools/installer-holder-probe/CMakeLists.txt b/tools/installer-holder-probe/CMakeLists.txt index e4def8c39..e8ea6df16 100644 --- a/tools/installer-holder-probe/CMakeLists.txt +++ b/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) diff --git a/tools/installer-holder-probe/close_holders.cpp b/tools/installer-holder-probe/close_holders.cpp index 53a1fed8e..7c599bdec 100644 --- a/tools/installer-holder-probe/close_holders.cpp +++ b/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 diff --git a/tools/installer-holder-probe/main.cpp b/tools/installer-holder-probe/main.cpp index d88760b35..f9ca84ac8 100644 --- a/tools/installer-holder-probe/main.cpp +++ b/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; diff --git a/tools/installer-holder-probe/payload_paths.cpp b/tools/installer-holder-probe/payload_paths.cpp new file mode 100644 index 000000000..8fac40040 --- /dev/null +++ b/tools/installer-holder-probe/payload_paths.cpp @@ -0,0 +1,52 @@ +#include "payload_paths.h" + +#include +#include +#include + +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(a - L'A' + L'a') : a; + const wchar_t foldedB = b >= L'A' && b <= L'Z' ? static_cast(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; +} diff --git a/tools/installer-holder-probe/payload_paths.h b/tools/installer-holder-probe/payload_paths.h new file mode 100644 index 000000000..247fc64ed --- /dev/null +++ b/tools/installer-holder-probe/payload_paths.h @@ -0,0 +1,39 @@ +#ifndef PAYLOAD_PATHS_H +#define PAYLOAD_PATHS_H + +#include + +/** + * @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 diff --git a/tools/installer-holder-probe/restart_manager_holders.cpp b/tools/installer-holder-probe/restart_manager_holders.cpp index e8c982f26..e3fb3e3c8 100644 --- a/tools/installer-holder-probe/restart_manager_holders.cpp +++ b/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 &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(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 collectPayloadFiles(const std::wstring &installDir) { std::vector files; @@ -279,7 +261,6 @@ std::vector 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 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); } diff --git a/tools/installer-holder-probe/restart_manager_holders.h b/tools/installer-holder-probe/restart_manager_holders.h index c14499941..e966b70b8 100644 --- a/tools/installer-holder-probe/restart_manager_holders.h +++ b/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 #include @@ -21,18 +22,6 @@ */ std::vector 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 &files, std::vector &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 &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 findProcessesRunningFrom(const std::wstring &installDir);