From 64c780933212c5a753013edaa09800af4a455cc1 Mon Sep 17 00:00:00 2001 From: "sentry[bot]" <39604003+sentry[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 07:49:20 +0100 Subject: [PATCH] [DlgCreateGame] Heap-allocate create game dialog to prevent crash on parent deletion (#7777) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [DlgCreateGame] Heap-allocate create game dialog to prevent crash on parent deletion * [Tests] Run the short-write cases without forking a death test The three cases that force a write failure - CardDatabaseTest.Xml4ShortWriteKeepsInstalledFile, OracleDatabaseWriter.ShortWriteKeepsPreviouslyInstalledDatabase and SpoilerFileTest.ShortWriteLeavesExistingFileIntact - all forked a child, lowered RLIMIT_FSIZE and ignored SIGXFSZ. That child is killed by SIGSEGV on the macOS runners, so all three have been red on every macOS leg of master since they landed, while the same cases pass on Linux. Cap the soft limit in the test process instead: ctest already runs one case per process, and the guard restores the limit and the SIGXFSZ handler, so a whole binary keeps working. Same real EFBIG failure, no fork, and macOS keeps its coverage of the writers. * [CI] Derive DEBUG_TEST and RUN_TESTS in the nightly job The shared steps gate on both, but build-linux-nightly never set them, so its coverage leg skipped "Build debug and test" and then failed collecting a report from an empty build/ directory on every nightly and tag run. continue-on-error kept the run green, so the coverage artifact silently never existed. Derive both exactly as the pull request job does. --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: Lukas Brübach <44814898+BruebachL@users.noreply.github.com> --- .github/workflows/desktop-build.yml | 6 ++ .../widgets/server/game_selector.cpp | 15 ++-- .../src/interface/widgets/tabs/tab_game.cpp | 6 +- tests/carddatabase/carddatabase_test.cpp | 30 ++----- tests/oracle/oracle_importer_test.cpp | 31 +++---- tests/spoiler_file_test.cpp | 32 ++----- tests/support/scoped_file_size_limit.h | 83 +++++++++++++++++++ 7 files changed, 132 insertions(+), 71 deletions(-) create mode 100644 tests/support/scoped_file_size_limit.h diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 4ac28d3a5..5a306e023 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -518,6 +518,12 @@ jobs: CMAKE_GENERATOR: 'Ninja' NAME: ${{ matrix.distro }}${{ matrix.version }} IMAGE_SUFFIX: ${{ matrix.variant }} + # The shared steps below gate on these, so they have to be derived here too. Left + # undefined they silently skipped the build the leg exists for: the coverage leg + # found build/ empty and its report collection failed on every nightly and tag run. + # Same derivation as the pull request job above. + DEBUG_TEST: ${{ matrix.sanitize == 'yes' || matrix.coverage == 'yes' || matrix.debug_test == 'yes' }} + RUN_TESTS: ${{ matrix.test != 'skip' && !(matrix.sanitize == 'yes' || matrix.coverage == 'yes' || matrix.debug_test == 'yes') }} steps: *linux-steps build-vcpkg: diff --git a/cockatrice/src/interface/widgets/server/game_selector.cpp b/cockatrice/src/interface/widgets/server/game_selector.cpp index 2c0050afe..468095adb 100644 --- a/cockatrice/src/interface/widgets/server/game_selector.cpp +++ b/cockatrice/src/interface/widgets/server/game_selector.cpp @@ -266,9 +266,13 @@ void GameSelector::actCreate() return; } - DlgCreateGame dlg(room, room->getGameTypes(), this); - dlg.exec(); - updateTitle(); + // Heap-allocate the dialog: if this selector (or its room tab) is destroyed while the + // dialog is open, e.g. on server disconnect, Qt deletes its children. A stack-allocated + // child would then be deleted by Qt and crash. + auto *dlg = new DlgCreateGame(room, room->getGameTypes(), this); + dlg->setAttribute(Qt::WA_DeleteOnClose); + connect(dlg, &QDialog::finished, this, &GameSelector::updateTitle); + dlg->open(); } void GameSelector::checkResponse(const Response &response) @@ -362,8 +366,9 @@ void GameSelector::customContextMenu(const QPoint &point) const ServerInfo_Game &gameInfo = gameListModel->getGame(index.data(Qt::UserRole).toInt()); const QMap &gameTypes = gameListModel->getGameTypes().value(gameInfo.room_id()); - DlgCreateGame dlg(gameInfo, gameTypes, this); - dlg.exec(); + auto *dlg = new DlgCreateGame(gameInfo, gameTypes, this); + dlg->setAttribute(Qt::WA_DeleteOnClose); + dlg->open(); }); QAction copyLink(tr("Cop&y game link")); diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.cpp b/cockatrice/src/interface/widgets/tabs/tab_game.cpp index ac9edd1ba..01d021907 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_game.cpp @@ -570,8 +570,10 @@ void TabGame::adminLockChanged(bool lock) void TabGame::actGameInfo() { - DlgCreateGame dlg(game->getGameMetaInfo()->proto(), game->getGameMetaInfo()->getRoomGameTypes(), this); - dlg.exec(); + // Heap-allocated so that destroying this tab while the dialog is open cannot delete a stack object. + auto *dlg = new DlgCreateGame(game->getGameMetaInfo()->proto(), game->getGameMetaInfo()->getRoomGameTypes(), this); + dlg->setAttribute(Qt::WA_DeleteOnClose); + dlg->open(); } void TabGame::actCopyGameLink() diff --git a/tests/carddatabase/carddatabase_test.cpp b/tests/carddatabase/carddatabase_test.cpp index b54205f30..350d9be7e 100644 --- a/tests/carddatabase/carddatabase_test.cpp +++ b/tests/carddatabase/carddatabase_test.cpp @@ -1,3 +1,4 @@ +#include "../support/scoped_file_size_limit.h" #include "libcockatrice/card/database/card_database.h" #include "libcockatrice/card/database/card_database_loader.h" #include "libcockatrice/card/database/card_database_querier.h" @@ -36,12 +37,6 @@ #include #include -#ifdef Q_OS_UNIX -#include -#include -#include -#endif - namespace { @@ -247,21 +242,14 @@ TEST(CardDatabaseTest, Xml4ShortWriteKeepsInstalledFile) ASSERT_EQ(installed.write("installed database"), 18); installed.close(); - // A real filesystem write failure, isolated in the death-test child so the - // process-wide file limit never affects other cases or the parent runner. - ASSERT_EXIT( - { - std::signal(SIGXFSZ, SIG_IGN); - struct rlimit limit; - limit.rlim_cur = 128; - limit.rlim_max = 128; - if (setrlimit(RLIMIT_FSIZE, &limit) != 0) { - _exit(2); - } - CockatriceXml4Parser writer(&controller); - _exit(writer.saveToFile({}, sets, cards, fileName) ? 1 : 0); - }, - ::testing::ExitedWithCode(0), ""); + // A real filesystem write failure: the soft file-size cap makes the writer's own + // flush fail without filling a disk. The cap is scoped rather than forked, because a + // death-test child under RLIMIT_FSIZE is killed by SIGSEGV on macOS. + test_support::ScopedFileSizeLimit fileSizeCap(128); + ASSERT_TRUE(fileSizeCap.isValid()); + + CockatriceXml4Parser writer(&controller); + EXPECT_FALSE(writer.saveToFile({}, sets, cards, fileName)); ASSERT_TRUE(installed.open(QIODevice::ReadOnly)); EXPECT_EQ(installed.readAll(), "installed database"); diff --git a/tests/oracle/oracle_importer_test.cpp b/tests/oracle/oracle_importer_test.cpp index 259bae087..1b4e2d915 100644 --- a/tests/oracle/oracle_importer_test.cpp +++ b/tests/oracle/oracle_importer_test.cpp @@ -1,6 +1,7 @@ #include "../../oracle/src/database_writer.h" #include "../../oracle/src/oracleimporter.h" #include "../../oracle/src/raw_json_scanner.h" +#include "../support/scoped_file_size_limit.h" #include "libcockatrice/card/card_info.h" #include "gtest/gtest.h" @@ -33,12 +34,6 @@ #include #include -#ifdef Q_OS_UNIX -#include -#include -#include -#endif - class OracleImporterTest : public ::testing::Test { protected: @@ -1336,20 +1331,16 @@ TEST(OracleDatabaseWriter, ShortWriteKeepsPreviouslyInstalledDatabase) original.close(); const QByteArray data = ""; - // A real filesystem write failure, isolated in the death-test child so the - // process-wide file limit never affects other cases or the parent runner. - ASSERT_EXIT( - { - std::signal(SIGXFSZ, SIG_IGN); - struct rlimit limit; - limit.rlim_cur = 128; - limit.rlim_max = 128; - if (setrlimit(RLIMIT_FSIZE, &limit) != 0) { - _exit(2); - } - _exit(OracleDatabase::saveXml(path, data) ? 1 : 0); - }, - ::testing::ExitedWithCode(0), ""); + // A real filesystem write failure: the soft file-size cap makes the writer's own + // flush fail without filling a disk. The cap is scoped rather than forked, because a + // death-test child under RLIMIT_FSIZE is killed by SIGSEGV on macOS. + test_support::ScopedFileSizeLimit fileSizeCap(128); + ASSERT_TRUE(fileSizeCap.isValid()); + + QString error; + EXPECT_FALSE(OracleDatabase::saveXml(path, data, &error)); + EXPECT_FALSE(error.isEmpty()); + ASSERT_TRUE(original.open(QIODevice::ReadOnly)); EXPECT_EQ(original.readAll(), "installed database"); } diff --git a/tests/spoiler_file_test.cpp b/tests/spoiler_file_test.cpp index 9944f95bd..ecdc3285f 100644 --- a/tests/spoiler_file_test.cpp +++ b/tests/spoiler_file_test.cpp @@ -1,4 +1,5 @@ #include "client/network/update/card_spoiler/spoiler_file.h" +#include "support/scoped_file_size_limit.h" #include #include @@ -6,12 +7,6 @@ #include #include -#ifdef Q_OS_UNIX -#include -#include -#include -#endif - namespace { class SpoilerFileTest : public ::testing::Test @@ -123,25 +118,16 @@ TEST_F(SpoilerFileTest, FileInParentPathReportsFailureWithoutModifyingIt) #ifdef Q_OS_UNIX TEST_F(SpoilerFileTest, ShortWriteLeavesExistingFileIntact) { - // Limit only the child process: a real short write exercises QSaveFile's - // buffered flush failure without depending on disk capacity or file ownership. const QByteArray changed = ""; - EXPECT_EXIT( - { - struct rlimit limit; - if (getrlimit(RLIMIT_FSIZE, &limit) != 0) { - _exit(2); - } - limit.rlim_cur = 128; - if (setrlimit(RLIMIT_FSIZE, &limit) != 0) { - _exit(3); - } - std::signal(SIGXFSZ, SIG_IGN); - const auto result = save(changed); - _exit(result == SpoilerFile::SaveResult::Failed && !error.isEmpty() ? 0 : 1); - }, - ::testing::ExitedWithCode(0), ""); + // A real short write exercises QSaveFile's buffered flush failure without depending + // on disk capacity. The cap is scoped rather than forked, because a death-test child + // under RLIMIT_FSIZE is killed by SIGSEGV on macOS. + test_support::ScopedFileSizeLimit fileSizeCap(128); + ASSERT_TRUE(fileSizeCap.isValid()); + + EXPECT_EQ(save(changed), SpoilerFile::SaveResult::Failed); + EXPECT_FALSE(error.isEmpty()); EXPECT_EQ(read(), original); EXPECT_EQ(QDir(directory.path()).entryList(QDir::Files), QStringList{"spoiler.xml"}); } diff --git a/tests/support/scoped_file_size_limit.h b/tests/support/scoped_file_size_limit.h new file mode 100644 index 000000000..1c1f3d939 --- /dev/null +++ b/tests/support/scoped_file_size_limit.h @@ -0,0 +1,83 @@ +#ifndef COCKATRICE_TEST_SCOPED_FILE_SIZE_LIMIT_H +#define COCKATRICE_TEST_SCOPED_FILE_SIZE_LIMIT_H + +#include + +#ifdef Q_OS_UNIX + +#include +#include +#include + +namespace test_support +{ +/** + * @brief Caps how large a single file may grow, so a writer's short-write path is reachable. + * + * Lowering RLIMIT_FSIZE makes the next write past the cap fail with EFBIG instead of + * filling a filesystem, which is the only way to fail an install step mid-write without + * depending on free disk space or file ownership. Only the soft limit is lowered, so the + * destructor can hand the process back exactly as it found it: ctest runs one case per + * process, but a whole test binary must not carry the cap into its neighbours. + * + * Do not use this inside a gtest death test. A forked child under RLIMIT_FSIZE is killed + * by SIGSEGV on the macOS runners, while the same case passes on Linux, so the fork is + * what breaks - not the writer. The guard alone isolates the failure just as well. + */ +class ScopedFileSizeLimit +{ +public: + explicit ScopedFileSizeLimit(rlim_t maxBytes) + { + if (getrlimit(RLIMIT_FSIZE, &previousLimit) != 0) { + return; + } + + struct sigaction ignore = {}; + ignore.sa_handler = SIG_IGN; + sigemptyset(&ignore.sa_mask); + if (sigaction(SIGXFSZ, &ignore, &previousAction) != 0) { + return; + } + actionSaved = true; + + struct rlimit capped = previousLimit; + capped.rlim_cur = std::min(maxBytes, previousLimit.rlim_max); + if (setrlimit(RLIMIT_FSIZE, &capped) != 0) { + return; + } + active = true; + } + + ~ScopedFileSizeLimit() + { + if (active) { + setrlimit(RLIMIT_FSIZE, &previousLimit); + } + if (actionSaved) { + sigaction(SIGXFSZ, &previousAction, nullptr); + } + } + + ScopedFileSizeLimit(const ScopedFileSizeLimit &) = delete; + ScopedFileSizeLimit &operator=(const ScopedFileSizeLimit &) = delete; + + /** + * @brief Whether the cap is in force; a test that needs the failure must assert this. + */ + [[nodiscard]] bool isValid() const + { + return active; + } + +private: + bool active = false; + bool actionSaved = false; + struct rlimit previousLimit = {}; + struct sigaction previousAction = {}; +}; +} // namespace test_support + +#endif // Q_OS_UNIX + +#endif // COCKATRICE_TEST_SCOPED_FILE_SIZE_LIMIT_H