Fix excessive CPU use on decompressing small members

[Dreamsorcerer]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.99%. Comparing base (72eaa42) to head (aef7b24).
⚠️ Report is 15 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #13362    +/-   ##
========================================
  Coverage   98.99%   98.99%            
========================================
  Files         132      132            
  Lines       49189    49454   +265     
  Branches     2562     2572    +10     
========================================
+ Hits        48694    48959   +265     
  Misses        371      371            
  Partials      124      124            

Flag	Coverage Δ
Autobahn	22.08% <25.12%> (-0.02%)	⬇️
CI-GHA	98.91% <100.00%> (+<0.01%)	⬆️
OS-Linux	98.68% <100.00%> (+<0.01%)	⬆️
OS-Windows	97.04% <97.46%> (+<0.01%)	⬆️
OS-macOS	97.93% <97.46%> (+<0.01%)	⬆️
Py-3.10	98.13% <100.00%> (+<0.01%)	⬆️
Py-3.11	98.39% <100.00%> (+<0.01%)	⬆️
Py-3.12	98.47% <100.00%> (+<0.01%)	⬆️
Py-3.13	98.46% <100.00%> (+<0.01%)	⬆️
Py-3.14	98.47% <100.00%> (+<0.01%)	⬆️
Py-3.14t	97.56% <97.43%> (+<0.01%)	⬆️
Py-pypy-3.11	97.41% <92.89%> (+0.01%)	⬆️
VM-macos	97.93% <97.46%> (+<0.01%)	⬆️
VM-ubuntu	98.68% <100.00%> (+<0.01%)	⬆️
VM-windows	97.04% <97.46%> (+<0.01%)	⬆️
cython-coverage	38.15% <48.73%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[codspeed-hq]

Merging this PR will not alter performance

✅ 84 untouched benchmarks
⏩ 83 skipped benchmarks¹

---

_{Comparing fix-compr (aef7b24) with master (3f72616)}

Footnotes

1. 83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[Dreamsorcerer]

@aiolibsbot review

[aiolibsbot]

Previous review — superseded by a newer review below.

[Dreamsorcerer]

@aiolibsbot review

[aiolibsbot]

PR Review — Fix excessive CPU use on decompressing small members

Solid, well-verified fix for a real O(n²) decompression bomb. Merge-ready; everything below is non-blocking.

I checked the branch out and exercised it rather than reading the diff alone, and the fix does what it claims:

• Measured: 1024 concatenated 4 KiB gzip members (4 MiB stream) go from 1.02 s on master to 0.033 s on this branch (~31×). The 20 000-empty-member reproducer now raises in 3.7 ms.
• Fuzzed for correctness: ~500 randomised runs across gzip, raw deflate and zstd — varying member counts/sizes (0 B to 200 KiB), feed chunk sizes (1 B to 10 MiB) and max_length (unlimited, 1, 5, 64, 1000, 100 000), draining via data_available — byte-exact against the original data every time, no loop-guard trips.
• The trickiest invariant holds: after a mid-member budget cut, pos = end skips bytes that zlib parked in unconsumed_tail, and decompress_sync re-prepends them on the next call in the right order relative to _pending_unused_data. Same for zstd, where the retained input lives inside the decompressor object instead.
• Replacing the spent decompressor before the budget break (line 217, with the comment explaining why) is the right call — it stops the old object handing the same unused_data back on the next invocation.
• Prior round's max_length double-subtraction is genuinely fixed: budget = max_length - produced against a cumulative produced replaces the old repeated max_length -= len(result), in both the zlib and zstd paths.
• Coverage is broad: window-doubling ramp, single-walk assertion, at-limit / one-over-limit boundary, empty-members-interleaved-with-output, zstd multi-frame, plus end-to-end HttpPayloadParser and WebSocket integration tests. The two new WebSocket tests correctly separate "BFINAL unset stays one member" from "concatenated BFINAL members".

One note on the bot comments: github-code-quality's "Statement has no effect" on aiohttp/compression_utils.py:175/178/181 is a false positive — those are ... bodies of a Protocol, identical in form to the pre-existing ZLibDecompressObjProtocol right above.

Remaining points, all suggestions:

