From ecd955bc609c161cf665e13f6ed7fb2ebba5d102 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:15:29 +0200 Subject: [PATCH 1/5] [Installer] Wait on the launching process so auto-updates cannot write over a mixed Qt runtime (#7372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Installer] Abort instead of installing over leftover runtime files The silent upgrade could leave a mix of old and new Qt DLLs next to the new executables, which is what makes the next start fail with "The procedure entry point X could not be located in the dynamic link library ...Qt6QuickWidgets.dll" or "The code execution cannot proceed because Qt6Multimedia.dll was not found". The check that was supposed to catch this probed only Qt6Core.dll and Qt6Network.dll, so it was blind to the two files the reports name. It also only warned and then installed anyway, so a caught problem was still compounded. - Scan every file the purge targets with FindFirst, which takes the same wildcards the Delete/RMDir calls use, instead of two hardcoded names - Widen IsExistingCockatriceInstall from Qt6Core.dll to Qt6*.dll so a directory left holding only some Qt modules is still recognised as an install - Gate the scan on that, so a first-time install into a directory that happens to hold unrelated Qt5 DLLs is not turned into an abort - Abort with a non-zero exit code instead of warning, so the new payload is never written next to files of the previous version - Skip the message box on an unattended /S install, which would block on it forever, matching the two existing abort sites * [Installer] Cancel a silent upgrade when the running-instance guard fails The running instance guard is what stands between a locked install directory and the mixed Qt DLL set that breaks the next start, but it failed open in two places and let the install carry on anyway: - powershell.exe could not be started: the check warned and returned - the process lookup answered "could not determine": WaitForAppToClose returned on $R0 != 1, so "unknown" was treated exactly like "not running" On a silent upgrade there is nobody at the keyboard to close the applications in response to that warning, which is precisely the state in which the launcher was still holding its Qt DLLs mapped. - Treat "could not determine" as its own state rather than folding it into "not running", in all three places WaitForAppToClose inspects the result - Cancel a silent upgrade in both cases; keep an interactive install on the existing warn-and-continue path, since refusing there would leave an installer that can never complete and the stale-runtime check still applies - Route every abort through one AbortInstall so the dialog-vs-silent decision lives in a single place, and stop the two pre-existing warning dialogs that a /S install without /R would have blocked on forever * [Installer] Wait on the process that launched the update installer The in-app updater starts the installer and only then quits itself, so the installer has to decide whether the launching process is still alive while it is running. It did that by polling powershell.exe for an image name under the install directory, which is a proxy for the real question - is the process that started me still holding its Qt DLLs mapped - and a proxy that can answer "not running" or "could not determine" while the process is very much alive. Either answer let the install carry on into the mixed Qt DLL set. Take /PID= from the command line and wait on that process handle directly: - OpenProcess with PROCESS_SYNCHRONIZE only, which is all that is needed to wait and the least likely to be refused for an unrelated reason - WaitForSingleObject for the same 60 s the name based check allows, so a client slow to flush its card database is not squeezed - Treat an unopenable handle as "already exited", which is what it means for a dead process - Cancel the upgrade if the launcher has not exited, rather than replacing files it is still using Run it before EnsureAppsNotRunning, not after: it needs no powershell.exe, so it is the one check that still works on a machine where the process lookup cannot run, and it is what lets that case keep upgrading instead of being cancelled by what was only ever a proxy for it. Keep an older client, which passes no /PID=, on the existing path. An older installer ignores /PID=, and /D= stays the last parameter either way. * [Client] Pass our process id to the update installer The updater starts the installer and only then quits itself, so the installer begins replacing files while this process still has every Qt runtime DLL mapped, and a mapped DLL cannot be deleted. It has to work out that it must wait, and until now all it had was its own image name to go on. Pass /PID= so it can wait on this process directly instead of guessing, and cancel the update rather than replace files this process is still using. /D= stays the last parameter, which NSIS requires. * [CI] Fail the build when the installer is missing a Qt runtime DLL The Windows payload is assembled by globbing every *.dll out of the build output directories, so nothing but luck decides whether a module the client links statically - Qt6Multimedia.dll, Qt6QuickWidgets.dll - reaches the installer at all. A module that is left out is invisible until a user installs an update and the client then refuses to start, with "The code execution cannot proceed because Qt6Multimedia.dll was not found" or a missing procedure entry point in a Qt6QuickWidgets.dll carried over from the previous version. Derive the required list from SERVATRICE_QT_MODULES, COCKATRICE_QT_MODULES and ORACLE_QT_MODULES, the same module lists the targets are actually built against, so it cannot drift from what the binaries need, and write it next to the CMake cache as required-qt-runtime.txt. Add the platform plugin, without which the client cannot start at all. Assert it in the existing 7-Zip pass that already rejects build-tree artifacts, comparing exact relative paths so a same-named file in the wrong directory does not satisfy a requirement, and normalizing the archive's own separators so it does not matter which one NSIS chose. Missing manifest, missing entry or missing 7-Zip-independent failures all stop the build rather than warn, which is the whole point: the previous check already failed hard for artifacts, and a warning here is what let a broken package ship. * [Client] Point the Windows qt.conf translations path at the real one The Windows qt.conf carried Translations = Resources/translations, copied from the macOS bundle block, but the Windows install has no Resources directory: the .qm files are installed to COCKATRICE_WIN32_QM_INSTALL_DIR, which is translations, and main.cpp looks for them at applicationDirPath() + "/translations". Leave the macOS value alone, which is correct for it. * [Installer] Close the /? help string so the installer compiles again The last line of the continued MessageBox string ended in $\n\" instead of $\n". NSIS reads \" as an escaped quote, so the string never terminated and makensis rejected the script, which took the CPack packaging step down with it. The Windows build failed on "Problem running NSIS command" before ever reaching the new required-runtime check. * [Installer] Give FindFirst the two arguments it takes FindFirst takes the pattern and the output variable and leaves the search handle in $0. There is no handle argument, and FindNext taking only a variable is the giveaway: the handle has to live somewhere implicit, and it is $0. The call passed three arguments, so makensis rejected the script and the CPack packaging step failed with "Problem running NSIS command" before ever reaching the required-runtime check. This was not only a compile error. The handle variable was never populated, so the stale-runtime check that commit 844a1a9b added could never see a match: it probed $StaleFindHandle, which stayed empty, and the widened wildcard scan over the purge set was a no-op. Pass the pattern, test $0, and close $0. No FindNext is needed, because $R0 is a yes/no flag rather than a list. Drops the now unused StaleFindHandle Var. * [CI] Dump the NSIS compile log when packaging fails CPack reports a failed makensis run as "Problem running NSIS command" plus a path to NSISOutput.log, and that log exists only on the runner, so the actual compile error never reaches the workflow output. An installer script that does not compile is therefore undiagnosable from CI alone, which is exactly how a three-argument FindFirst cost two build cycles here. * [CI] Run the NSIS log dump under bash The dump step used the default pwsh shell, where `find` resolves to Windows' text-search find.exe rather than a file finder, so it died with "FIND: Parameter format not correct" instead of printing anything. Pin shell: bash. * [Installer] Test the error flag with LogicLib's ${Errors} condition makensis rejected the script with "macro _If requires 4 parameter(s), passed 2" at the ClearErrors/OpenProcess pair. A ${If} condition cannot be a bare $Var: LogicLib needs either a comparison or one of its flag macros, and a lone $Errors splits into too few arguments for _If. ${Errors} is the documented flag condition and is the same mechanism as the ${Silent} test already used here. The dump step earned its keep - this took one run to locate rather than another round of guessing. * [Installer] Give FindFirst its handle back and stop blocking silent aborts makensis: "FindFirst expects 3 parameters, got 2. Usage: FindFirst $(user_var: handle output) $(user_var: filename output) filespec". The previous commit was wrong about this. FindFirst does take the handle explicitly, and FindNext finds it in $0 as a side effect of the FindFirst that set it, which is why FindNext takes only a variable. The three-argument form the stale-runtime check was written with was correct all along, and the two-argument form broke it. Restored, along with Var StaleFindHandle. That same commit inverted the interactive check in both abort paths. `${If} $ReinstallMode = 1` with `${OrIfNot} ${Silent}` shows the message box exactly when the install is the silent /R upgrade - the one case the comment says must never block, since nothing will dismiss it and the installer hangs until it is killed. Replaced with `${IfNot} ${Silent}`, so the box appears only for an interactive install and a silent abort goes straight to SetErrorLevel. Audited every construct added by this branch against master, which compiles: all 9 LogicLib condition shapes have a master precedent, FindFirst is the only FindFirst and is back to 3 arguments, Function/Section and block depth balance, and the 12 lines with an odd number of quotes are the same pre-existing string continuations master has. * [CI] Strip the CR 7-Zip leaves on its paths before comparing them The installer check reported all 14 required files missing, including Qt6Core-era basics an 82 MB package obviously contains, so the payload was fine and the comparison was broken. 7-Zip writes CRLF on Windows, sed 's/^Path = //p' kept the \r on every line, and grep -qxF matches whole lines - so every entry carried a trailing \r and nothing matched. The build-tree artifact check next to it uses an unanchored grep -E and was unaffected, which is what made the two checks disagree. Reproduced locally against a 7z archive with CRLF injected: the old pipeline matches nothing, the new one finds the present files and reports only the genuinely absent one. Also match case-insensitively. That is how the installed tree behaves - the uninstaller clears both $INSTDIR\plugins and $INSTDIR\Plugins - so a Plugins vs plugins difference in the archive is not a missing file. * [CI] Print both sides of the installer path comparison, and strip its prefixes The check still reported all 14 files missing after the CR fix, so the CR was not the cause and the real difference between 7-Zip's output and the manifest is still unknown. Inferring it again is what produced two wrong fixes in a row, so on failure the check now prints the required list and the first 40 paths 7-Zip lists. A bare "missing" line cannot distinguish a missing file from a misread listing, and twice it was the latter. Also strip $INSTDIR/, $OUTDIR/, $PLUGINSDIR/ and ./ prefixes from the listed paths. Written as separate -e expressions rather than one alternation: with | as the sed delimiter an escaped \| is a literal pipe, so the alternation version silently matched nothing at all. Verified against a 7z archive fed through seven path shapes - plain, Windows backslashes with CRLF, each prefix, lowercase plugins, and all of them combined - each reporting only the file genuinely absent from the archive. * [CI] Strip CR from the manifest side of the installer path check The dump added last run settled it: 7-Zip listed 209 paths including Qt6Gui.dll, Qt6Multimedia.dll and Plugins/platforms/qwindows.dll verbatim, as plain relative paths with no prefix - so the archive side was already clean and the previous fix was right to strip CR there but not sufficient. The trailing CR was on the other side. CMake writes required-qt-runtime.txt with the platform line ending, so on Windows $entry arrived as "Qt6Gui.dll\r", which cannot whole-line match a clean "Qt6Gui.dll". Every entry then read as missing while the listing held it, which is precisely what the two sides of the dump showed. Reproduced locally with a CRLF manifest against a 7z archive: without the strip every entry is reported missing, with it only the genuinely absent file is. Checked across plain paths, Windows backslashes with CRLF, $INSTDIR, $OUTDIR, $PLUGINSDIR, ./ and / prefixes. The literal $INSTDIR-style prefixes are written as [$] bracket expressions so shellcheck does not read them as unexpanded variables; that clears the SC2016 that failed the format job, without a disable directive. * [CI] Reword the bracket-expression comment so the linter stops parsing it A comment whose first word is shellcheck is read as a directive, so "# shellcheck does not read these as unexpanded shell variables" was parsed as a directive named "does" and failed with SC1073 and SC1072 - which cascaded into the SC1009 on the else clause above it. The SC2016 it was meant to document is already gone now that the prefixes are written as [$] bracket expressions, so the note only had to stop looking like a directive. * [CI] Indent the diagnostic path lists without sed, and undo comment drift SC2001 asks for parameter expansion instead of sed 's/^/ /'. Echoing ${manifest//$'\n'/$'\n '} prepends the two spaces directly and keeps the diagnostic block readable. Both occurrences are converted; only the one reading from a here-string was reported, but the piped one has the same shape. Also restore master's wording of the build-tree artifact comment. An earlier edit inserted "in " ("not the install)" became "not in the install)"). It has no functional effect, but it was an unintended change to a line this branch has no reason to touch, and .ci/compile.sh now differs from master only by additions. * [CI] Ship the Qt module runtimes the Windows package check requires The installer check reports Qt6Concurrent.dll and Qt6ShaderTools.dll missing from the Windows package. Both are genuinely absent, not another comparison artifact - the manifest now matches 12 of 14 entries and the dual-list diagnostic shows the other two are not in the archive at all. Nothing deploys them. fixup_bundle copies only the DLLs it finds in Cockatrice.exe's import table, and the *.dll glob only sees what was already copied next to the executable. Qt6::Concurrent is linked but never imported, because QtConcurrent::run and QFutureWatcher are header-only in Qt 6, so the linker drops the import; Qt6::ShaderTools is skipped the same way. The package has shipped without them and the client still starts, but that is luck rather than intent, and required-qt-runtime.txt asks for both. Install the runtime of every linked module explicitly so the installer contains what the check demands. An unused DLL costs a few hundred kilobytes; a missing one is what stops the client starting after an update, and stays invisible until then. Derive both the manifest and the install list from the imported target's TYPE, so a statically linked Qt module - which ships no runtime at all - is skipped by each and the two can never disagree. Scope the manifest to the client rather than the union across server, client and Oracle: only the client is packaged on Windows, and the other two module lists are subsets of the client's, so this changes nothing today. --------- Co-authored-by: Lukas Brübach --- .ci/compile.sh | 56 ++++ .github/workflows/desktop-build.yml | 11 + CMakeLists.txt | 33 ++ cmake/NSIS.template.in | 296 +++++++++++++++--- cockatrice/CMakeLists.txt | 24 +- .../interface/widgets/dialogs/dlg_update.cpp | 10 +- 6 files changed, 378 insertions(+), 52 deletions(-) diff --git a/.ci/compile.sh b/.ci/compile.sh index f20432893..72a017f33 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -352,6 +352,62 @@ if [[ $MAKE_PACKAGE ]]; then exit 1 fi echo "Installer content is clean" + + # Fail the build if the installer is missing a Qt runtime the applications + # link against. The DLLs reach the package by globbing every *.dll out of + # the build output directories, so a module the client links statically + # (Qt6Multimedia.dll, Qt6QuickWidgets.dll) can be left out without anything + # else noticing, and the only symptom is a client that refuses to start + # after an update. required-qt-runtime.txt is written by the top level + # CMakeLists.txt from the same module list the targets are built against. + if [[ ! -f required-qt-runtime.txt ]]; then + echo "::error file=$0::required-qt-runtime.txt not found in the build dir, cannot verify the Qt runtime" + exit 1 + fi + # 7-Zip writes CRLF on Windows, so the \r has to come off before any + # whole-line comparison below. Normalize separators to /, and strip the + # install-root prefixes an NSIS listing may carry, so the entries can be + # compared as exact relative paths. -xF keeps a '.' in a DLL name from + # being a wildcard; -i because that is how the installed tree behaves - + # the uninstaller clears both $INSTDIR\plugins and $INSTDIR\Plugins. + # The archive side and the manifest side have to agree on line endings. + # CMake writes required-qt-runtime.txt with the platform's line ending and + # 7-Zip writes CRLF on Windows, so a trailing \r survives on $entry and + # defeats the whole-line match - every entry then reads as missing even + # though the listing holds it verbatim. + # [$] rather than \$ so the literal dollar is a bracket expression and the + # linter does not read these as unexpanded shell variables (SC2016). + installer_paths="$("$seven_zip" l -slt "$package" | tr -d '\r' | + sed -n 's/^Path = //p' | + sed -e 's|\\|/|g' -e 's|^[$]INSTDIR/||' -e 's|^[$]OUTDIR/||' -e 's|^[$]PLUGINSDIR/||' -e 's|^\./||' -e 's|^/||')" + manifest="$(tr -d '\r' " DESTINATION ./) + endif() + endforeach() + install( DIRECTORY "${CMAKE_BINARY_DIR}/cockatrice/" DESTINATION ./ @@ -729,11 +747,15 @@ if(WIN32) PATTERN "tls/qschannelbackend.dll" ) + # The Windows install has no Resources directory: the .qm files go to + # ${COCKATRICE_WIN32_QM_INSTALL_DIR} and main.cpp looks for them at + # applicationDirPath() + "/translations". The macOS bundle above keeps + # Resources/translations, which is correct for it. install( CODE " file(WRITE \"\${CMAKE_INSTALL_PREFIX}/${qtconf_dest_dir}/qt.conf\" \"[Paths] Plugins = Plugins -Translations = Resources/translations +Translations = translations Data = Resources\") " COMPONENT Runtime diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp index c84a46528..bac270580 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp @@ -265,8 +265,16 @@ void DlgUpdate::downloadSuccessful(const QUrl &filepath) // NSIS needs the /D= argument to be an UNQUOTED string, even if it contains spaces. Qt likes to quote arguments if // they contain spaces, so we use the windows exclusive QProcess::setNativeArguments in the only case where this is // relevant, which preserves the argument unquoted. + // + // /PID= tells the installer which process to wait for before it replaces anything. We launch the installer first + // and only quit afterwards, so it starts while this process is still alive and still has every Qt runtime DLL + // mapped, and a mapped DLL cannot be deleted. Without the handle the installer can only poll for an image name and + // guess, and a wrong guess is what leaves the update writing new executables next to runtime files of the previous + // version, which fails on the next start with "The procedure entry point ... could not be located in the dynamic + // link library + // ...Qt6QuickWidgets.dll". /D= has to stay the last parameter, which is why it is last here too. #ifdef Q_OS_WIN - process.setNativeArguments(QString("/R /D=%1").arg(appDir)); + process.setNativeArguments(QString("/R /PID=%1 /D=%2").arg(QCoreApplication::applicationPid()).arg(appDir)); #else // Linux/macOS: normal argument passing (not relevant since they update differently.) process.setArguments({"/R", QString("/D=%1").arg(appDir)}); From ffcb9094ca5e9b08ec579a764795914e85dfa360 Mon Sep 17 00:00:00 2001 From: RickyRister <42636155+RickyRister@users.noreply.github.com> Date: Fri, 2 Oct 2026 08:17:34 -0700 Subject: [PATCH 2/5] [Replay] Fix cards accumulating in zone view when rewinding (#7357) * [Replay] Fix cards accumulating in zone view when rewinding * rename method --- cockatrice/src/game/zones/view_zone_logic.cpp | 8 ++++++++ cockatrice/src/game/zones/view_zone_logic.h | 4 ++++ cockatrice/src/game_graphics/game_scene.cpp | 10 ++++++++++ cockatrice/src/game_graphics/game_scene.h | 3 +++ cockatrice/src/interface/widgets/tabs/tab_game.cpp | 7 +++++-- cockatrice/src/interface/widgets/tabs/tab_game.h | 2 +- 6 files changed, 31 insertions(+), 3 deletions(-) diff --git a/cockatrice/src/game/zones/view_zone_logic.cpp b/cockatrice/src/game/zones/view_zone_logic.cpp index 2ef04284a..de00581d1 100644 --- a/cockatrice/src/game/zones/view_zone_logic.cpp +++ b/cockatrice/src/game/zones/view_zone_logic.cpp @@ -154,6 +154,14 @@ void ZoneViewZoneLogic::removeCard(int position, bool toNewZone) reorganizeCards(); } +void ZoneViewZoneLogic::clearCards() +{ + for (auto card : cards) { + card->deleteLater(); + } + cards.clear(); +} + void ZoneViewZoneLogic::setWriteableRevealZone(bool _writeableRevealZone) { diff --git a/cockatrice/src/game/zones/view_zone_logic.h b/cockatrice/src/game/zones/view_zone_logic.h index 04ed20e45..e3080d044 100644 --- a/cockatrice/src/game/zones/view_zone_logic.h +++ b/cockatrice/src/game/zones/view_zone_logic.h @@ -41,6 +41,10 @@ public: bool prepareAddCard(int x); void removeCard(int position, bool toNewZone); void updateCardIds(CardAction action); + + /** @brief Removes all cards from the view, without touching anything else. Used in replay rewind */ + void clearCards(); + int getNumberCards() const { return numberCards; diff --git a/cockatrice/src/game_graphics/game_scene.cpp b/cockatrice/src/game_graphics/game_scene.cpp index 17af7618b..e4466f95d 100644 --- a/cockatrice/src/game_graphics/game_scene.cpp +++ b/cockatrice/src/game_graphics/game_scene.cpp @@ -735,6 +735,16 @@ void GameScene::clearViews() } } +void GameScene::emptyActiveViews() +{ + for (auto zoneView : zoneViews) { + auto logic = qobject_cast(zoneView->getZone()->getLogic()); + if (!logic->getRevealZone()) { + logic->clearCards(); + } + } +} + /** * @brief Closes the most recently added zone view. */ diff --git a/cockatrice/src/game_graphics/game_scene.h b/cockatrice/src/game_graphics/game_scene.h index 859d7a6eb..cf81eec9d 100644 --- a/cockatrice/src/game_graphics/game_scene.h +++ b/cockatrice/src/game_graphics/game_scene.h @@ -227,6 +227,9 @@ public slots: /** @brief Closes all zone views. */ void clearViews(); + /** @brief Empties the contents of all non-reveal zone views. Used for replay rewinding */ + void emptyActiveViews(); + /** @brief Closes the most recently added zone view. */ void closeMostRecentZoneView(); QTransform getViewTransform() const; diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.cpp b/cockatrice/src/interface/widgets/tabs/tab_game.cpp index 035ab1004..23976340b 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_game.cpp @@ -259,7 +259,7 @@ void TabGame::linkCardToChat(const QString &cardName) sayEdit->setFocus(); } -void TabGame::resetChatAndPhase() +void TabGame::resetForRewind() { // reset chat log messageLog->clearChat(); @@ -267,6 +267,9 @@ void TabGame::resetChatAndPhase() // reset phase markers game->getGameState()->setCurrentPhase(-1); + // reset view zones + static_cast(gameView->scene())->emptyActiveViews(); + // reset spectator state so the replay can rebuild it from the start game->getPlayerManager()->clearSpectators(); playerListWidget->clearSpectators(); @@ -1270,7 +1273,7 @@ void TabGame::createReplayDock(GameReplay *replay) replayDock->setWidget(replayWidget); replayDock->setFloating(false); - connect(replayWidget, &ReplayWidget::rewound, this, &TabGame::resetChatAndPhase); + connect(replayWidget, &ReplayWidget::rewound, this, &TabGame::resetForRewind); connect(replayWidget, &ReplayWidget::eventReplayed, game->getGameEventHandler(), [this](const auto &event, auto options) { game->getGameEventHandler()->processGameEventContainer(event, nullptr, options); diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.h b/cockatrice/src/interface/widgets/tabs/tab_game.h index a05b49a9f..29a2f40a3 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.h +++ b/cockatrice/src/interface/widgets/tabs/tab_game.h @@ -204,7 +204,7 @@ public: public slots: void viewCardInfo(const CardRef &cardRef = {}) const; - void resetChatAndPhase(); + void resetForRewind(); void updateTimeElapsedLabel(QString newTime); void addPlayerToAutoCompleteList(QString playerName); void removePlayerFromAutoCompleteList(QString playerName); From 750d5b3c211328cd72f8ae843c9946dda819e2e8 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:30:48 +0200 Subject: [PATCH 3/5] [Client] Add option to use game time in game log timestamps (#7265) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Client] Add option to use game time in game log timestamps (#2202) Game log timestamps showed the local clock time, which is meaningless in a replay or in a log read after the fact — nobody can tell when a play happened relative to the game. Add a "Use game time instead of local time in game logs" setting. When enabled, the message log stamps entries with the elapsed game time (HH:MM:SS since the game started); regular chat views keep the local clock. * [Game] Move formatElapsedTime helper out of public slots (#2202) The message log needs the HH:MM:SS formatter used by the game timer, but it lives in GameState's public slots, tying a pure formatting helper to the signal-slot machinery. Move formatElapsedTime into a public section of GameState so the log can call it directly, and add the missing trailing newline to game_state.cpp. --------- Co-authored-by: Lukas Brübach --- cockatrice/src/game/game_state.cpp | 12 ++++++++---- cockatrice/src/game/game_state.h | 8 ++++++++ .../src/game_graphics/log/message_log_widget.cpp | 16 +++++++++++++--- .../src/game_graphics/log/message_log_widget.h | 3 +++ .../widgets/server/chat_view/chat_view.cpp | 9 +++++++-- .../widgets/server/chat_view/chat_view.h | 6 +++--- .../settings_page/messages_settings_page.cpp | 6 ++++++ .../settings_page/messages_settings_page.h | 1 + .../interface_chat_settings_provider.h | 1 + .../libcockatrice/settings/chat_settings.cpp | 10 ++++++++++ .../libcockatrice/settings/chat_settings.h | 2 ++ tests/settings/settings_defaults_test.cpp | 6 ++++++ 12 files changed, 68 insertions(+), 12 deletions(-) diff --git a/cockatrice/src/game/game_state.cpp b/cockatrice/src/game/game_state.cpp index 854878b92..dc9ba02a3 100644 --- a/cockatrice/src/game/game_state.cpp +++ b/cockatrice/src/game/game_state.cpp @@ -29,13 +29,17 @@ void GameState::incrementGameTime() void GameState::setGameTime(int _secondsElapsed) { secondsElapsed = _secondsElapsed; + emit updateTimeElapsedLabel(formatElapsedTime(_secondsElapsed)); +} + +QString GameState::formatElapsedTime(int _secondsElapsed) +{ int seconds = _secondsElapsed; int minutes = seconds / 60; seconds -= minutes * 60; int hours = minutes / 60; minutes -= hours * 60; - emit updateTimeElapsedLabel(QString::number(hours).rightJustified(2, '0') + ":" + - QString::number(minutes).rightJustified(2, '0') + ":" + - QString::number(seconds).rightJustified(2, '0')); -} \ No newline at end of file + return QString::number(hours).rightJustified(2, '0') + ":" + QString::number(minutes).rightJustified(2, '0') + ":" + + QString::number(seconds).rightJustified(2, '0'); +} diff --git a/cockatrice/src/game/game_state.h b/cockatrice/src/game/game_state.h index 5a57d4321..94bbb853c 100644 --- a/cockatrice/src/game/game_state.h +++ b/cockatrice/src/game/game_state.h @@ -110,6 +110,11 @@ public: return hostId; } + int getSecondsElapsed() const + { + return secondsElapsed; + } + signals: void updateTimeElapsedLabel(QString newTime); void gameStarted(bool resuming); @@ -121,6 +126,9 @@ public slots: void incrementGameTime(); void setGameTime(int _secondsElapsed); +public: + static QString formatElapsedTime(int _secondsElapsed); + private: QTimer *gameTimer; int secondsElapsed; diff --git a/cockatrice/src/game_graphics/log/message_log_widget.cpp b/cockatrice/src/game_graphics/log/message_log_widget.cpp index ccd903b04..06b0301bd 100644 --- a/cockatrice/src/game_graphics/log/message_log_widget.cpp +++ b/cockatrice/src/game_graphics/log/message_log_widget.cpp @@ -1,7 +1,9 @@ #include "message_log_widget.h" +#include "../../client/settings/cache_settings.h" #include "../../client/settings/card_counter_settings.h" #include "../../client/sound_engine.h" +#include "../../game/game_state.h" #include "../../game/phase.h" #include "../../game/player/player_logic.h" #include "../../interface/widgets/tabs/tab_game.h" @@ -10,6 +12,7 @@ #include #include +#include #include #include @@ -626,13 +629,12 @@ void MessageLogWidget::logSetActivePhase(int phaseNumber) soundEngine->playSound(phase.soundFileName); - appendHtml("" + QDateTime::currentDateTime().toString("[hh:mm:ss] ") + - phase.getName() + ""); + appendHtml("" + getCurrentTime() + phase.getName() + ""); } void MessageLogWidget::logSetActivePlayer(PlayerLogic *player) { - appendHtml("
" + QDateTime::currentDateTime().toString("[hh:mm:ss] ") + + appendHtml("
" + getCurrentTime() + QString(tr("%1's turn.")).arg(player->getPlayerInfo()->getName()) + "
"); } @@ -826,6 +828,14 @@ void MessageLogWidget::appendHtmlServerMessage(const QString &html, bool optiona ChatView::appendHtmlServerMessage(messagePrefix + html + messageSuffix, optionalIsBold, optionalFontColor); } +QString MessageLogWidget::getCurrentTime() const +{ + if (SettingsCache::instance().chat().getUseGameTime()) { + return "[" + GameState::formatElapsedTime(game->getGameState()->getSecondsElapsed()) + "] "; + } + return ChatView::getCurrentTime(); +} + void MessageLogWidget::connectToPlayerEventHandler(PlayerEventHandler *playerEventHandler) { connect(playerEventHandler, &PlayerEventHandler::logSay, this, &MessageLogWidget::logSay); diff --git a/cockatrice/src/game_graphics/log/message_log_widget.h b/cockatrice/src/game_graphics/log/message_log_widget.h index a145d358d..375c6df4d 100644 --- a/cockatrice/src/game_graphics/log/message_log_widget.h +++ b/cockatrice/src/game_graphics/log/message_log_widget.h @@ -104,6 +104,9 @@ public slots: void appendHtmlServerMessage(const QString &html, bool optionalIsBold = false, QString optionalFontColor = QString()) override; + +private: + [[nodiscard]] QString getCurrentTime() const override; }; #endif diff --git a/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp b/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp index 0a287c9ce..ae9b2deb4 100644 --- a/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp +++ b/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp @@ -167,7 +167,7 @@ void ChatView::appendHtmlServerMessage(const QString &html, bool optionalIsBold, QString htmlText = "" + - QDateTime::currentDateTime().toString("[hh:mm:ss] ") + html + ""; + getCurrentTime() + html + ""; if (optionalIsBold) { htmlText = "" + htmlText + ""; @@ -179,6 +179,11 @@ void ChatView::appendHtmlServerMessage(const QString &html, bool optionalIsBold, } } +QString ChatView::getCurrentTime() const +{ + return QDateTime::currentDateTime().toString("[hh:mm:ss] "); +} + void ChatView::appendCardTag(QTextCursor &cursor, const QString &cardName) { QTextCharFormat oldFormat = cursor.charFormat(); @@ -290,7 +295,7 @@ void ChatView::appendMessage(QString message, timeFormat.setForeground(serverMessageColor); timeFormat.setFontWeight(QFont::Bold); cursor.setCharFormat(timeFormat); - cursor.insertText(QDateTime::currentDateTime().toString("[hh:mm:ss] ")); + cursor.insertText(getCurrentTime()); } // nickname diff --git a/cockatrice/src/interface/widgets/server/chat_view/chat_view.h b/cockatrice/src/interface/widgets/server/chat_view/chat_view.h index 671b35163..06e3a0ed8 100644 --- a/cockatrice/src/interface/widgets/server/chat_view/chat_view.h +++ b/cockatrice/src/interface/widgets/server/chat_view/chat_view.h @@ -107,9 +107,8 @@ public: ChatView(TabSupervisor *_tabSupervisor, AbstractGame *_game, bool _showTimestamps, QWidget *parent = nullptr); void retranslateUi(); void appendHtml(const QString &html); - void virtual appendHtmlServerMessage(const QString &html, - bool optionalIsBold = false, - QString optionalFontColor = QString()); + virtual void + appendHtmlServerMessage(const QString &html, bool optionalIsBold = false, QString optionalFontColor = QString()); void appendMessage(QString message, RoomMessageTypeFlags messageType = {}, const ServerInfo_User &userInfo = {}, @@ -119,6 +118,7 @@ public: QString getRecentChatLog(int maxMessages = 50) const; protected: + [[nodiscard]] virtual QString getCurrentTime() const; void enterEvent(QEnterEvent *event) override; void leaveEvent(QEvent *event) override; void mouseMoveEvent(QMouseEvent *event) override; diff --git a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp index a3b89f8c4..68e3f2ac7 100644 --- a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp @@ -64,6 +64,10 @@ MessagesSettingsPage::MessagesSettingsPage() connect(&ignoreAllPrivateMessagesCheckBox, &QCheckBox::QT_STATE_CHANGED, &SettingsCache::instance().chat(), &ChatSettings::setIgnoreAllPrivateMessages); + useGameTimeCheckBox.setChecked(SettingsCache::instance().chat().getUseGameTime()); + connect(&useGameTimeCheckBox, &QCheckBox::QT_STATE_CHANGED, &SettingsCache::instance().chat(), + &ChatSettings::setUseGameTime); + customAlertString = new QLineEdit(); customAlertString->setText(SettingsCache::instance().chat().getHighlightWords()); connect(customAlertString, &QLineEdit::textChanged, &SettingsCache::instance().chat(), @@ -82,6 +86,7 @@ MessagesSettingsPage::MessagesSettingsPage() chatGrid->addWidget(&mentionPopups, 6, 0); chatGrid->addWidget(&roomHistory, 7, 0); chatGrid->addWidget(&ignoreAllPrivateMessagesCheckBox, 8, 0); + chatGrid->addWidget(&useGameTimeCheckBox, 9, 0); chatGroupBox = new QGroupBox; chatGroupBox->setLayout(chatGrid); @@ -263,6 +268,7 @@ void MessagesSettingsPage::retranslateUi() mentionPopups.setText(tr("Enable desktop notification for mentions")); roomHistory.setText(tr("Enable room message history on join")); ignoreAllPrivateMessagesCheckBox.setText(tr("Ignore all private messages")); + useGameTimeCheckBox.setText(tr("Use game time instead of local time in game logs")); hexLabel.setText(tr("(Color is hexadecimal)")); hexHighlightLabel.setText(tr("(Color is hexadecimal)")); customAlertStringLabel.setText(tr("Separate words with a space, alphanumeric characters only")); diff --git a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h index 436ebbad9..4d491136f 100644 --- a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h @@ -41,6 +41,7 @@ private: QCheckBox mentionPopups; QCheckBox roomHistory; QCheckBox ignoreAllPrivateMessagesCheckBox; + QCheckBox useGameTimeCheckBox; QGroupBox *chatGroupBox; QGroupBox *highlightGroupBox; QGroupBox *messageGroupBox; diff --git a/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h b/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h index cdf2da5eb..04dad9243 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h +++ b/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h @@ -21,6 +21,7 @@ public: [[nodiscard]] virtual bool getShowMentionPopup() const = 0; [[nodiscard]] virtual bool getRoomHistory() const = 0; [[nodiscard]] virtual bool getIgnoreAllPrivateMessages() const = 0; + [[nodiscard]] virtual bool getUseGameTime() const = 0; [[nodiscard]] virtual QString getHighlightWords() const = 0; }; diff --git a/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp b/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp index 974173371..0ee29b4f5 100644 --- a/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp @@ -70,6 +70,11 @@ bool ChatSettings::getIgnoreAllPrivateMessages() const return getValue("ignoreAllPrivateMessages", QString(), QString(), false).toBool(); } +bool ChatSettings::getUseGameTime() const +{ + return getValue("useGameTime", QString(), QString(), false).toBool(); +} + QString ChatSettings::getHighlightWords() const { return getValue("highlightWords").toString(); @@ -141,6 +146,11 @@ void ChatSettings::setIgnoreAllPrivateMessages(bool _ignoreAllPrivateMessages) setValue(_ignoreAllPrivateMessages, "ignoreAllPrivateMessages"); } +void ChatSettings::setUseGameTime(bool _useGameTime) +{ + setValue(_useGameTime, "useGameTime"); +} + void ChatSettings::setHighlightWords(const QString &_highlightWords) { setValue(_highlightWords, "highlightWords"); diff --git a/libcockatrice_settings/libcockatrice/settings/chat_settings.h b/libcockatrice_settings/libcockatrice/settings/chat_settings.h index 9566671e2..d928b23b7 100644 --- a/libcockatrice_settings/libcockatrice/settings/chat_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/chat_settings.h @@ -24,6 +24,7 @@ public: [[nodiscard]] bool getShowMentionPopup() const override; [[nodiscard]] bool getRoomHistory() const override; [[nodiscard]] bool getIgnoreAllPrivateMessages() const override; + [[nodiscard]] bool getUseGameTime() const override; [[nodiscard]] QString getHighlightWords() const override; void setChatMention(bool _chatMention); @@ -39,6 +40,7 @@ public: void setShowMentionPopups(bool _showMentionPopups); void setRoomHistory(bool _roomHistory); void setIgnoreAllPrivateMessages(bool _ignoreAllPrivateMessages); + void setUseGameTime(bool _useGameTime); void setHighlightWords(const QString &_highlightWords); signals: diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index 56555eae6..379f6cd84 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -306,6 +306,12 @@ TEST_F(SettingsDefaultsTest, Chat_IgnoreAllPrivateMessages_Default) ASSERT_EQ(s.getIgnoreAllPrivateMessages(), false); } +TEST_F(SettingsDefaultsTest, Chat_UseGameTime_Default) +{ + ChatSettings s(settingsPath, nullptr); + ASSERT_EQ(s.getUseGameTime(), false); +} + // --- PersonalSettings --- TEST_F(SettingsDefaultsTest, Personal_Lang_Default) From 4ce2e8c3d31e3dbd5241e029befa0c743e2c82ba Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:57:07 +0200 Subject: [PATCH 4/5] [UserList] Fix join/spectate from the profile hover card doing nothing (#7383) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The games list in the profile hover card offered "Join game" and "Spectate", but UserInfoPopup::joinGameRequested was only forwarded to an identical UserListWidget::joinGameRequested signal that nothing connected to. Clicking either entry sent no command, opened no tab and showed no error. Route the request to TabSupervisor, which owns the join flow. The popup's games list spans every room the user is in, so the target room may not be open yet; IntentJoinServerGame already joins the room first and retries, which is why this goes through the intent rather than straight to GameSelector. Extract the intent construction that joinReportGame already had into a shared joinGameByIntent helper instead of duplicating it. Also stop disabling "Join game" for games that are full or already running. GameSelector::joinGame() offers to join as a spectator in that case, so a greyed-out entry read as a dead button; GameSelector's own context menu never disables it either. Co-authored-by: Lukas Brübach --- .../widgets/server/user/user_info_popup.cpp | 5 +++-- .../widgets/server/user/user_list_widget.cpp | 14 ++++++++++--- .../widgets/server/user/user_list_widget.h | 1 - .../interface/widgets/tabs/tab_supervisor.cpp | 21 +++++++++++++++++-- .../interface/widgets/tabs/tab_supervisor.h | 4 ++++ 5 files changed, 37 insertions(+), 8 deletions(-) diff --git a/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp b/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp index 8be76eea0..967de2f2c 100644 --- a/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp +++ b/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp @@ -634,9 +634,10 @@ void UserInfoPopup::onGamesContextMenu(const QPoint &pos) "QMenu::item:selected{background:%4;}") .arg(colorStr(theme.bg), colorStr(theme.text), colorStr(theme.border), colorStr(theme.buttonHover))); - const bool canJoin = !game.started() && game.player_count() < game.max_players(); + // Never grey the join action out: GameSelector::joinGame() asks whether to + // fall back to spectating when the game is full or already running, so a + // disabled entry would just read as a dead button. QAction *join = menu.addAction(tr("Join game")); - join->setEnabled(canJoin); QAction *spec = nullptr; if (game.spectators_allowed()) { diff --git a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp b/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp index d9e1e1e7e..3b8fc08b7 100644 --- a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp +++ b/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp @@ -430,9 +430,6 @@ UserListWidget::UserListWidget(TabSupervisor *_tabSupervisor, hidePopup(true); requestVisibleItemResources(); }); - - // Forward join requests from popup upward - connect(userInfoPopup, &UserInfoPopup::joinGameRequested, this, &UserListWidget::joinGameRequested); } else { // Dialog mode: keyboard selection drives the Invite button. connect(userTree, &QTreeWidget::currentItemChanged, this, [this](QTreeWidgetItem *current, QTreeWidgetItem *) { @@ -692,6 +689,17 @@ void UserListWidget::connectPopupSignals() [this](const QString &n) { userContextMenu->execAdjustJudge(n, true); }); connect(userInfoPopup, &UserInfoPopup::demoteFromJudgeRequested, this, [this](const QString &n) { userContextMenu->execAdjustJudge(n, false); }); + + // Joining is a tab-level action, not a per-user one: the popup's games list + // spans every room the user is in, so the target room may not be open yet. + // Hand it to the supervisor, which owns the intent that joins the room if + // needed. The popup closes here because that intent switches tabs, which + // would otherwise strand it on a list that is no longer on screen. + connect(userInfoPopup, &UserInfoPopup::joinGameRequested, this, + [this](const int gameId, const int roomId, const bool asSpectator) { + hidePopup(true); + tabSupervisor->joinGameFromUserCard(gameId, roomId, asSpectator); + }); } bool UserListWidget::eventFilter(QObject *obj, QEvent *event) diff --git a/cockatrice/src/interface/widgets/server/user/user_list_widget.h b/cockatrice/src/interface/widgets/server/user/user_list_widget.h index 412271160..9802387b2 100644 --- a/cockatrice/src/interface/widgets/server/user/user_list_widget.h +++ b/cockatrice/src/interface/widgets/server/user/user_list_widget.h @@ -172,7 +172,6 @@ signals: void removeBuddy(const QString &userName); void addIgnore(const QString &userName); void removeIgnore(const QString &userName); - void joinGameRequested(int gameId, int roomId, bool asSpectator); void sectionExpanded(Section section, bool expanded); /** Dialog mode: the user activated (Enter/double-click) the given row. */ void userActivated(const QString &userName); diff --git a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp index c9bda7703..0dc246eaa 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp @@ -1100,10 +1100,27 @@ void TabSupervisor::replayLeft(TabGame *tab) } void TabSupervisor::joinReportGame(const int gameId, const int roomId) +{ + joinGameByIntent(gameId, roomId, true); +} + +void TabSupervisor::joinGameFromUserCard(const int gameId, const int roomId, const bool asSpectator) +{ + joinGameByIntent(gameId, roomId, asSpectator); +} + +/** + * Runs the join/spectate intent for @p gameId in @p roomId. + * + * The intent owns the whole flow: if the room tab is not open yet it joins the + * room first, then retries the game, so callers do not have to care whether the + * room happens to be on screen. + */ +void TabSupervisor::joinGameByIntent(const int gameId, const int roomId, const bool asSpectator) { auto *remoteClient = qobject_cast(client); if (!remoteClient) { - actShowPopup(tr("Report joins are only available on a remote server.")); + actShowPopup(tr("Game joins are only available on a remote server.")); return; } @@ -1112,7 +1129,7 @@ void TabSupervisor::joinReportGame(const int gameId, const int roomId) ctx->roomContext.serverContext.port = QString::number(remoteClient->peerPort()); ctx->roomContext.roomId = roomId; ctx->gameId = gameId; - ctx->asSpectator = true; + ctx->asSpectator = asSpectator; auto *joinGameIntent = new IntentJoinServerGame(this, remoteClient, std::move(ctx)); joinGameIntent->setParent(this); diff --git a/cockatrice/src/interface/widgets/tabs/tab_supervisor.h b/cockatrice/src/interface/widgets/tabs/tab_supervisor.h index 066c84a77..1a4f7f749 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_supervisor.h +++ b/cockatrice/src/interface/widgets/tabs/tab_supervisor.h @@ -128,6 +128,8 @@ private: static QString sanitizeTabName(QString dirty); static QString sanitizeHtml(QString dirty); void resetTabsMenu(); + /** Shared join/spectate driver behind the report tab and the profile hover card. */ + void joinGameByIntent(int gameId, int roomId, bool asSpectator); public: explicit TabSupervisor(AbstractClient *_client, QMenu *tabsMenu, QWidget *parent = nullptr); @@ -198,6 +200,8 @@ public slots: TabEdhRec *addEdhrecTab(const CardInfoPtr &cardToQuery, bool isCommander = false); void openReplay(GameReplay *replay); void joinReportGame(int gameId, int roomId); + /** Joins or spectates a game listed on a user's profile hover card. */ + void joinGameFromUserCard(int gameId, int roomId, bool asSpectator); void openTabModeration(const QString &userName = {}); void switchToFirstAvailableNetworkTab(); void maximizeMainWindow(); From add65caae5b0e77997bda67e5a1325f7d57540b9 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Fri, 2 Oct 2026 21:19:02 +0200 Subject: [PATCH 5/5] [Client] Hand off single-instance payloads outside the readyRead signal (#7373) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Client] Hand off single-instance payloads outside the readyRead signal Opening a cockatrice:// link from outside a running client showed the "Join game ...?" confirmation and then crashed on both answers. The join reached the server first, so the other players saw the user join and immediately disconnect. SingleInstanceManager::handleNewConnection() emitted filesReceived from inside the forwarding socket's readyRead lambda and only disconnected afterwards. A joingame link makes the handler show a modal QMessageBox, which runs a nested event loop. The sending instance exits as soon as it has the acknowledgment, so the socket was disconnected and deleteLater'd inside that loop: it died in the middle of its own readyRead signal, and both this lambda and Qt's signal dispatch kept using the freed memory. Disconnect right after the acknowledgment and emit filesReceived through a queued call, so no handler and no nested event loop ever runs inside the socket's signal. Guarding the disconnect call alone is not enough, since Qt still touches the socket after the lambda returns. Add a regression test that reproduces the nested-event-loop hand-off. * [Tests] Apply cmake-format to the single-instance test wiring * [Client] Defer the single-instance socket teardown past the readyRead frame The forwarded payload is emitted synchronously from inside the forwarding socket's readyRead, which is what lets the modal "Join game?" confirmation appear. A handler can spin a nested event loop, and the sending instance exits as soon as it has the ACK, so the socket is disconnected and deleteLater'd while that loop runs. Control then returns to this frame with the socket already destroyed, and the inline disconnectFromServer() touched freed memory. Queue the teardown instead and guard it with a QPointer taken before the handlers run -- building the guard afterwards is itself a use-after-free, since QPointer dereferences the pointer it is constructed from. The payload still reaches the handlers synchronously, so the confirmation is unchanged. The regression test covers both halves: the handlers must run while the socket is still connected, and the socket must not be torn down until the readyRead frame has returned. * Lint. * [Client] Only delete the single-instance socket outside the readyRead frame The auto-delete on `disconnected` was the remaining use-after-free. A handler may spin a nested event loop (the modal "Join game?" box), the sending instance exits as soon as it has the ACK, and so the peer closes while that loop runs. deleteLater() issued from inside such a loop is not deferred to the outer loop: Qt frees the object as soon as the nested loop unwinds, which is still inside this readyRead emission, so Qt's own signal dispatch and the handler lambda both keep using a freed socket. When the notification happens to arrive late the bug hides, which is why this only reproduced on some platforms. Drop the auto-delete and let the queued teardown own the socket's lifetime: it disconnects and then deletes, once the frame has unwound. The QPointer guard was necessary but not sufficient -- it stopped us dereferencing a dead pointer, not the socket being freed underneath the frame. The test waits on the socket's destroyed signal instead of polling, so the teardown assertion does not depend on platform event timing. * [Client] Do not queue the single-instance socket teardown The previous commit moved the deleteLater() off the disconnected signal and into a queued invocation, believing that "queued" meant "after the readyRead frame returns". It does not. A queued call is delivered by whichever event loop is running, and the modal "Join game?" box runs a nested one -- so the teardown was still executed inside the readyRead emission, which is exactly the use-after-free it claimed to prevent. Queueing was never necessary. Because nothing connects deleted/close to the socket's disconnected signal any more, no nested loop can free it while the handlers run, and emit filesReceived() is synchronous: once it returns, the frame has unwound and the socket can be closed and deleted inline. The QPointer stays, covering the one case that remains -- the primary shutting down while the confirmation box is open, since QLocalServer destroys its pending connections. --------- Co-authored-by: Lukas Brübach --- cockatrice/src/single_instance_manager.cpp | 38 +++++- tests/CMakeLists.txt | 16 +++ tests/single_instance_manager_test.cpp | 134 +++++++++++++++++++++ 3 files changed, 182 insertions(+), 6 deletions(-) create mode 100644 tests/single_instance_manager_test.cpp diff --git a/cockatrice/src/single_instance_manager.cpp b/cockatrice/src/single_instance_manager.cpp index eff0e6586..8d363ff34 100644 --- a/cockatrice/src/single_instance_manager.cpp +++ b/cockatrice/src/single_instance_manager.cpp @@ -1,6 +1,7 @@ #include "single_instance_manager.h" #include +#include namespace { @@ -155,19 +156,44 @@ void SingleInstanceManager::handleNewConnection() socket->write(ACK_MESSAGE); socket->flush(); - // Drop the payload from the buffer before handling it: the handlers - // run synchronously and can spin a nested event loop (e.g. a modal - // dialog) that re-reads this socket, which would re-parse and re-emit - // the same files. + // Drop the payload from the buffer before handing it off, so a + // re-entrant event loop cannot re-parse and re-emit the same files. buffer->clear(); *expectedSize = 0; + // Take the guard while the socket is still alive: QPointer has to be + // built from a live pointer, and by the time the handlers below return + // the socket may already be gone. + QPointer guardedSocket(socket); + + // Hand the payload over synchronously, exactly as before, so the + // "Join game?" confirmation still appears. A handler can spin a nested + // event loop (that modal box is one), and the sending instance exits as + // soon as it has the ACK above, so while that loop runs the peer closes + // and this socket is disconnected. emit filesReceived(files); - socket->disconnectFromServer(); + // The handlers run synchronously, so returning from the emit means the + // readyRead frame has unwound and it is safe to touch the socket again. + // This teardown must therefore NOT be queued: a queued call would be + // picked up by the nested loop above while the frame is still on the + // stack, which is precisely the use-after-free this guards against. + // The guard covers the one remaining way the socket can die under us -- + // the primary being torn down (e.g. quit) while the dialog was open -- + // since QLocalServer owns and destroys its pending connections. + if (guardedSocket) { + guardedSocket->disconnectFromServer(); + guardedSocket->deleteLater(); + } return; } }); - connect(socket, &QLocalSocket::disconnected, socket, &QLocalSocket::deleteLater); + // Deliberately no `disconnected -> deleteLater()` here. The handlers above run + // synchronously and may spin a nested event loop, during which the sender exits + // and this socket is disconnected. A deleteLater() triggered from there is not + // deferred to the outer loop: Qt frees the object as soon as that nested loop + // unwinds, which is still inside this readyRead emission, so Qt's own signal + // dispatch is left holding a freed socket. The socket is instead closed and + // deleted once the handlers have returned, on the code path above. } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d80995d26..1f9cdb9a3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -55,6 +55,17 @@ add_executable( ) target_include_directories(loader_local_matching_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) +# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt. +if(WITH_CLIENT) + add_executable( + single_instance_manager_test ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp + single_instance_manager_test.cpp + ) + target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) + + add_test(NAME single_instance_manager_test COMMAND single_instance_manager_test) +endif() + find_package(GTest) if(NOT GTEST_FOUND) @@ -98,6 +109,7 @@ if(NOT GTEST_FOUND) add_dependencies(latency_tracker_test gtest) add_dependencies(metrics_registry_test gtest) add_dependencies(loader_local_matching_test gtest) + add_dependencies(single_instance_manager_test gtest) endif() include_directories(${GTEST_INCLUDE_DIRS}) @@ -153,6 +165,10 @@ target_link_libraries( loader_local_matching_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) +if(WITH_CLIENT) + target_link_libraries(single_instance_manager_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) +endif() + add_subdirectory(card_zone_algorithms) add_subdirectory(carddatabase) add_subdirectory(deck_list_model) diff --git a/tests/single_instance_manager_test.cpp b/tests/single_instance_manager_test.cpp new file mode 100644 index 000000000..49193850d --- /dev/null +++ b/tests/single_instance_manager_test.cpp @@ -0,0 +1,134 @@ +#include "single_instance_manager.h" + +#include +#include +#include +#include +#include +#include +#include + +namespace +{ +constexpr int HANDSHAKE_TIMEOUT_MS = 5000; +constexpr int MODAL_DIALOG_MS = 50; + +const QStringList PAYLOAD{QStringLiteral("cockatrice://joingame?hostname=127.0.0.1&port=4747&roomid=0&gameid=1")}; + +/// Serializes a payload the way SingleInstanceManager::forwardToPrimary() does. +QByteArray encodePayload(const QStringList &files) +{ + QByteArray payload; + QDataStream out(&payload, QIODevice::WriteOnly); + out << files; + + QByteArray message; + QDataStream msgStream(&message, QIODevice::WriteOnly); + msgStream << static_cast(payload.size()); + message.append(payload); + return message; +} +} // namespace + +// Regression test for the use-after-free in SingleInstanceManager::handleNewConnection(). +// +// The forwarded payload is handed to the handlers from inside the forwarding +// socket's readyRead emission, which is what lets the "Join game?" confirmation +// appear at all. A handler can spin a nested event loop (a modal QMessageBox), +// and the sending instance exits as soon as it has the acknowledgment, so the +// peer closes *while that loop runs*. deleteLater() called from inside such a +// nested loop does not wait for the outer loop: Qt frees the object as soon as +// the nested loop unwinds, still inside the readyRead emission, so Qt's own +// signal dispatch and the handler lambda both end up holding a freed socket. +// Nothing may therefore delete the socket from the disconnected signal, and the +// teardown is queued until that frame has unwound. +TEST(SingleInstanceManagerTest, HandlersRunSynchronouslyAndSocketIsNotTouchedAfterTheyReturn) +{ + // Scope the hand-off socket to this test process so a Cockatrice the + // developer happens to be running cannot claim it. + qputenv("USER", QByteArrayLiteral("cockatrice-test-") + QByteArray::number(QCoreApplication::applicationPid())); + + SingleInstanceManager primary; + ASSERT_TRUE(primary.tryRun(QStringList())) << "could not become the primary instance"; + + auto *server = primary.findChild(); + ASSERT_NE(nullptr, server) << "the primary instance is not listening"; + + QLocalSocket sender; + QStringList handled; + QByteArray acknowledgment; + int handlerCalls = 0; + bool socketConnectedWhenHandled = false; + bool socketReleased = false; + + // Waited on after the handlers return: the deferred teardown deletes the + // forwarding socket once the readyRead frame has unwound. + QEventLoop teardownLoop; + + QEventLoop loop; + QObject::connect(&primary, &SingleInstanceManager::filesReceived, &primary, [&](const QStringList &files) { + ++handlerCalls; + + // The payload must reach the handlers while the forwarding socket is + // still connected: deferring the emit would suppress the modal "Join + // game?" confirmation this hand-off exists to show. + auto *connection = server->findChild(); + socketConnectedWhenHandled = connection != nullptr && connection->state() == QLocalSocket::ConnectedState; + + // Observe the socket's destruction rather than polling for it. `destroyed` + // is emitted just before the object goes away, so this stays correct + // however the teardown ends up deleting it, and `&primary` keeps the + // observer alive even once the socket itself is gone. + if (connection != nullptr) { + QObject::connect(connection, &QObject::destroyed, &primary, [&] { + socketReleased = true; + teardownLoop.quit(); + }); + } + + // The primary acknowledges the payload before handling it, so that a + // sender waiting on the acknowledgment never mistakes a busy primary + // for a dead one. + sender.waitForReadyRead(HANDSHAKE_TIMEOUT_MS); + acknowledgment = sender.readAll(); + + // Stand in for the modal "Join game?" confirmation: the sending instance + // goes away while this nested loop is running, which is what used to + // destroy the socket underneath this readyRead frame. + sender.abort(); + + QEventLoop modalDialog; + QTimer::singleShot(MODAL_DIALOG_MS, &modalDialog, &QEventLoop::quit); + modalDialog.exec(); + + handled = files; + loop.quit(); + }); + + sender.connectToServer(server->serverName()); + ASSERT_TRUE(sender.waitForConnected(HANDSHAKE_TIMEOUT_MS)); + sender.write(encodePayload(PAYLOAD)); + sender.flush(); + + QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &loop, &QEventLoop::quit); + loop.exec(); + + EXPECT_EQ(1, handlerCalls) << "the payload must be handed to the handlers exactly once"; + EXPECT_EQ(PAYLOAD, handled); + EXPECT_TRUE(socketConnectedWhenHandled) << "the payload was not handled from inside the readyRead emission"; + EXPECT_FALSE(acknowledgment.isEmpty()) << "the primary never acknowledged the payload"; + + // The deferred teardown must not be skipped, and must not delete the socket + // from inside the nested loop above: either would show up here as a crash or + // as a socket that outlives the hand-off and leaks a connection per payload. + QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &teardownLoop, &QEventLoop::quit); + teardownLoop.exec(); + EXPECT_TRUE(socketReleased) << "the forwarding socket was never released after the hand-off"; +} + +int main(int argc, char **argv) +{ + QCoreApplication app(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}