Bound replicated vector size before resize()

[eGurucharri]

Problem

std::vector<T> replication reads a size prefix straight off the network and calls resize() with it before reading a single element — both the generic sp::io::operator>> for std::vector<T> in src/multiplayer/basic.h and the BASIC_REPLICATION_VECTOR macro's Receive case.

A corrupt packet, or a hostile client inside a multiplayer session, can announce a size near UINT32_MAX, forcing a multi-gigabyte allocation before any bounds/data-availability check happens — a memory-exhaustion DoS / crash for whoever processes the packet (client or server).

For contrast, the per-element index in BASIC_REPLICATION_VECTOR's receive loop is already validated against the vector's current size before writing — this is specifically about the unbounded resize() call itself.

Fix

Clamp the announced size to a fixed upper bound (max_replicated_vector_size = 1'000'000) before resizing, logging a warning when a packet exceeds it, in both places that resize from a wire-provided size.

Testing

Built with WARNING_IS_ERROR=1 against a sibling SeriousProton checkout — full clean build succeeds (539/539 targets), no warnings introduced.

I did not add an automated regression test for this — I'm not familiar enough with this codebase's network/replication test setup to know where a targeted test would fit; happy to add one if pointed at the right harness.

[daid]

This smells AI assisted/generated. I don't mind people using AI, but I do ask that if they did that they be up-front about it.

[eGurucharri]

Hello, I'm Eloy. Yes, that was AI generated, as a little project to learn how to use it in our free time we're doing https://github.com/VaroTv7/espaciokooplagunak.

And a very big part of it basically is based on mixing Empty Epsilon and FoundryVTT, as we wanted to make a game that allows us to roleplay DnD while going on a cool spaceship.

Anyway, as we were advancing there were some small fixes the AI/QA tools found (stuff like this one, or the CSS one, or some dependencies), so we thought it wouldn't hurt to open a few PRs upstream.

But of course it's up to you guys if you want to accept them or not, if it's okay we can keep sending those upstream, and if not we'll just tell the AI to not do that.

Best regards!

[VaroTv7]

AI-assisted review from the downstream Espaciokoop Lagunak maintainers, on 7ba791ab1233091a28ffe1d2bee4edc3303eecd9.

The bound is a useful direction, but clamping does not fully close the DoS:

• In the generic operator>>, an oversized prefix is changed to 1,000,000, then resize(1'000'000) still happens and the loop performs one million reads. DataBuffer::read*() returns zero/default values when the buffer is exhausted and exposes no failure state, so a tiny malformed packet still causes the full allocation and O(1,000,000) work.
• In BASIC_REPLICATION_VECTOR, the same prefix still allocates one million elements before the sparse-update loop sees an exhausted/default vector_flags and exits.
• If a sender actually includes more than the capped number of elements, clamping also leaves the remaining encoded elements unread, so subsequent fields in the same buffer can be parsed at the wrong offset.

I would fail closed rather than clamp: detect the oversized declaration before mutating the destination and make the caller discard/reject the current replication payload (or add an error state/result to DataBuffer that the receive path must propagate). A protocol-specific limit smaller than one million may also be appropriate for the concrete beam/shield/missile vectors, but the important property is that an invalid declaration cannot allocate the cap or continue parsing as if it were valid.

A focused regression should feed only an oversized size prefix and assert that the destination remains unchanged, no cap-sized vector is allocated, and the receive path reports/rejects the malformed payload. A second case with a truncated ordinary vector would cover the current silent-underflow behavior.

The existing explicit mapping of both affected resize sites is good; this is about rejection semantics, not the original finding or attribution.

[daid]

Note that I'm fine with people using AI tooling (especially like this, where you just use it as an analyzer, and not to generate large blobs of code). I just like people to be up-front about it.

I do fail to see the importance. As it's just the server that can DoS clients, as vector replication is only done in that direction, I'm not sure how realistic this problem really is. But a little protection in case of version mismatch is miss-interpreting packet data does not hurt.