AVRO-4330: [python] Bound bytes/string allocation on non-seekable streams - #3931
Open
iemejia wants to merge 2 commits into
Open
AVRO-4330: [python] Bound bytes/string allocation on non-seekable streams#3931iemejia wants to merge 2 commits into
iemejia wants to merge 2 commits into
Conversation
…eams The available-bytes guard added under AVRO-4296 rejects a declared bytes/string length that exceeds the data remaining only when the reader can report the number of bytes remaining (a seekable source). On a non-seekable stream (socket, pipe, decompression stream) the check is skipped, and a single reader.read(n) for a huge declared n allocates n bytes up front before any payload is validated, so a tiny truncated or hostile input can force a large allocation. When the remaining byte count is unknown, read the value into a buffer that grows in bounded chunks rather than allocating the full declared length up front. A truncated or hostile stream then fails with a bounded InvalidAvroBinaryEncoding after a bounded allocation. The existing single-read fast path is kept when the remaining byte count is known.
There was a problem hiding this comment.
Pull request overview
This PR hardens the Python Avro BinaryDecoder against denial-of-service scenarios on non-seekable streams by avoiding a single large read(n) allocation when a huge bytes/string length prefix is declared but the remaining byte count cannot be determined.
Changes:
- Add a bounded, chunked read path (
_read_bounded) used whenbytes_remaining()is unknown (non-seekable / tell-less readers). - Add new Python unit tests intended to cover huge declared bytes/string lengths on non-seekable streams and ensure large legitimate payloads still round-trip.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/py/avro/io.py | Adds bounded chunked reads when remaining byte count is unknown to prevent large up-front allocations on hostile/truncated streams. |
| lang/py/avro/test/test_bounded_stream_read.py | Adds coverage for huge length prefixes on non-seekable streams and for large payload round-trips on the chunked-read path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback:
- Rewrite _read_bounded to accumulate into a growing bytearray instead of
a list of chunks joined at the end, matching the docstring ("growing
buffer") and avoiding the intermediate list of chunk objects.
- Strengthen the tests: the non-seekable stream wrapper now records the
largest single read request, and each test asserts the decoder never
requests a single read larger than _MAX_UNCHECKED_READ. This actually
exercises the bounded-chunk path (a truncated huge length and a
legitimately large payload both stay within the per-chunk bound) rather
than only asserting that decoding raises.
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.
What is the purpose of the change
Python SDK implementation of AVRO-4303 (parent). The available-bytes guard added
under AVRO-4296 rejects a declared bytes/string length that exceeds the data
remaining only when the reader can report the number of bytes remaining (a
seekable source). On a non-seekable stream (socket, pipe, decompression stream)
the check is skipped, and a single
reader.read(n)for a huge declarednallocates
nbytes up front before any payload is validated, so a tinytruncated or hostile input can force a large allocation.
When the remaining byte count is unknown, this reads the value into a buffer
that grows in bounded chunks rather than issuing a single
reader.read(n). Atruncated or hostile stream then fails with a bounded
InvalidAvroBinaryEncodingafter a bounded allocation. The existing single-readfast path is kept when the remaining byte count is known.
Verifying this change
This change added tests and can be verified as follows:
test_bounded_stream_read.py: a near-2GB declared bytes/string length ona truncated non-seekable stream fails with a bounded
InvalidAvroBinaryEncoding(not a huge allocation), and a legitimately largevalue on a non-seekable stream still round-trips.
test_io.py) continue to pass;ruff check,ruff formatandmypy --strictpass.Documentation
helper in
avro/io.py)