diff --git a/.ci/compile.sh b/.ci/compile.sh index cea837528..bf5a169af 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -559,6 +559,29 @@ Start-Sleep -Seconds 120 } if ($self.Code -ne 0) { throw "expected exit 0 when the asking process is the only holder, got $($self.Code) : $($self.Output)" } if ($self.Output -match "pid\s+$PID\b") { throw "the lookup named the process that started it: $($self.Output)" } + + # A payload executable that refuses to be replaced is running, whether or not this probe + # may name it: the Restart Manager does not report every mapping, and the image path of a + # process under another account cannot be read from here, so both process lookups can + # come back empty while the program is still there. Held here by the asking process, so + # that it cannot be named - the answer has to be "could not name it", never "nothing is + # holding it", which is what sent an update into an uninstall that could only fail + # halfway. Exit 3 is the code the guard answers by asking the user to close it and retry. + $image = Join-Path $dir "oracle.exe" + [IO.File]::WriteAllText($image, "x") + $handle = [IO.File]::Open($image, "Open", "ReadWrite", "None") + try { + $unnameable = Invoke-Probe $dir + } finally { + $handle.Dispose() + } + if ($unnameable.Code -ne 3) { + throw "expected exit 3 for a payload executable that is in use but cannot be named, got $($unnameable.Code) : $($unnameable.Output)" + } + if ($unnameable.Output -notmatch "oracle\.exe is in use") { + throw "the answer did not name the executable it could not replace: $($unnameable.Output)" + } + $empty = Join-Path $dir "empty" New-Item -ItemType Directory -Force $empty | Out-Null $nothing = Invoke-Probe $empty @@ -623,7 +646,7 @@ Start-Sleep -Seconds 120 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" + Write-Host "holder lookup named pid $($holder.Id) for a file another process holds, named nothing for a file only the asking process holds, reported a payload executable it could not name as in use instead of free, 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 d7a6e82c6..bbcc72b18 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -1087,8 +1087,9 @@ FunctionEnd ; $GuardExit to the probe's exit code and $GuardOutput to its one-line answer. ; 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 payload program -; runs from the directory either, "timeout" = it did not return in time). +; directory was given, 3 = no payload program was named but one refuses to be replaced, or +; 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 @@ -1249,25 +1250,27 @@ Function GuardAppsNotRunning Call AbortInstall ${EndIf} ; Not an answer at all: the probe was not configured, the Restart Manager - ; 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. + ; call failed and no payload program runs from the directory, a payload + ; program refuses to be replaced but no lookup could name it (it may run + ; under another account), or the probe 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, and a program the user has closed frees its file), 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" + DetailPrint "Could not confirm whether Cockatrice is still running: $GuardOutput" MessageBox MB_ABORTRETRYIGNORE|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ - "This installer could not check whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nClose them, then choose Retry to check again. Choose Ignore to install anyway, or Abort to cancel the installation." \ + "This installer could not confirm whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nClose them, then choose Retry to check again. Choose Ignore to install anyway, or Abort to cancel the installation." \ IDRETRY guard_check IDABORT guard_unknown_abort !insertmacro CockLog "guard: continuing without an answer from the probe" Return guard_unknown_abort: StrCpy $AbortReason \ - "The installation was cancelled because it could not check whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nPlease close them and run the installer again." + "The installation was cancelled because it could not confirm whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nPlease close them and run the installer again." ; Same reason as the cancel path above: this abort can be reached from the ; leftover-removal guard, after the backup was taken. Call RestoreUserDataAfterUninstall @@ -1868,9 +1871,9 @@ FunctionEnd ; 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. +; is a handle taken in kernel mode, where there is no process to name; a payload executable +; among the files then still refuses to be replaced, and the probe answers with that file +; instead of naming nothing. ; ; $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. @@ -1894,7 +1897,7 @@ Function LogProcessesUsingInstallDir ${ElseIf} $HolderExit == "0" 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" + StrCpy $HolderOutput "the holder probe could not name what holds them: $HolderOutput" ${EndIf} !insertmacro CockLog "holders: $HolderOutput" FunctionEnd @@ -2039,11 +2042,12 @@ Function un.GuardAppsNotRunning ; Same reasoning as in the installer: without an answer from the probe we cannot tell ; whether a running instance still holds file locks, and refusing outright would leave ; the user with an uninstaller that never completes. Same three answers too, so an - ; uninstall that hits a Restart Manager that was busy can simply be retried. + ; uninstall that hits a Restart Manager that was busy, or a payload program the user has + ; not closed yet, can simply be retried. !insertmacro CockLogUn "uninst-guard: the holder probe could not answer ($GuardExit): $GuardOutput" - DetailPrint "Could not check whether Cockatrice is still running: $GuardOutput" + DetailPrint "Could not confirm whether Cockatrice is still running: $GuardOutput" MessageBox MB_ABORTRETRYIGNORE|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ - "This uninstaller could not check whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nClose them, then choose Retry to check again. Choose Ignore to uninstall anyway, or Abort to cancel the uninstallation." \ + "This uninstaller could not confirm whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nClose them, then choose Retry to check again. Choose Ignore to uninstall anyway, or Abort to cancel the uninstallation." \ IDRETRY un_guard_check IDABORT un_guard_unknown_abort !insertmacro CockLogUn "uninst-guard: continuing without an answer from the probe" Return diff --git a/tools/installer-holder-probe/CMakeLists.txt b/tools/installer-holder-probe/CMakeLists.txt index e8ea6df16..732e2879f 100644 --- a/tools/installer-holder-probe/CMakeLists.txt +++ b/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) diff --git a/tools/installer-holder-probe/main.cpp b/tools/installer-holder-probe/main.cpp index f9ca84ac8..31901a0ec 100644 --- a/tools/installer-holder-probe/main.cpp +++ b/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 @@ -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(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 &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 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. diff --git a/tools/installer-holder-probe/payload_locks.cpp b/tools/installer-holder-probe/payload_locks.cpp new file mode 100644 index 000000000..83b77ac58 --- /dev/null +++ b/tools/installer-holder-probe/payload_locks.cpp @@ -0,0 +1,67 @@ +#include "payload_locks.h" + +#include "payload_paths.h" + +#include + +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 payloadExecutablesInUse(const std::wstring &installDir) +{ + std::vector 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 settled; + for (const std::wstring &path : refused) { + if (refusesWrite(path)) { + settled.push_back(path); + } + } + return settled; +} diff --git a/tools/installer-holder-probe/payload_locks.h b/tools/installer-holder-probe/payload_locks.h new file mode 100644 index 000000000..1d23648a2 --- /dev/null +++ b/tools/installer-holder-probe/payload_locks.h @@ -0,0 +1,36 @@ +#ifndef PAYLOAD_LOCKS_H +#define PAYLOAD_LOCKS_H + +#include +#include + +/** + * @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 payloadExecutablesInUse(const std::wstring &installDir); + +#endif diff --git a/tools/installer-holder-probe/payload_paths.cpp b/tools/installer-holder-probe/payload_paths.cpp index 8fac40040..8d6274e91 100644 --- a/tools/installer-holder-probe/payload_paths.cpp +++ b/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 diff --git a/tools/installer-holder-probe/payload_paths.h b/tools/installer-holder-probe/payload_paths.h index 247fc64ed..ea63732f5 100644 --- a/tools/installer-holder-probe/payload_paths.h +++ b/tools/installer-holder-probe/payload_paths.h @@ -3,6 +3,18 @@ #include +/** + * @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. diff --git a/tools/installer-holder-probe/restart_manager_holders.cpp b/tools/installer-holder-probe/restart_manager_holders.cpp index e3fb3e3c8..21ddd6988 100644 --- a/tools/installer-holder-probe/restart_manager_holders.cpp +++ b/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 &files) { if (files.size() >= MAX_PROBED_FILES) { @@ -129,7 +126,12 @@ Holder holderOf(const RM_PROCESS_INFO &info) std::vector collectPayloadFiles(const std::wstring &installDir) { std::vector 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 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) {