• No regression test reaches the mid-member budget-exhaustion resume path — the one place where correctness silently depends on unconsumed_tail being re-prepended.
• MAX_DECOMPRESS_MEMBERS comment: "a fem members" typo, and it no longer states the real reason for the cap (per-member decompressobj allocation churn, not quadratic copying).
• THREAT_MODEL.md §3.9 / §4.10 / §5.5 and the hardening recap are untouched, though AGENTS.md names size-limit changes as a trigger.
• Changelog does not mention that over-limit payloads are now rejected (ContentEncodingError / WS 1009), which is the user-visible half of the change.
• Pre-existing: DeflateBuffer.feed_data's bare except Exception: re-raise drops the cause, so the new limit message never surfaces on the HTTP path (the WebSocket path already chains with from exc).

---

✅ Resolved since last review (5)

Previously-flagged issues verified fixed

• aiohttp/compression_utils.py:301 Fixed member count may reject legitimate multi-member bodies
• aiohttp/compression_utils.py:42 Cap is per-call, so amplification is bounded at 128x input, not absolutely
• CHANGES/13362.bugfix.rst:1 THREAT_MODEL.md not updated for the new decompression limit
• aiohttp/compression_utils.py:311 [Pre-Existing Issue] max_length budget double-counts prior output across members
• aiohttp/compression_utils.py:305 ValueError message is discarded by DeflateBuffer's exception wrapper

---

🟢 Suggestions

1. No regression test for a budget exhausted mid-member inside the walk
tests/test_compression_utils.py:234-243

The subtlest invariant in _decompress_members has no test pinning it.

When max_length runs out mid-member (not at a member boundary), the walk does pos = end (line 234) even though zlib kept part of remaining[pos:end] in unconsumed_tail. Those bytes are not included in pending. Correctness then depends entirely on ZLibDecompressor.decompress_sync prepending self._decompressor.unconsumed_tail on the next call — and on the same obj being returned and re-bound to self._decompressor. A future refactor that drops the prepend, or hands back a fresh decompressor on that path, silently corrupts the body instead of failing loudly.

No current test reaches it:

• test_zlib_gzip_many_members[capped] — 1024 × 64 bytes = 65536 output vs max_length=262144, so the budget never runs out at all.
• test_zlib_gzip_multi_member_max_length_exhausted_preserves_unused_data (pre-existing) exhausts at a member boundary, where the walk breaks before decompressing and pending covers everything.
• test_zlib_gzip_multi_member_max_length_partial truncates mid-member but the member fits in one window (end == len(remaining)), so nothing is skipped, and it never resumes.

Reaching it needs a member whose wire size exceeds MEMBER_WINDOW_MIN plus a small max_length. I ran this against the branch and it passes — so this is coverage, not a bug:

def test_zlib_gzip_walk_resumes_mid_member() -> None:
    payload = os.urandom(1024)  # incompressible, so wire size > MEMBER_WINDOW_MIN
    blob = gzip.compress(b"A") + gzip.compress(payload)
    d = ZLibDecompressor(encoding="gzip")
    out = d.decompress_sync(blob, max_length=16)
    while d.data_available:
        out += d.decompress_sync(b"", max_length=16)
    assert out == b"A" + payload

@pytest.mark.parametrize("max_length", (0, 262144), ids=("unlimited", "capped"))
def test_zlib_gzip_many_members(max_length: int) -> None:

2. MAX_DECOMPRESS_MEMBERS comment has a typo and omits the actual rationale
aiohttp/compression_utils.py:50-52

Two small things on the new constant's comment.

• Typo: "more than a fem members" → "a few members". codespell in pre-commit will not catch this one.
• The stated reason ("real payloads are unlikely to have many members") is no longer the reason a cap is needed. After the windowing change the walk is linear in input size, so CPU alone no longer justifies rejecting. What still justifies it is per-member cost: each boundary calls fresh(), and a decompressobj allocates a full inflate state (tens of KiB of window buffer). 20 000 empty members in one 400 KiB read means 20 000 inflate inits — allocation churn, not memcpy.

Spelling that out makes the constant reviewable — someone tuning it later needs to know they are trading off allocator pressure, not quadratic copying.

# Cap on concatenated members decoded in one call. Real payloads are unlikely
# to have more than a fem members.
MAX_DECOMPRESS_MEMBERS = 1024

3. THREAT_MODEL.md not updated for the new decode-path limit
aiohttp/compression_utils.py:52

Still open from the previous round. AGENTS.md lists parser size limits and defaults referenced by the document as explicit triggers for revising THREAT_MODEL.md, and this PR adds a new hard limit on a path the document already covers in three places:

