mirror of
https://github.com/Cockatrice/Cockatrice.git
synced 2026-10-28 02:20:24 -05:00
Some checks failed
CodeQL / Analyze (cpp) (push) Has been cancelled
CodeQL / Analyze (actions) (push) Has been cancelled
Build Desktop / Configure (push) Has been cancelled
Build Desktop / Debian 13 (push) Has been cancelled
Build Desktop / Debian 12 (push) Has been cancelled
Build Desktop / Fedora 44 (push) Has been cancelled
Build Desktop / Fedora 43 (push) Has been cancelled
Build Desktop / Servatrice_Debian 12 (push) Has been cancelled
Build Desktop / Ubuntu 24.04 (push) Has been cancelled
Build Desktop / Ubuntu 26.04 (push) Has been cancelled
Build Desktop / Arch (push) Has been cancelled
Build Desktop / Ubuntu 26.04 cover (push) Has been cancelled
Build Desktop / macOS 13 Intel (push) Has been cancelled
Build Desktop / macOS 14 (push) Has been cancelled
Build Desktop / macOS 15 (push) Has been cancelled
Build Desktop / macOS 26 Debug (push) Has been cancelled
Build Desktop / Windows 10 (push) Has been cancelled
Build Docker / Servatrice (arm) (push) Has been cancelled
Build Docker / Servatrice (x86) (push) Has been cancelled
Build Docker / Publish multi-platform Servatrice image (push) Has been cancelled
Command_DumpZone.number_cards is an unbounded sint32 supplied by the client, and
cmdDumpZone used it as an index without ever putting it in range.
for (int i = 0; (i < cards.size()) && (i < numberCards || numberCards == -1); ++i) {
const auto &findId = cmd.is_reversed() ? cards.size() - numberCards + i : i;
Server_Card *card = cards[findId];
A forward request happened to be safe, because the loop stopped at the zone's own size.
A reversed one was not. number_cards is documented by the client as -1 for "the whole
zone", and cards.size() - numberCards is therefore cardCount + 1, one past the last card.
A request larger than the zone puts the offset at a negative index instead. Both read
out of bounds, then dereference the result as a Server_Card, so either adjacent heap
memory or a crash.
Any player can reach it: the zone only has to be a public one, or the requester's own,
and both number_cards and is_reversed come straight off the wire. A reversed dump of -1
is enough on any public zone with cards in it.
Resolve the request to a half-open span up front. -1 and any positive count are clamped
to what the zone actually holds, any other negative count resolves to nothing, and a
reversed request takes the span from the end rather than computing an offset from a value
that may not be in range. cards.at() then carries the bound as a second line of defence,
and qsizetype keeps the loop and the list's own indexing type aligned.
Also report card_count as the zone's real size. It was
numberCards < cards.size() ? cards.size() : numberCards, which agrees with the zone size
for every request that was in range and reported the client's own oversized number
otherwise. The client uses card_count to draw placeholders when a zone lists no cards, so
overstating it invents cards that were never there.
Put the resolution in its own translation unit. server_abstract_player.cpp refers to the
RNG global that only servatrice's main.cpp defines, so anything linking that object needs
the whole server binary behind it, which is why no test could reach this code before.
server_dump_zone_test covers each request shape and then asserts the span is contained in
[0, cardCount) across the edges of the request domain, INT_MIN and INT_MAX included.
Reverting the arithmetic makes three of them fail, the two out-of-bounds shapes and the
containment sweep.
The client's own copy of this convention at view_zone.cpp:93 already clamps the way this
now does. Leaving that duplication for a follow-up, since the client reaching the same
answer by a different route is not a defect.
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
126 lines
4.1 KiB
C++
126 lines
4.1 KiB
C++
/** @file server_dump_zone_test.cpp
|
|
* @brief Tests for resolving a client's zone dump request against a zone's card count.
|
|
* @ingroup Tests
|
|
*/
|
|
|
|
#include <QList>
|
|
#include <climits>
|
|
#include <gtest/gtest.h>
|
|
#include <libcockatrice/network/server/remote/game/dump_zone_range.h>
|
|
#include <vector>
|
|
|
|
namespace
|
|
{
|
|
/// Indices a dump request resolves to, as the handler would walk them.
|
|
std::vector<qsizetype> walked(qsizetype cardCount, int numberCards, bool isReversed)
|
|
{
|
|
const auto [begin, end] = dumpZoneCardRange(cardCount, numberCards, isReversed);
|
|
std::vector<qsizetype> out;
|
|
for (qsizetype i = begin; i < end; ++i) {
|
|
out.push_back(i);
|
|
}
|
|
return out;
|
|
}
|
|
} // namespace
|
|
|
|
TEST(ServerDumpZone, ForwardTakesLeadingCards)
|
|
{
|
|
EXPECT_EQ(walked(5, 2, false), (std::vector<qsizetype>{0, 1}));
|
|
}
|
|
|
|
TEST(ServerDumpZone, ForwardTakingAllCardsCoversTheZone)
|
|
{
|
|
EXPECT_EQ(walked(3, 3, false), (std::vector<qsizetype>{0, 1, 2}));
|
|
}
|
|
|
|
TEST(ServerDumpZone, ForwardAskingForOneDumpsOne)
|
|
{
|
|
EXPECT_EQ(walked(3, 1, false), (std::vector<qsizetype>{0}));
|
|
}
|
|
|
|
TEST(ServerDumpZone, MinusOneMeansTheWholeZone)
|
|
{
|
|
EXPECT_EQ(walked(4, -1, false), (std::vector<qsizetype>{0, 1, 2, 3}));
|
|
}
|
|
|
|
TEST(ServerDumpZone, ReversedTakesTrailingCards)
|
|
{
|
|
EXPECT_EQ(walked(5, 2, true), (std::vector<qsizetype>{3, 4}));
|
|
}
|
|
|
|
TEST(ServerDumpZone, ReversedAskingForOneTakesTheLast)
|
|
{
|
|
EXPECT_EQ(walked(5, 1, true), (std::vector<qsizetype>{4}));
|
|
}
|
|
|
|
TEST(ServerDumpZone, ZeroDumpsNothing)
|
|
{
|
|
EXPECT_TRUE(walked(5, 0, false).empty());
|
|
EXPECT_TRUE(walked(5, 0, true).empty());
|
|
}
|
|
|
|
// A request for more cards than the zone holds is clamped rather than walked off the end.
|
|
// Forward, the old loop stopped at the zone's size on its own.
|
|
TEST(ServerDumpZone, ForwardOversizedRequestIsClamped)
|
|
{
|
|
EXPECT_EQ(walked(3, 1000, false), (std::vector<qsizetype>{0, 1, 2}));
|
|
}
|
|
|
|
// Reversed, an oversized request put the offset at cardCount - numberCards, which is
|
|
// negative, so the walk started before the first card.
|
|
TEST(ServerDumpZone, ReversedOversizedRequestIsClamped)
|
|
{
|
|
EXPECT_EQ(walked(3, 1000, true), (std::vector<qsizetype>{0, 1, 2}));
|
|
}
|
|
|
|
// Reversed, -1 put the offset at cardCount + 1, so the walk started one past the last card.
|
|
TEST(ServerDumpZone, ReversedWholeZoneRequestStaysInRange)
|
|
{
|
|
EXPECT_EQ(walked(3, -1, true), (std::vector<qsizetype>{0, 1, 2}));
|
|
}
|
|
|
|
// Only -1 means "the whole zone". Any other negative count is not a request for anything,
|
|
// so it resolves to an empty range rather than to the whole zone.
|
|
TEST(ServerDumpZone, OtherNegativeCountsDumpNothing)
|
|
{
|
|
EXPECT_TRUE(walked(5, -2, false).empty());
|
|
EXPECT_TRUE(walked(5, -2, true).empty());
|
|
EXPECT_TRUE(walked(5, INT_MIN, true).empty());
|
|
}
|
|
|
|
TEST(ServerDumpZone, EmptyZoneYieldsAnEmptyRange)
|
|
{
|
|
EXPECT_TRUE(walked(0, -1, false).empty());
|
|
EXPECT_TRUE(walked(0, -1, true).empty());
|
|
EXPECT_TRUE(walked(0, 10, false).empty());
|
|
EXPECT_TRUE(walked(0, 10, true).empty());
|
|
}
|
|
|
|
TEST(ServerDumpZone, NonPositiveCardCountYieldsAnEmptyRange)
|
|
{
|
|
EXPECT_TRUE(walked(-1, -1, false).empty());
|
|
EXPECT_TRUE(walked(-1, -1, true).empty());
|
|
}
|
|
|
|
// The invariant the handler depends on: whatever a client asks for, the indices it walks
|
|
// are a subset of the zone's cards and never step outside the list. number_cards is an
|
|
// unbounded client-supplied sint32, so this sweeps the edges rather than a chosen few.
|
|
TEST(ServerDumpZone, ResolvedRangeIsAlwaysWithinTheZone)
|
|
{
|
|
const QList<int> cardCounts = {0, 1, 2, 3, 7, 60, 1000000};
|
|
const QList<int> requests = {INT_MIN, -1000, -2, -1, 0, 1, 2, 3, 59, 60, 61, 999999, INT_MAX};
|
|
|
|
for (int cardCount : cardCounts) {
|
|
for (int request : requests) {
|
|
for (bool isReversed : {false, true}) {
|
|
const auto [begin, end] = dumpZoneCardRange(cardCount, request, isReversed);
|
|
SCOPED_TRACE(testing::Message()
|
|
<< "cardCount=" << cardCount << " request=" << request << " isReversed=" << isReversed);
|
|
ASSERT_GE(begin, 0);
|
|
ASSERT_GE(end, begin);
|
|
ASSERT_LE(end, cardCount);
|
|
}
|
|
}
|
|
}
|
|
}
|