mirror of
https://github.com/Cockatrice/Cockatrice.git
synced 2026-11-05 01:23:24 -08:00
[Client] Fix crash when local game commands are sent after shutdown (#7714)
Some checks are pending
CodeQL / Analyze (cpp) (push) Waiting to run
CodeQL / Analyze (actions) (push) Waiting to run
Build Desktop / Configure (push) Waiting to run
Build Desktop / vcpkg baseline (push) Waiting to run
Build Desktop / Debian 13 (push) Blocked by required conditions
Build Desktop / Debian 12 (push) Blocked by required conditions
Build Desktop / Fedora 44 (push) Blocked by required conditions
Build Desktop / Fedora 43 (push) Blocked by required conditions
Build Desktop / Servatrice_Debian 12 (push) Blocked by required conditions
Build Desktop / Ubuntu 24.04 (push) Blocked by required conditions
Build Desktop / Ubuntu 26.04 (push) Blocked by required conditions
Build Desktop / Arch (push) Blocked by required conditions
Build Desktop / Ubuntu 26.04 cover (push) Blocked by required conditions
Build Desktop / macOS 13 Intel (push) Blocked by required conditions
Build Desktop / macOS 14 (push) Blocked by required conditions
Build Desktop / macOS 15 (push) Blocked by required conditions
Build Desktop / macOS 26 Debug (push) Blocked by required conditions
Build Desktop / Windows 10 (push) Blocked by required conditions
Build Desktop / Prune caches (push) Waiting to run
Build Docker / Servatrice (arm) (push) Waiting to run
Build Docker / Servatrice (x86) (push) Waiting to run
Build Docker / Publish multi-platform Servatrice image (push) Blocked by required conditions
Some checks are pending
CodeQL / Analyze (cpp) (push) Waiting to run
CodeQL / Analyze (actions) (push) Waiting to run
Build Desktop / Configure (push) Waiting to run
Build Desktop / vcpkg baseline (push) Waiting to run
Build Desktop / Debian 13 (push) Blocked by required conditions
Build Desktop / Debian 12 (push) Blocked by required conditions
Build Desktop / Fedora 44 (push) Blocked by required conditions
Build Desktop / Fedora 43 (push) Blocked by required conditions
Build Desktop / Servatrice_Debian 12 (push) Blocked by required conditions
Build Desktop / Ubuntu 24.04 (push) Blocked by required conditions
Build Desktop / Ubuntu 26.04 (push) Blocked by required conditions
Build Desktop / Arch (push) Blocked by required conditions
Build Desktop / Ubuntu 26.04 cover (push) Blocked by required conditions
Build Desktop / macOS 13 Intel (push) Blocked by required conditions
Build Desktop / macOS 14 (push) Blocked by required conditions
Build Desktop / macOS 15 (push) Blocked by required conditions
Build Desktop / macOS 26 Debug (push) Blocked by required conditions
Build Desktop / Windows 10 (push) Blocked by required conditions
Build Desktop / Prune caches (push) Waiting to run
Build Docker / Servatrice (arm) (push) Waiting to run
Build Docker / Servatrice (x86) (push) Waiting to run
Build Docker / Publish multi-platform Servatrice image (push) Blocked by required conditions
* [Client] Fix crash when local game commands are sent after shutdown * [Network] Fix formatting in local client debug log * [CI] Keep the vcpkg submodule pointer on the manifest baseline vcpkg.json asks for builtin-baseline 9eec002 (vcpkg 2026.10.16), but the submodule gitlink was still at 9e593bb. The runners check submodules out shallowly, so vcpkg-tool cannot read versions/baseline.json out of the baseline commit and every vcpkg leg (Windows and all macOS) fails in CMake configure with: failed to `git show` versions/baseline.json git phrases that as "path ... exists on disk, but not in <sha>" even when the commit is simply absent from the shallow clone, so it reads like a tooling or network failure rather than a mismatched pointer. Point the submodule back at 9eec002, and add a cheap job that fails when the gitlink and builtin-baseline disagree, before the five build legs pay for it. --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
This commit is contained in:
parent
8b0c980e85
commit
9f30892dff
7 changed files with 208 additions and 9 deletions
|
|
@ -350,6 +350,18 @@ cockatrice_add_test(
|
|||
LABELS
|
||||
ci
|
||||
)
|
||||
# The local game runs one client and one server in the same process.
|
||||
# Either of them can be destroyed first; the survivor has to stay usable.
|
||||
cockatrice_add_test(
|
||||
NAME
|
||||
local_client_lifetime_test
|
||||
SOURCES
|
||||
local_client_lifetime_test.cpp
|
||||
LIBS
|
||||
libcockatrice_network
|
||||
LABELS
|
||||
ci
|
||||
)
|
||||
cockatrice_add_test(
|
||||
NAME
|
||||
server_room_channels_test
|
||||
|
|
|
|||
149
tests/local_client_lifetime_test.cpp
Normal file
149
tests/local_client_lifetime_test.cpp
Normal file
|
|
@ -0,0 +1,149 @@
|
|||
/**
|
||||
* @file local_client_lifetime_test.cpp
|
||||
* @brief Tests that a LocalClient survives the death of the LocalServer it talks to.
|
||||
* @ingroup Tests
|
||||
*/
|
||||
|
||||
#include <QCoreApplication>
|
||||
#include <QDebug>
|
||||
#include <QMessageLogContext>
|
||||
#include <QString>
|
||||
#include <QStringList>
|
||||
#include <cstdio>
|
||||
#include <cstring>
|
||||
#include <gtest/gtest.h>
|
||||
#include <libcockatrice/network/client/local/local_client.h>
|
||||
#include <libcockatrice/network/server/local/local_server.h>
|
||||
#include <libcockatrice/network/server/local/local_server_interface.h>
|
||||
#include <libcockatrice/protocol/pb/commands.pb.h>
|
||||
#include <libcockatrice/rng/rng_abstract.h>
|
||||
#include <memory>
|
||||
|
||||
// The server library refers to this global; the real applications define it in main.cpp.
|
||||
RNG_Abstract *rng = nullptr;
|
||||
|
||||
namespace
|
||||
{
|
||||
|
||||
// GTest's main() creates no QCoreApplication, which QObject::deleteLater() needs.
|
||||
const QCoreApplication &application()
|
||||
{
|
||||
static int argc = 1;
|
||||
static char executable[] = "local_client_lifetime_test";
|
||||
static char *argv[] = {executable, nullptr};
|
||||
static const QCoreApplication app(argc, argv);
|
||||
return app;
|
||||
}
|
||||
|
||||
/// Collects the warnings one logging category emits while this object is alive.
|
||||
///
|
||||
/// The handler is process-global and Qt does not nest handlers, so only one capture can be
|
||||
/// installed at a time; GTest running one case at a time is what makes that safe.
|
||||
class CategoryWarningCapture
|
||||
{
|
||||
public:
|
||||
explicit CategoryWarningCapture(const char *_category) : category(_category)
|
||||
{
|
||||
previousHandler = qInstallMessageHandler(captureMessage);
|
||||
s_active = this;
|
||||
}
|
||||
|
||||
~CategoryWarningCapture()
|
||||
{
|
||||
qInstallMessageHandler(previousHandler);
|
||||
s_active = nullptr;
|
||||
}
|
||||
|
||||
CategoryWarningCapture(const CategoryWarningCapture &) = delete;
|
||||
CategoryWarningCapture &operator=(const CategoryWarningCapture &) = delete;
|
||||
|
||||
const QStringList &warnings() const
|
||||
{
|
||||
return capturedWarnings;
|
||||
}
|
||||
|
||||
private:
|
||||
static void captureMessage(QtMsgType type, const QMessageLogContext &context, const QString &message)
|
||||
{
|
||||
if (s_active != nullptr && type == QtWarningMsg && context.category != nullptr &&
|
||||
std::strcmp(s_active->category, context.category) == 0) {
|
||||
s_active->capturedWarnings.append(message);
|
||||
}
|
||||
|
||||
// Forward everything else, so a failure elsewhere in the test is still visible.
|
||||
if (previousHandler != nullptr) {
|
||||
previousHandler(type, context, message);
|
||||
return;
|
||||
}
|
||||
std::fprintf(stderr, "%s\n", qPrintable(qFormatLogMessage(type, context, message)));
|
||||
}
|
||||
|
||||
const char *category;
|
||||
QStringList capturedWarnings;
|
||||
|
||||
static CategoryWarningCapture *s_active;
|
||||
static QtMessageHandler previousHandler;
|
||||
};
|
||||
|
||||
CategoryWarningCapture *CategoryWarningCapture::s_active = nullptr;
|
||||
QtMessageHandler CategoryWarningCapture::previousHandler = nullptr;
|
||||
|
||||
class LocalClientLifetimeTest : public ::testing::Test
|
||||
{
|
||||
protected:
|
||||
void SetUp() override
|
||||
{
|
||||
(void)application();
|
||||
|
||||
server = std::make_unique<LocalServer>();
|
||||
LocalServerInterface *lsi = server->newConnection();
|
||||
ASSERT_NE(lsi, nullptr);
|
||||
// Building the client logs in and joins room 0 through that interface, exactly as
|
||||
// MainWindow::startLocalGame() does.
|
||||
client = std::make_unique<LocalClient>(lsi, QStringLiteral("lifetime-test-player"),
|
||||
QStringLiteral("lifetime-test-client-id"));
|
||||
}
|
||||
|
||||
void TearDown() override
|
||||
{
|
||||
// Either case may already have deleted one of these.
|
||||
client.reset();
|
||||
server.reset();
|
||||
}
|
||||
|
||||
std::unique_ptr<LocalServer> server;
|
||||
std::unique_ptr<LocalClient> client;
|
||||
};
|
||||
|
||||
TEST_F(LocalClientLifetimeTest, SendAfterServerDeletedIsDropped)
|
||||
{
|
||||
server.reset(); // also destroys the LocalServerInterface the client was built on
|
||||
|
||||
CategoryWarningCapture capture("local_client");
|
||||
// A default container stands in for a command still queued when the local game ended.
|
||||
client->sendCommandContainer(CommandContainer());
|
||||
|
||||
ASSERT_EQ(capture.warnings().size(), 1);
|
||||
EXPECT_TRUE(capture.warnings().first().contains(QStringLiteral("dropping command")))
|
||||
<< capture.warnings().first().toStdString();
|
||||
|
||||
// The surviving client is intact: reading its state must not touch the deleted interface.
|
||||
EXPECT_EQ(client->getUserName(), QStringLiteral("lifetime-test-player"));
|
||||
}
|
||||
|
||||
TEST_F(LocalClientLifetimeTest, DestroyingClientAfterServerIsSafe)
|
||||
{
|
||||
server.reset();
|
||||
|
||||
CategoryWarningCapture capture("local_client");
|
||||
client.reset(); // must neither crash nor touch the destroyed interface
|
||||
|
||||
// The destructor itself must have been silent.
|
||||
EXPECT_TRUE(capture.warnings().isEmpty());
|
||||
|
||||
// A working capture proves the emptiness above is real rather than a broken handler.
|
||||
qCWarning(LocalClientLog) << "local_client_lifetime_test capture self-check";
|
||||
EXPECT_EQ(capture.warnings().size(), 1);
|
||||
}
|
||||
|
||||
} // namespace
|
||||
Loading…
Add table
Add a link
Reference in a new issue