AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections#3861
AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections#3861iemejia wants to merge 33 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
|
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
…ypes when skipping
…reject negative skips
|
|
||
| _reader: IO[bytes] | ||
|
|
||
| #: Reads with a declared length above this many bytes are validated against |
There was a problem hiding this comment.
Just curious -- what is the #: for? We don't use this anywhere else in the project.
There was a problem hiding this comment.
Good catch — #: is Sphinx/autodoc syntax that documents the attribute on the following line, but you're right the project doesn't use it anywhere else and doesn't render autodoc attribute docs, so it was just inconsistent. Switched all of these to plain # comments (and dropped the couple of :data: roles for the same reason).
| if n < 0: | ||
| raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to skip, expected positive integer.") |
There was a problem hiding this comment.
Redundant with skip(n) also checking negative bytes -- remove or align error messages?
There was a problem hiding this comment.
Removed the redundant check — a negative length now falls through to skip(), which already rejects backward seeks, so there's a single enforcement point. (read() keeps its own check since it doesn't delegate to skip().)
|
Took a pass over this since it supersedes my #3876. The enum/union index hardening here is a superset of what I had, so #3876 is closed. Checked out the branch,
One thing worth considering before merge. On
if min_bytes_per_element > 0:
if count > _MAX_UNCHECKED_COLLECTION: # e.g. 1024
remaining = decoder.bytes_remaining()
if remaining is not None and count > remaining // min_bytes_per_element:
raise ...
if existing + count > structural_limit: # keep unconditional, no seek
raise ...I tried that locally: back to 0.074s, and both attack payloads above are still rejected. It does not weaken the bound either, since with Minor, take it or leave it: Neither is a blocker. LGTM otherwise. |
|
Thanks for the detailed benchmarking, @arib06. Applied both suggestions:
|
The #: attribute-doc syntax was not used anywhere else in the project and autodoc attribute docs are not rendered, so convert these comments (and the couple of :data: roles) to plain # comments for consistency.
skip() already rejects a negative byte count, so a negative length prefix falls through to it; keep skip() as the single backward-seek guard.
Only pay for the per-block bytes_remaining() seek (tell + seek-to-end + seek-back) when a block count exceeds _MAX_UNCHECKED_COLLECTION, mirroring _MAX_UNCHECKED_READ in read(): a small block of positive-size elements must be backed by real bytes on the wire and cannot over-allocate meaningfully. The structural cap stays unconditional and the zero-byte path is unchanged, so the bound is not weakened. Also document that AVRO_MAX_COLLECTION_ITEMS pins both the zero-byte and structural limits to the same value.
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