AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections#3861
Open
iemejia wants to merge 30 commits into
Open
AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections#3861iemejia wants to merge 30 commits into
iemejia wants to merge 30 commits into
Conversation
…ngth-prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data. - BinaryDecoder.bytes_remaining() reports the bytes still readable for a seekable reader (else None). read() uses it to reject an over-large declared length above a threshold before allocating. - DatumReader.read_array/read_map reject a block whose element count could not be backed by the bytes remaining, using min_bytes_per_element() computed from the element schema so a zero-byte element type (e.g. null) is not falsely rejected. Mirrors the Java SDK's checks (AVRO-4241). Non-seekable readers, whose remaining length is unknown, are unaffected. Assisted-by: GitHub Copilot:claude-opus-4.8
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens the Python Avro binary decoding path against malicious or truncated inputs that declare excessively large length/count prefixes, by validating declared sizes against bytes remaining for seekable inputs before allocating/iterating.
Changes:
- Add
BinaryDecoder.bytes_remaining()and use it to pre-reject oversizedread(n)requests (above a threshold) when the reader is seekable. - Add minimum on-wire-size estimation for schemas and use it to validate array/map block counts in
DatumReaderagainst remaining bytes. - Add unit tests covering oversized length prefixes, oversized collection block counts, and a non-false-positive case (array of
null).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lang/py/avro/io.py | Adds remaining-bytes introspection plus pre-checks for large length-prefixed reads and collection block count validation using per-element minimum sizes. |
| lang/py/avro/test/test_io.py | Adds targeted tests for the new available-bytes validation behavior in BinaryDecoder and DatumReader (including array-of-nulls). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review feedback: - avro/io.py imported the standard library 'io' module, which CodeQL flags as a module importing itself. Import 'os' and use os.SEEK_END instead (both equal 2). - _ensure_collection_available compares count against remaining // min bytes per element rather than multiplying, so an attacker-controlled (unbounded) count does not create a huge intermediate product. Assisted-by: GitHub Copilot:claude-opus-4.8
…n minimum once Review feedback: - bytes_remaining() now wraps tell()/seek() in try/except and uses tell() for the end offset, returning None on any failure (non-seekable reader, seek() that returns None, or a reader without seekable()). This keeps reads unaffected when the remaining size cannot be determined. - read_array/read_map compute the per-element minimum once before the block loop instead of on every block, avoiding repeated schema traversal. Assisted-by: GitHub Copilot:claude-opus-4.8
The Typechecks CI step (mypy) flagged the isinstance(pos/end, int) check as unreachable: reader is typed IO[bytes], so tell() already returns int, making the guard always true and the following return None unreachable. The try/except around tell()/seek() already provides the resilience the check was meant to add, so remove the redundant guard. Assisted-by: GitHub Copilot:claude-opus-4.8
Review feedback: bytes_remaining() could leave the reader positioned at EOF if tell()/seek() failed after seeking to the end, corrupting subsequent decoding. Move the restore seek into a finally block so the original position is always restored. Added tests that the position is restored on both success and when reading the end offset fails. Assisted-by: GitHub Copilot:claude-opus-4.8
… dead test assignment Review feedback: - The finally block in bytes_remaining() only caught (OSError, ValueError) when restoring the position. A reader that implements tell() but not seek() would raise AttributeError from reader.seek(pos) and let it escape; catch AttributeError there too so the method reliably falls back to None. - Removed the unused self._calls assignment from the FailingEndStream test helper. Assisted-by: GitHub Copilot:claude-opus-4.8
Completes the available-bytes protection for collections and supersedes the
separate collection-limit change. Elements whose schema encodes to zero bytes
(null, a zero-length fixed, or a record with only zero-byte fields) consume no
input, so the bytes-remaining check cannot bound their count. A tiny payload
declaring a huge array block count of such elements (e.g.
{"type":"array","items":"null"} with a count of 200,000,000) therefore drove an
unbounded list allocation and exhausted memory.
_ensure_collection_available now enforces, per block:
- the bytes-remaining check for elements with a positive on-wire minimum;
- a heap-independent cap on zero-byte elements (DEFAULT_MAX_COLLECTION_ITEMS =
10,000,000);
- a structural cap on all collections (DEFAULT_MAX_COLLECTION_STRUCTURAL =
Integer.MAX_VALUE - 8) as an overflow / defense-in-depth guard, covering
non-seekable readers where the bytes check cannot run.
AVRO_MAX_COLLECTION_ITEMS, when set, caps both limits. Applied to read_array,
read_map, skip_array and skip_map (cumulative across blocks, and after
normalizing a negative block count); maps are additionally bounded by their
>=1-byte keys. Raises the new AvroCollectionSizeException.
Assisted-by: GitHub Copilot:claude-opus-4.8
Member
Author
|
This PR now also includes the collection block-count cap for [python], so it is the single complete fix for collection allocation DoS in this SDK. In addition to validating available bytes before allocating length-prefixed values, it bounds the number of array/map items per block:
With this, the standalone collection-limit change for [python] (AVRO-4282, #3845) is redundant and is being closed as superseded by this PR. |
iemejia
changed the title
read_array/read_map now read the running length (len(read_items)) to enforce the collection limits before the first append/assignment, which left mypy unable to infer the element type of the empty list/dict from later usage (var-annotated). Annotate read_items explicitly (List[object] and Dict[str, object]) so mypy is satisfied; this is a typing-only change with no runtime effect. Assisted-by: GitHub Copilot:claude-opus-4.8
…ages ruff format (>=0.15.1, as run by ./build.sh) collapses the two multi-line InvalidAvroBinaryEncoding raises in _skip_block_bytes onto single lines. Apply it so `./build.sh test` (which runs `ruff format --diff` first) passes in CI. Assisted-by: GitHub Copilot:claude-opus-4.8
…ypes when skipping
…reject negative skips
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
A
bytesorstringvalue is encoded as a length prefix followed by that many bytes of data, and anarrayormapblock is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.This applies the equivalent of the Java SDK fix AVRO-4241 to the Python SDK and extends it to collections. It has two complementary parts.
1. Validate available bytes before allocating
When the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as
null) is never falsely rejected. Sources that cannot report their remaining size are unaffected.BinaryDecoder.bytes_remaining()reports the bytes still readable for a seekable reader (elseNone).read()rejects an over-large declared length above a threshold, andDatumReader.read_array/read_mapreject a block whose element count could not be backed by the bytes remaining, usingmin_bytes_per_element()from the element schema.2. Cap collection allocation for zero-byte elements
Zero-byte elements (
null, a zero-lengthfixed, or a record with only zero-byte fields) consume no input, so the available-bytes check cannot bound their count: a tiny payload such as{{"type":"array","items":"null"}}declaring a block count of 200,000,000 would otherwise drive an unbounded allocation. In addition to the available-bytes check,_ensure_collection_availablecaps the cumulative count of zero-byte elements (DEFAULT_MAX_COLLECTION_ITEMS= 10,000,000) and applies a structural cap to every collection (DEFAULT_MAX_COLLECTION_STRUCTURAL=Integer.MAX_VALUE - 8) covering non-seekable readers. It is applied toread_array,read_map,skip_arrayandskip_map(cumulative across blocks, and after normalizing a negative block count); maps are additionally bounded by their >=1-byte keys. Rejections raise the newAvroCollectionSizeException. When set, theAVRO_MAX_COLLECTION_ITEMSenvironment variable caps both limits.This folds in and supersedes the standalone collection-limit change (AVRO-4282, #3845), so this PR is the single complete fix for collection/length-prefixed allocation DoS in the Python SDK.
This is a sub-task of AVRO-4292 and resolves AVRO-4296.
Verifying this change
This change added tests and can be verified as follows:
TestBinaryDecoderAvailableBytes,TestDatumReaderCollectionAvailableBytesand the zero-byte cap tests inlang/py/avro/test/test_io.py, including anarray<null>with a huge block count that must be rejected and a small one that still decodes.cd lang/py && python3 -m unittest avro.test.test_ioDocumentation