fix: don't truncate compressed packets that outgrow the output reserve - #757
Open
luizribeiro wants to merge 1 commit into
Open
fix: don't truncate compressed packets that outgrow the output reserve#757luizribeiro wants to merge 1 commit into
luizribeiro wants to merge 1 commit into
Conversation
The existing code assumes a compressed packet is always smaller than the uncompressed one: Compress::compress_into sizes zlib's output buffer at input.len() + 10 bytes and works from there. That isn't true when the data is already compressed. For pack files, encrypted blobs or media, zlib has nothing left to squeeze out, so it wraps the bytes in a stored block and hands back slightly more than it was given. Past 32 KiB the stored-block overhead is more than the 10 bytes of slack. Overflowing is supposed to be recoverable, but the loop grew the buffer only on flate2::Status::BufError, and zlib returns BufError only when it is completely stuck. Filling the buffer while still making progress comes back as Status::Ok, which the loop read as done, so it sent the packet with the tail still inside zlib. The packet went out short and the leftover bytes desynced the stream for every later packet, which OpenSSH reports as channel 0: get data: incomplete message. Ask whether zlib left any room instead of whether it is stuck. Room to spare means it emitted everything; a buffer filled to the brim means there may be more, so grow and call again. compress now delegates to compress_into so the two copies of the loop cannot drift apart again. The regression tests round-trip packets that outgrow the reserve, including through compress_into at a non-zero start_len, the shape the packet writer uses, which the existing compressible under-4096-byte tests don't cover.
luizribeiro
force-pushed
the
fix/compress-truncation
branch
from
August 23, 2026 00:01
dd9cee1 to
4a631b5
Compare
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.
The existing code assumes a compressed packet is always smaller than the uncompressed one:
Compress::compress_intosizes zlib's output buffer atinput.len() + 10bytes and works from there.That isn't true when the data is already compressed. For pack files, encrypted blobs or media, zlib has nothing left to squeeze out, so it wraps the bytes in a stored block and hands back slightly more than it was given. Past 32 KiB the stored-block overhead is more than the 10 bytes of slack, so the buffer is too small.
Overflowing is supposed to be recoverable: the loop grows the buffer and calls zlib again. But it only grows on
flate2::Status::BufError, and zlib returnsBufErroronly when it is completely stuck. Filling the buffer while still making progress comes back asStatus::Ok, which the loop reads as "done", so it sends the packet with the tail still inside zlib. The packet arrives shorter than its header claims, and the bytes left behind are prepended to the next one, so the stream stays broken from there on. OpenSSH reports it aschannel 0: get data: incomplete message: agit cloneagainst a russh server dies as soon as pack data starts flowing, while ordinary traffic is unaffected because it does compress.The fix stops asking zlib whether it is stuck and asks whether it left any room. Room to spare means it emitted everything; a buffer filled to the brim means there may be more, so grow and call again.
compressnow delegates tocompress_intoso the two copies of the loop can't drift apart again.The new tests round-trip packets that outgrow the reserve, including through
compress_intoat a non-zerostart_len, the shape the packet writer uses. The existing tests only cover compressible payloads under 4096 bytes, which always fit. The new ones fail without the fix (payload of 32769 bytes came back as 32735 bytes) and pass with it; the full suite passes. Verified end to end too: a stockgit cloneover a compressed session against a russh-backed git server fails on 0.62.6 with exactly that error and succeeds with this change.