DeComp(): make the grow step actually double + guard empty input - #500
Draft
IrPgFKS0 wants to merge 1 commit into
Draft
DeComp(): make the grow step actually double + guard empty input#500IrPgFKS0 wants to merge 1 commit into
IrPgFKS0 wants to merge 1 commit into
Conversation
The grow used std::max(size * 2, 30MB), which is almost always 30MB -- every packet that needed one grow allocated the full cap instead of doubling as the adjacent comment intends. Use std::min so it doubles up to the cap (the existing >= cap check still terminates the loop). Guard empty input up front (throw, same error path as other invalid input): required for the capped doubling to be safe -- with std::min a zero-length body (e.g. a bare 4-byte "ABG:" frame) would keep the buffer at 0 forever and spin the loop; it also skips a pointless 30MB allocation + two uncompress calls for input that can never decompress. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The grow step in
DeComp()(src/Common.cpp) doesn't do what its comment says:std::max(size * 2, 30MB)is almost always 30 MB, so every packet that needs one grow allocates the full 30 MB instead of doubling — on every such packet, on the hot receive path. The intended capped doubling isstd::min.Fix
std::min<size_t>(output_buffer.size() * 2, MAX_DECOMPRESSION_BUFFER_SIZE)so it actually doubles up to the cap (the existing>=cap check above still terminates the loop).std::min, a zero-length body (e.g. a bare 4-byteABG:frame from a buggy/malicious peer) would keep the buffer at 0 forever (0 * 2stays 0) and spin the loop — today'sstd::maxis accidentally the only thing preventing that. The guard also skips a pointless 30 MB allocation + twouncompresscalls for input that can never decompress.The two changes are intentionally in one commit because each alone changes the safety story:
minwithout the guard introduces an infinite loop; the guard withoutminleaves the 30 MB-per-grow over-allocation.How this was found
Found while auditing decompression robustness on a LAN fork of BeamMP (the launcher's
DeComphas the unguarded-doubling variant of this bug, submitted separately as BeamMP-Launcher#257); the equivalent fix has been running in that fork's server for weeks of sessions.Transparency
This fix comes from an AI-assisted fork: the bug was found and the patch written with the help of an AI coding tool (Claude), then tested by a human in real multiplayer sessions. Given this project's policy on AI-generated code, it's submitted as a draft for the maintainers to decide — happy to close it if that's not wanted, or for a maintainer to re-implement it independently.
🤖 Generated with Claude Code