Fix multipart base64 edge cases

[Dreamsorcerer]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.02%. Comparing base (c207cad) to head (16ffafe).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13509      +/-   ##
==========================================
+ Coverage   99.01%   99.02%   +0.01%     
==========================================
  Files         132      133       +1     
  Lines       49813    49924     +111     
  Branches     2587     2594       +7     
==========================================
+ Hits        49323    49439     +116     
+ Misses        368      366       -2     
+ Partials      122      119       -3     

Flag	Coverage Δ
Autobahn	21.99% <21.37%> (+<0.01%)	⬆️
CI-GHA	98.93% <100.00%> (+0.01%)	⬆️
OS-Linux	98.70% <100.00%> (+0.01%)	⬆️
OS-Windows	97.04% <100.00%> (+0.01%)	⬆️
OS-macOS	97.96% <100.00%> (+0.01%)	⬆️
Py-3.10	98.15% <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.48% <100.00%> (+0.01%)	⬆️
Py-3.14t	97.58% <100.00%> (+0.01%)	⬆️
Py-pypy-3.11	97.43% <100.00%> (+0.01%)	⬆️
VM-macos	97.96% <100.00%> (+0.01%)	⬆️
VM-ubuntu	98.70% <100.00%> (+0.01%)	⬆️
VM-windows	97.04% <100.00%> (+0.01%)	⬆️
cython-coverage	83.02% <32.25%> (-0.03%)	⬇️

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.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported boundary-prefix consumption path was removed because base64 alignment no longer reads supplemental bytes from the underlying stream.

_{Reviews (4): Last reviewed commit: "Fix" | Re-trigger Greptile}

[codspeed-hq]

Merging this PR will not alter performance

✅ 84 untouched benchmarks
🆕 2 new benchmarks
⏩ 83 skipped benchmarks¹

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
🆕	test_read_base64_part	N/A	20.5 ms	N/A
🆕	test_read_chunk_base64_realignment	N/A	7.5 ms	N/A

---

_{Comparing fix-multipart-b64 (16ffafe) with master (2e64b0f)}

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]

@Dreamsorcerer: @aiolibsbot review

Reviewed against PR head 8dc7772, measured in a worktree. Two blockers.

1. _prev_chunk grows unbounded on boundary-delimited parts. _read_chunk_from_stream reads flat size per call; _align_base64_chunk returns only cut and defers rest. Difference accumulates forever. Measured: 16 MiB part -> 3.36 MB buffer, single read_chunk(8192) returned 3.36 MB. Master stays at 8192 for same input. Benign CRLF base64 drifts too: 8276 -> 9590 bytes across 256 KiB -> 4 MiB payloads.

2. cut == 0 fallback regressed working sizes. Revert tests/test_multipart.py:428/:438 from 8 back to 6 — both fail with binascii.Error: Incorrect padding on b'VG\r\r\nl'. Master decodes sizes 5 through 15; this branch fails 5 and 6.

Credit where due: master raises ValueError for every non-multiple-of-4 size on length-delimited base64 parts. This branch fixes all sizes >= 4. Diagnosis correct, bound missing. Details inline.

[aiolibsbot]

Previous review — superseded by a newer review below.

[Dreamsorcerer]

@aiolibsbot review

[aiolibsbot]

Previous review — superseded by a newer review below.

[Dreamsorcerer]

@aiolibsbot review

[aiolibsbot]

PR Review — Fix multipart base64 edge cases

The unbounded-memory finding from last round is genuinely fixed and the underlying bug fix is substantial — one blocking item remains untouched: a confirmed read_chunk(5|6) regression that two pre-existing tests were edited to hide.

Verified on the current head 16ffafe4 in a clean worktree (aiohttp/multipart.py re-read in full, 141 passed / 2 skipped in tests/test_multipart.py, 288 passed across multipart + web-functional, black clean):

Fixed since last round. The one-line change _align_base64_chunk(chunk, size) → _align_base64_chunk(chunk, len(carry) + want) closes warning ＃1 completely. Capping against the inflated read rather than the requested size stops the call from reading more than it hands back. Measured at size == _boundary_len (the worst case I reported last time, 40-char boundary):