• §3.9 / row 3.9 recap (THREAT_MODEL.md:534, :552) — PMCE decompression bomb. WebSocketReader._handle_frame is exactly the call site this PR wraps, and the new MESSAGE_TOO_BIG path belongs in that row.
• §4.10 (:669) — Content-Encoding decompression mitigations, currently listing only max_decompress_size / client_max_size.
• §5.5 compression codecs — the max_length-honouring caveat referenced from §4.10.

A line in each mitigation cell plus an entry in the "Past advisories / hardening (recap)" audit trail (alongside the existing PR #11898 entry) would keep the document accurate — it is the audit trail for precisely this class of hardening.

MAX_DECOMPRESS_MEMBERS = 1024

4. Changelog omits the new rejection behaviour
CHANGES/13362.bugfix.rst:1

The fragment describes the CPU fix but not the behaviour change that comes with it: a payload with more than 1024 concatenated members in a single decompression call is now rejected (ContentEncodingError on HTTP, close code 1009 on WebSocket) rather than decoded slowly.

At 1024 the false-positive class is narrow — you would need a producer emitting one Z_FINISH member per record with members averaging under ~250 bytes to fill a 256 KiB read — but that traffic previously worked and now hard-fails on a generic "Can not decode content-encoding: gzip". A user who hits it has nothing to search for unless the changelog names the limit.

One extra sentence naming the cap would fix that.

Fixed quadratic CPU usage when decompressing payloads built from many small concatenated members/frames (``gzip``, ``deflate`` and ``zstd``) -- by :user:`Dreamsorcerer`.

---

Checklist

• Fix addresses the stated CPU-exhaustion bug (measured 31x on 1024x4KiB members)
• Multi-member output byte-exact across gzip/deflate/zstd under fuzzing
• max_length budget accounting correct across members (prior double-subtraction fixed)
• No infinite-loop or data-loss path in the window walk
• WebSocket (incl. Cython build — reader_c.py is a symlink to reader_py.py) covered by the new catch
• Test coverage for the mid-member budget-exhaustion resume path — suggestion ＃1
• THREAT_MODEL.md revised per AGENTS.md size-limit trigger — suggestion ＃3
• Changelog fragment present, correctly attributed, and describes user-visible behaviour — suggestion ＃4
• Comments accurate and free of typos — suggestion ＃2
• No scope creep beyond the stated fix
• No hardcoded secrets / unsafe deserialization / injection surface

---

Silent Failure Analysis

🟡 **MEDIUM** — error detail erased by caller's catch-all
aiohttp/compression_utils.py:195-205

Risk: The only HTTP-side consumer is http_parser.DeflateBuffer.feed_data's pre-existing except Exception: raise ContentEncodingError("Can not decode content-encoding: %s") — with no from, so the new error type, its message, and its traceback are all discarded and an operator debugging a rejected body cannot tell a member-cap trip from corrupt gzip.

raise TooManyMembersError(
    f"Compressed stream has more than "
    f"{MAX_DECOMPRESS_MEMBERS} members"
)

Fix: Chain the cause (raise ContentEncodingError(...) from exc) or special-case TooManyMembersError in DeflateBuffer.feed_data so the member-limit reason survives into logs.

🟡 **MEDIUM** — silent truncation via state stored on a discarded object
aiohttp/compression_utils.py:212-215

Risk: Undecoded input is parked in self._pending_unused_data and returned output is short with no error, so any caller that does not loop on data_available silently loses the remainder — multipart.py:571-578 (_decode_content) is exactly that: a single-shot decompress_sync(data, max_length=self._max_decompress_size) on a throwaway decompressor (measured: 80000-byte two-member part returns 65536 bytes, pending non-empty, no exception).

if max_length != unlimited:
    budget = max_length - produced
    if budget <= 0:
        pending = bytes(remaining[pos:])
        break

Fix: Pre-existing shape, but since this break path is being rewritten here, either raise when a caller drops pending state or fix _decode_content to drain like _decode_content_async does.

---

Automated review by Kōan (Claude) HEAD=99f99f3 15 min 2s

[patchback]

Backport to 3.14: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.14/d041d4d0fd48c3f0832084d33be16cf1c4835f85/pr-13362

Backported as #13391

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to 3.15: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.15/d041d4d0fd48c3f0832084d33be16cf1c4835f85/pr-13362

Backported as #13392

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.