payload	prev head max carry	this head max carry
3 KB	190	2
300 KB	19,046	2
3 MB	190,476	2

Flat, and max chunk size + 2. I then tried to break it on purpose — six whitespace-run shapes (AAAAA + 8000 spaces repeated, AAAAA + 8190 spaces, a 1 MiB single run) across sizes 42 / 43 / 8192 — and every carry stayed under size, every chunk under size + boundary_len.

The fix is worth more than the changelog says. A fuzz over 3 boundary lengths x 9 payload sizes x 4 wrapping shapes x 7 chunk sizes x both length/boundary paths: 141 failures on master, 0 on this branch. And on master a 1 MiB insignificant-byte run costs 11.3 s of CPU before raising, growing quadratically; here it is 0.01 s.

Pushing back on greptile's P1 (aiohttp/multipart.py:471, partial boundary prefix consumption): resolved by construction. want = max(want, self._boundary_len) means _read_chunk_from_stream can no longer be asked for fewer than _boundary_len bytes, so the one- or two-byte supplemental read that comment describes cannot occur.

Test quality is good. test_read_chunk_base64_bounded_by_requested_size and ..._length_delimited_carries_quartet both guard against going quiet (carried > len(chunks) // 2, carried > 10), and ..._padding_run_does_not_amplify states outright what it does not prove. That is the right answer to last round's objection.

• 🟡 multipart.py:413 — the cut == 0 fallback still returns an unaligned chunk. On b"VG\r\r\nltZSB0byBSZ\r\nWxheCE=", sizes 5 and 6 raise binascii.Error here and decode fine on master. tests/test_multipart.py:428/:438 were switched from size=6 to size=8; reverting that one number fails both on this branch. Reach is narrow (needs a very short boundary), but the contract narrowing is currently recorded nowhere — fix it, or restore the sizes in a test that asserts the new failure and name the minimum in the changelog.
• 🟢 THREAT_MODEL.md:668 — mitigation 4.9 still describes the deleted "extends each chunk" loop, and the CPU-amplification win above is not recorded anywhere.
• 🟢 docs/multipart_reference.rst:49 — size is now a cap that a chunk may exceed when a carry is pending; the reference and the changelog fragment both still describe the old contract.
• 🟢 tests/test_benchmarks_multipart.py:61 — same stale wording, and the body takes the cut == 0 return so the carry path the name promises is never measured. (Correcting myself: the walk-back loop is exercised, so this is not the no-op I called it last round.)

---

🟡 Important

1. `cut == 0` fallback still regresses `read_chunk(5|6)` vs master, and two pre-existing tests were edited to absorb it
aiohttp/multipart.py:413-417

This is the one finding from the last round that the new commit (16ffafe4) did not touch, so I re-verified it on the current head.

What's wrong. When a size-byte window holds fewer than 4 significant characters and starts on a significant byte, cut walks back to 0 and the chunk is handed back unaligned. A caller following the documented read-chunk-then-decode loop then gets a binascii.Error.

Reproduced on 16ffafe4 with the unchanged body b"VG\r\r\nltZSB0byBSZ\r\nWxheCE=\r\n--:--", sweeping size 4..15:

• master: 5..15 all decode to b"Time to Relax!".
• this branch: 5 and 6 raise binascii.Error: Incorrect padding; 7+ work.

Chunk-by-chunk at size=6 the branch returns b'VG\r\r\nl' (3 significant chars, carry=b'') where master returns b'VG\r\r\nlt'.

Why it matters. tests/test_multipart.py:428 and :438 change read_chunk(size=6) to read_chunk(size=8). I reverted just that one number on this head and both pre-existing tests fail; they pass on master. So the diff currently absorbs a read_chunk contract narrowing rather than fixing or recording it, and nothing in the changelog or docs tells a user their working size may stop working.

The new test_read_chunk_base64_small_size_carry_drains parametrises over exactly the affected sizes (5, 6, 7, 8), but with a well-formed base64.encodebytes body that never reaches this branch — so it reads like coverage of the small-size case while leaving the regression uncovered.

Reach, stated fairly (correcting my earlier over-claim): size >= _boundary_len, so a realistic 40+ character boundary makes this essentially unreachable for normally-wrapped base64. A 12-shape x 7-size x both-paths fuzz over 3 boundary lengths found zero mismatches on this head. The confirmed regression is confined to very short boundaries. On the whitespace-padded shapes that hit cut == 0 with a real boundary, master fails too — and much worse (see summary).

Either direction resolves this:

• Complete the quartet with one bounded extra read so size=5/6 keep working, and restore size=6 in both tests; or
• Keep the behaviour deliberately: restore the original sizes in a separate test that asserts the new failure, and name the new practical minimum in the changelog fragment.

        if not cut:
            # No whole quartet to hand back, and carrying the lot would make
            # no progress: the caller asked for this many bytes, and a part
            # that holds no quartet within them holds none to give.
            return chunk

🟢 Suggestions

2. `read_chunk` semantics changed for base64 parts without a docs or changelog update
docs/multipart_reference.rst:49-56

For base64 parts size changes meaning here: it becomes an upper bound rather than a target, and a chunk may still exceed it when a carry is pending.

The PR's own tests acknowledge both halves of this — assert all(len(c) <= 64 for c in chunks[:-1]) excludes the last chunk, and test_read_chunk_base64_small_size_carry_drains asserts len(obj._b64_carry) <= size + obj._boundary_len because the want = max(want, self._boundary_len) floor lets a call hand back more than size. I measured max chunk size + 2 at size=42 with a 40-char boundary, and size + boundary_len on whitespace-heavy shapes.

AGENTS.md ("Documentation & code style") asks for a docs/ update on user-visible API changes; the reference entry still says only "Reads body part content chunk of the specified size." Two sentences on the base64 alignment behaviour and the over-size exception would close the gap.

The changelog fragment is also very terse for what this actually delivers. "Fixed some edge case handling" undersells it — on master a length-delimited base64 part raises ValueError("Reader did not read all the data or it is malformed") for essentially every non-multiple-of-4 size; my fuzz counted 141 failing (payload, wrapping, size, path) combinations on master and 0 on this branch. Naming that concretely tells users which previously-broken calls now work.

   .. method:: read_chunk(size=chunk_size)
      :async:

      Reads body part content chunk of the specified size.

      :param int size: chunk size

---

Checklist

• Bounded memory on attacker-controlled input (was warning ＃1)
• No quadratic CPU on insignificant-byte runs
• No behavioural regression vs master — warning ＃1
• Pre-existing tests not weakened to fit the change — warning ＃1
• New behaviour covered by meaningful tests
• Length-delimited base64 works for all sizes >= boundary_len (broken on master)
• No over-read past Content-Length into following parts
• Full multipart + web-functional suites pass (288 passed, 14 skipped)
• Changelog fragment present and correctly attributed
• Docs updated for user-visible API change — suggestion ＃2
• THREAT_MODEL.md kept in sync per AGENTS.md
• Benchmarks follow repo conventions and measure what they name
• No hardcoded secrets or unsafe deserialization
• No resource leaks or infinite-loop risk (read_chunk never returns empty while not at_eof)
• black/isort/flake8 clean

---

To rebase and address feedback, mention me: @aiolibsbot rebase critical (fixes 🔴 only), @aiolibsbot rebase important (fixes 🔴 + 🟡), or @aiolibsbot rebase --fix for all. (A bare @aiolibsbot rebase only rebases onto the base branch.)

ℹ️ Triage summary

2 pre-existing finding(s) on unchanged code suppressed (freeze).

---

Automated review by Kōan (Claude) HEAD=16ffafe 10 min 51s

[patchback]

Backport to 3.15: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.15/2d9c5c1062eff0034fb3ab120897a6c1b3ff7041/pr-13509

Backported as #13512

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

[patchback]

Backport to 3.14: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.14/2d9c5c1062eff0034fb3ab120897a6c1b3ff7041/pr-13509

Backported as #13513

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