You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "iemejia (via GitHub)" <gi...@apache.org> on 2026/07/11 21:20:25 UTC
[PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
iemejia opened a new pull request, #3861:
URL: https://github.com/apache/avro/pull/3861
## What is the purpose of the change
A `bytes` or `string` value is encoded as a length prefix followed by that many bytes of data, and an `array` or `map` block 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](https://issues.apache.org/jira/browse/AVRO-4241) to the Python SDK: 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 (else `None`). `read()` rejects an over-large declared length above a threshold, and `DatumReader.read_array`/`read_map` reject a block whose element count could not be backed by the bytes remaining, using `min_bytes_per_element()` from the element schema.
This is a sub-task of [AVRO-4292](https://issues.apache.org/jira/browse/AVRO-4292) and resolves [AVRO-4296](https://issues.apache.org/jira/browse/AVRO-4296).
## Verifying this change
This change added tests and can be verified as follows:
- Added `TestBinaryDecoderAvailableBytes` and `TestDatumReaderCollectionAvailableBytes` in `lang/py/avro/test/test_io.py`, including an array of nulls that must not be falsely rejected.
- Run: `cd lang/py && python3 -m unittest avro.test.test_io`
## Documentation
- Does this pull request introduce a new feature? (no — hardening / robustness)
- If yes, how is the feature documented? (not applicable)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565706911
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
+ except (OSError, ValueError, AttributeError):
+ return None
+ finally:
+ # Always restore the original position, even if seeking to the end
+ # or reading it failed, so the reader is never left at EOF.
+ try:
+ reader.seek(pos)
+ except (OSError, ValueError):
Review Comment:
`bytes_remaining()` intends to return `None` for non-seekable / unsupported readers, but the `finally` restore only catches `(OSError, ValueError)`. If a reader implements `tell()` but not `seek()`, the `AttributeError` from `reader.seek(pos)` will escape from the `finally` block and break decoding. Catch `AttributeError` there as well so the method reliably falls back to `None`/best-effort behavior.
##########
lang/py/avro/test/test_io.py:
##########
@@ -550,6 +551,136 @@ def test_deserialization_fails(self) -> None:
)
+class TestBinaryDecoderAvailableBytes(unittest.TestCase):
+ """A bytes/string value declares a length prefix; a malicious or truncated
+ input can declare far more bytes than actually exist. On a seekable reader
+ that is rejected before allocating for it."""
+
+ @staticmethod
+ def _encode_length_prefix(length: int) -> bytes:
+ buf = io.BytesIO()
+ avro.io.BinaryEncoder(buf).write_long(length)
+ return buf.getvalue()
+
+ def test_read_rejects_length_beyond_stream(self) -> None:
+ # Declares 100 MiB but provides no data.
+ prefix = self._encode_length_prefix(100 * 1024 * 1024)
+ with io.BytesIO(prefix) as bio:
+ decoder = avro.io.BinaryDecoder(bio)
+ self.assertRaises(avro.errors.InvalidAvroBinaryEncoding, decoder.read_bytes)
+
+ def test_read_within_stream_still_reads(self) -> None:
+ # A well-formed large value whose data is actually present still reads.
+ payload = b"x" * (2 * 1024 * 1024)
+ buf = io.BytesIO()
+ avro.io.BinaryEncoder(buf).write_bytes(payload)
+ with io.BytesIO(buf.getvalue()) as bio:
+ decoder = avro.io.BinaryDecoder(bio)
+ self.assertEqual(decoder.read_bytes(), payload)
+
+ def test_bytes_remaining_restores_position(self) -> None:
+ # bytes_remaining() must leave the reader position unchanged.
+ with io.BytesIO(b"abcdefghij") as bio:
+ bio.seek(3)
+ decoder = avro.io.BinaryDecoder(bio)
+ self.assertEqual(decoder.bytes_remaining(), 7)
+ self.assertEqual(bio.tell(), 3)
+
+ def test_bytes_remaining_restores_position_on_error(self) -> None:
+ # If reading the end offset fails after seeking, the original position
+ # must still be restored (via the finally block).
+ class FailingEndStream(io.BytesIO):
+ def __init__(self, data: bytes) -> None:
+ super().__init__(data)
+ self._calls = 0
+
Review Comment:
`FailingEndStream.__init__` sets `self._calls = 0` but the attribute is never used in the test. Removing the dead assignment reduces noise and avoids implying additional assertions around call counts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567489457
##########
lang/py/avro/io.py:
##########
@@ -795,22 +971,35 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
def skip_array(self, writers_schema: avro.schema.ArraySchema, decoder: BinaryDecoder) -> None:
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
decoder.skip(block_size)
else:
Review Comment:
When an array block is encoded with a negative count, the following `block_size` comes from the input. Currently it is passed directly to `decoder.skip(block_size)` with no validation, so a malicious negative `block_size` would seek backwards and corrupt decoding (and an oversized `block_size` can seek past EOF). Validate that `block_size` is non-negative (and, when `bytes_remaining()` is known, that it does not exceed the remaining bytes) before skipping.
##########
lang/py/avro/io.py:
##########
@@ -832,23 +1021,42 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, items_read, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_read += block_count
for i in range(block_count):
key = decoder.read_utf8()
read_items[key] = self.read_data(writers_schema.values, readers_schema.values, decoder)
block_count = decoder.read_long()
return read_items
def skip_map(self, writers_schema: avro.schema.MapSchema, decoder: BinaryDecoder) -> None:
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
decoder.skip(block_size)
else:
Review Comment:
When a map block is encoded with a negative count, the subsequent `block_size` is attacker-controlled. Passing it directly to `decoder.skip(block_size)` without validating it allows negative sizes (seeking backwards) and oversized sizes (seeking past EOF), which can corrupt the decoder state. Validate `block_size` before skipping (and optionally ensure it does not exceed `bytes_remaining()` when available).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565066650
##########
lang/py/avro/io.py:
##########
@@ -780,6 +841,22 @@ def read_enum(self, writers_schema: avro.schema.EnumSchema, readers_schema: avro
def skip_enum(self, writers_schema: avro.schema.EnumSchema, decoder: BinaryDecoder) -> None:
return decoder.skip_int()
+ @staticmethod
+ def _ensure_collection_available(decoder: BinaryDecoder, count: int, min_bytes_per_element: int) -> None:
+ """
+ Reject a collection (array or map) block whose declared element count
+ could not be backed by the bytes actually remaining, before iterating.
+ Skipped when the per-element minimum is zero (e.g. an array of nulls) or
+ when the reader cannot report how many bytes remain.
+ """
+ if count <= 0 or min_bytes_per_element <= 0:
+ return
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and count * min_bytes_per_element > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(
+ f"Collection claims {count} elements with at least {min_bytes_per_element} bytes each, but only {remaining} bytes are available."
+ )
Review Comment:
`count * min_bytes_per_element` can create a very large intermediate integer when `count` is attacker-controlled (Avro long is unbounded in Python). You can avoid the potentially expensive multiplication by comparing via integer division instead, while keeping the same semantics for positive values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565162634
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,30 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader). Used to reject a
+ declared length or collection block count that exceeds the data
+ actually available before allocating for it.
+ """
+ reader = self.reader
+ if not getattr(reader, "seekable", None) or not reader.seekable():
+ return None
+ pos = reader.tell()
+ end = reader.seek(0, os.SEEK_END)
+ reader.seek(pos)
+ return end - pos
Review Comment:
`bytes_remaining()` relies on `reader.seekable()` existing and on `seek()` returning the new position. Some seekable file-like objects may not implement `seekable()`, and some `seek()` implementations return `None`, which would raise a `TypeError` at `end - pos` and/or skip the protection unnecessarily. Consider using `tell()/seek()` directly with a try/except fallback and use `tell()` to get the end position after seeking.
##########
lang/py/avro/io.py:
##########
@@ -801,6 +881,7 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, block_count, _min_bytes_per_element(writers_schema.items))
for i in range(block_count):
Review Comment:
`_min_bytes_per_element(writers_schema.items)` is recomputed for every array block. For complex schemas and many blocks this adds avoidable overhead; compute it once before the loop and reuse it.
##########
lang/py/avro/io.py:
##########
@@ -838,6 +919,8 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ self._ensure_collection_available(decoder, block_count, 1 + _min_bytes_per_element(writers_schema.values))
for i in range(block_count):
Review Comment:
`1 + _min_bytes_per_element(writers_schema.values)` is recomputed for every map block. Computing it once before the loop avoids repeated schema traversal for complex value types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570359177
##########
lang/py/avro/io.py:
##########
@@ -795,23 +994,60 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int, block_count: int, min_bytes: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Also require that block_size can plausibly hold block_count
+ elements at their minimum on-wire size, so a too-small size cannot
+ misalign the decoder. Reject before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} exceeds the {remaining} bytes remaining")
+ if min_bytes > 0 and block_count > block_size // min_bytes:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} is too small for {block_count} elements of >= {min_bytes} bytes")
+ try:
+ decoder.skip(block_size)
+ except (OSError, ValueError, OverflowError) as e:
Review Comment:
Fixed in 27dafe6: `_skip_block_bytes()` now also catches `AttributeError` and `TypeError` from `decoder.skip()` (a reader lacking `seek()`/`tell()`, or returning an unexpected type) and re-raises them as `InvalidAvroBinaryEncoding`, so malformed sized blocks fail with a consistent Avro error. The regression test now exercises all five exception types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565728596
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
+ except (OSError, ValueError, AttributeError):
+ return None
+ finally:
+ # Always restore the original position, even if seeking to the end
+ # or reading it failed, so the reader is never left at EOF.
+ try:
+ reader.seek(pos)
+ except (OSError, ValueError):
Review Comment:
Fixed in 102061267c: the finally block now also catches AttributeError when restoring the position, so a reader that implements tell() but not seek() reliably falls back to None instead of raising.
##########
lang/py/avro/test/test_io.py:
##########
@@ -550,6 +551,136 @@ def test_deserialization_fails(self) -> None:
)
+class TestBinaryDecoderAvailableBytes(unittest.TestCase):
+ """A bytes/string value declares a length prefix; a malicious or truncated
+ input can declare far more bytes than actually exist. On a seekable reader
+ that is rejected before allocating for it."""
+
+ @staticmethod
+ def _encode_length_prefix(length: int) -> bytes:
+ buf = io.BytesIO()
+ avro.io.BinaryEncoder(buf).write_long(length)
+ return buf.getvalue()
+
+ def test_read_rejects_length_beyond_stream(self) -> None:
+ # Declares 100 MiB but provides no data.
+ prefix = self._encode_length_prefix(100 * 1024 * 1024)
+ with io.BytesIO(prefix) as bio:
+ decoder = avro.io.BinaryDecoder(bio)
+ self.assertRaises(avro.errors.InvalidAvroBinaryEncoding, decoder.read_bytes)
+
+ def test_read_within_stream_still_reads(self) -> None:
+ # A well-formed large value whose data is actually present still reads.
+ payload = b"x" * (2 * 1024 * 1024)
+ buf = io.BytesIO()
+ avro.io.BinaryEncoder(buf).write_bytes(payload)
+ with io.BytesIO(buf.getvalue()) as bio:
+ decoder = avro.io.BinaryDecoder(bio)
+ self.assertEqual(decoder.read_bytes(), payload)
+
+ def test_bytes_remaining_restores_position(self) -> None:
+ # bytes_remaining() must leave the reader position unchanged.
+ with io.BytesIO(b"abcdefghij") as bio:
+ bio.seek(3)
+ decoder = avro.io.BinaryDecoder(bio)
+ self.assertEqual(decoder.bytes_remaining(), 7)
+ self.assertEqual(bio.tell(), 3)
+
+ def test_bytes_remaining_restores_position_on_error(self) -> None:
+ # If reading the end offset fails after seeking, the original position
+ # must still be restored (via the finally block).
+ class FailingEndStream(io.BytesIO):
+ def __init__(self, data: bytes) -> None:
+ super().__init__(data)
+ self._calls = 0
+
Review Comment:
Removed the unused self._calls assignment from the FailingEndStream test helper in 102061267c.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on PR #3861:
URL: https://github.com/apache/avro/pull/3861#issuecomment-4951284231
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:
- a heap-aware cap for zero-byte-element collections (e.g. `array<null>`), which otherwise bypass the available-bytes check because each element reads 0 bytes;
- a structural cap for all collections;
- bounded skip paths so projection/skip cannot loop unboundedly.
With this, the standalone collection-limit change for [python] (AVRO-4282, #3845) is redundant and is being closed as superseded by this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567433997
##########
lang/py/avro/io.py:
##########
@@ -599,6 +644,90 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 8
Review Comment:
Good catch — the value was actually off by one: `(1 << 31) - 8` = 2147483640, not `Integer.MAX_VALUE - 8` = 2147483639. Fixed the constant to `(1 << 31) - 1 - 8` so it matches the docstring and the other SDKs (C/C++/C# all use 2147483639).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570119296
##########
lang/py/avro/io.py:
##########
@@ -795,23 +994,60 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int, block_count: int, min_bytes: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Also require that block_size can plausibly hold block_count
+ elements at their minimum on-wire size, so a too-small size cannot
+ misalign the decoder. Reject before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} exceeds the {remaining} bytes remaining")
+ if min_bytes > 0 and block_count > block_size // min_bytes:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} is too small for {block_count} elements of >= {min_bytes} bytes")
+ try:
+ decoder.skip(block_size)
+ except (OSError, ValueError, OverflowError) as e:
Review Comment:
`_skip_block_bytes()` wraps seek/skip failures as `InvalidAvroBinaryEncoding`, but it currently only catches `OSError/ValueError/OverflowError`. If the underlying `BinaryDecoder` is backed by a reader that lacks `seek()`/`tell()` (or returns an unexpected type), `decoder.skip()` can raise `AttributeError`/`TypeError` and leak out as a non-Avro exception. Catch those as well so malformed sized blocks fail with consistent Avro decoding errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565199903
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,30 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader). Used to reject a
+ declared length or collection block count that exceeds the data
+ actually available before allocating for it.
+ """
+ reader = self.reader
+ if not getattr(reader, "seekable", None) or not reader.seekable():
+ return None
+ pos = reader.tell()
+ end = reader.seek(0, os.SEEK_END)
+ reader.seek(pos)
+ return end - pos
Review Comment:
Fixed in f415f1f: bytes_remaining() now wraps tell()/seek() in try/except (catching OSError/ValueError/AttributeError), uses tell() for the end offset, and validates both are ints, returning None on any failure.
##########
lang/py/avro/io.py:
##########
@@ -801,6 +881,7 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, block_count, _min_bytes_per_element(writers_schema.items))
for i in range(block_count):
Review Comment:
Fixed in f415f1f: read_array computes the per-element minimum once before the block loop.
##########
lang/py/avro/io.py:
##########
@@ -838,6 +919,8 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ self._ensure_collection_available(decoder, block_count, 1 + _min_bytes_per_element(writers_schema.values))
for i in range(block_count):
Review Comment:
Fixed in f415f1f: read_map computes 1 + min-bytes once before the block loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567393889
##########
lang/py/avro/io.py:
##########
@@ -832,23 +1016,37 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
`read_map()` uses `len(read_items)` as the cumulative element count for `_ensure_collection_available()`. Because map keys are not guaranteed unique, duplicate keys can keep `len(read_items)` small while still decoding a very large number of entries, bypassing the intended cumulative structural/zero-byte caps across blocks. Track the number of decoded entries separately from the dictionary size and use that for the limit check.
##########
lang/py/avro/io.py:
##########
@@ -599,6 +644,90 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 8
Review Comment:
The docstring says this cap matches ``Integer.MAX_VALUE - 8``, but the value is computed as `(1 << 31) - 8` (i.e., `2**31 - 8`). Please align the comment with the actual constant to avoid confusion about the intended limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567610879
##########
lang/py/avro/io.py:
##########
@@ -795,23 +981,59 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
For negative block counts, the following block-size long is currently skipped via `decoder.skip_long()`. That bypasses the new `read_long()` varint-length validation (and will scan arbitrarily long malformed varints byte-by-byte), so a crafted stream can still cause excessive work here. Read and validate the block-size with `read_long()` (even if you ignore the value) so malformed/overlong varints are rejected consistently.
##########
lang/py/avro/io.py:
##########
@@ -832,24 +1054,43 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
For negative map block counts, the block-size long is skipped via `decoder.skip_long()`, which bypasses `read_long()`’s overlong-varint checks and can scan arbitrarily long malformed varints. Read (and minimally validate) the block size with `read_long()` so malformed encodings are rejected consistently and can’t drive excessive work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3610581361
##########
lang/py/avro/io.py:
##########
@@ -198,6 +199,13 @@ class BinaryDecoder:
_reader: IO[bytes]
+ #: Reads with a declared length above this many bytes are validated against
Review Comment:
Just curious -- what is the `#:` for? We don't use this anywhere else in the project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "arib06 (via GitHub)" <gi...@apache.org>.
arib06 commented on PR #3861:
URL: https://github.com/apache/avro/pull/3861#issuecomment-5019260786
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, `python3 -m unittest avro.test.test_io` is green (186 tests). I confirmed the two things I would most expect to be off-by-one:
- the 10-byte varint cap in `read_long`/`skip_long` still round-trips the full 64-bit range (`2**63-1` and `-2**63` both encode to 10 bytes and decode correctly), and the `shift == 63 and (b & 0x7E)` guard correctly allows bit 63 while rejecting bits 64+.
- `array<null>` with a 200M block count raises `AvroCollectionSizeException`, and a huge declared count with a tiny payload raises `InvalidAvroBinaryEncoding`.
One thing worth considering before merge. `_ensure_collection_available` calls `decoder.bytes_remaining()` for every array/map block whose element type has a positive minimum size, no matter how small the declared count is, and `bytes_remaining()` does a `tell` + `seek(0, SEEK_END)` + `tell` + `seek(pos)` each time. On a real file object that is not free. Decoding 20k small `array<int>` values from a file:
```
with checks: 0.137s
bytes_remaining stubbed None: 0.074s (~84% overhead)
```
On `BytesIO` it barely shows, which is why the tests do not surface it, but the seek is per block against the underlying reader.
`read()` already avoids this with `_MAX_UNCHECKED_READ`, on the reasoning that a small read cannot over-allocate meaningfully. The same reasoning applies to a small block count. Gating just the `bytes_remaining()` call on a threshold recovers it:
```python
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 `min_bytes > 0` a block of under-threshold elements still has to be paid for in real bytes on the wire, so total allocation stays bounded by the input size. The zero-byte path is untouched and keeps the tighter cumulative cap, which is the case where the bytes-remaining check could not bound anything anyway.
Minor, take it or leave it: `_collection_limits()` returns `(parsed, parsed)`, so setting `AVRO_MAX_COLLECTION_ITEMS` to raise the zero-byte limit also lowers the structural cap from ~2.1B to that same value. It is documented, but the coupling runs in both directions and the name only suggests one. Might be worth a line in the docstring calling that out explicitly.
Neither is a blocker. LGTM otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3612690299
##########
lang/py/avro/io.py:
##########
@@ -198,6 +199,13 @@ class BinaryDecoder:
_reader: IO[bytes]
+ #: Reads with a declared length above this many bytes are validated against
Review 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).
##########
lang/py/avro/io.py:
##########
@@ -393,12 +459,28 @@ def skip_double(self) -> None:
self.skip(8)
def skip_bytes(self) -> None:
- self.skip(self.read_long())
+ # The length prefix is attacker-controlled: a negative value would seek
+ # backwards (corrupting the decoder position, e.g. an infinite loop
+ # during schema resolution) and an oversized value past EOF. Validate it
+ # the same way ``read`` does before skipping.
+ n = self.read_long()
+ if n < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to skip, expected positive integer.")
Review 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()`.)
##########
lang/py/avro/io.py:
##########
@@ -246,6 +286,11 @@ def read_long(self) -> int:
n = b & 0x7F
shift = 7
while (b & 0x80) != 0:
+ # A 64-bit value needs at most 10 bytes (shifts 0..63); reject an
+ # overlong varint rather than accepting a malformed, arbitrarily
+ # large value.
+ if shift >= 70:
+ raise avro.errors.InvalidAvroBinaryEncoding("Varint is too long")
b = ord(self.read(1))
n |= (b & 0x7F) << shift
shift += 7
Review Comment:
Took a look and compared against Java's `BinaryDecoder.readLong`/`innerLongDecode`:
- Our 10-byte cap matches Java: both reject an 11th continuation byte, so neither accepts a varint longer than 10 bytes.
- On the 10th byte we're actually *stricter* than Java — we reject payload bits above bit 63 (`b & 0x7E`), whereas Java `^=`s them shifted past the 64-bit boundary and silently discards the overflow. Nothing a conformant encoder emits is rejected by our check.
- You're right that the spec doesn't mandate minimal encoding, so non-minimal forms within the 10-byte budget (trailing zero-continuation groups) are accepted by both SDKs and decode identically — a real determinism gap, but a cross-language one that would need a spec change to require canonical varints.
For this PR the 10-byte bound is the security-relevant part (it bounds both the work and the value range), so I'd prefer to keep that here. How would you suggest handling the determinism/canonical-encoding question — is it worth a separate JIRA + spec PR to require minimal varint encoding across languages, and do you think we should also relax our extra 10th-byte strictness to exactly match Java's behavior in the meantime? Happy to go either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3571065025
##########
lang/py/avro/io.py:
##########
@@ -795,23 +1001,61 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int, block_count: int, min_bytes: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Also require that block_size can plausibly hold block_count
+ elements at their minimum on-wire size, so a too-small size cannot
+ misalign the decoder. Reject before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} exceeds the {remaining} bytes remaining")
+ if min_bytes > 0 and block_count > block_size // min_bytes:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} is too small for {block_count} elements of >= {min_bytes} bytes")
Review Comment:
Fixed in 25eb3ab: `_skip_block_bytes()` now requires `block_size == 0` when `min_bytes == 0` (an always-zero-byte element type: null, a zero-length fixed, or a record of only zero-byte fields). Such a block has no payload, so a positive size is rejected as malformed rather than skipped into the following fields. Added a regression test (positive size rejected, zero size accepted); the seek-error wrapping test now uses min_bytes>0 so it still exercises the skip path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3568037516
##########
lang/py/avro/io.py:
##########
@@ -795,23 +981,59 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
Addressed in commit 09dbd4f2fd — skip_long now enforces the 10-byte varint cap (raising InvalidAvroBinaryEncoding), so consuming the negative-block byte-size via skip_long is now bounded.
##########
lang/py/avro/io.py:
##########
@@ -832,24 +1054,43 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
Same — the map negative-block byte-size skipped via skip_long is now bounded by the 10-byte cap added to skip_long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3568464169
##########
lang/py/avro/io.py:
##########
@@ -795,23 +994,55 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int, block_count: int, min_bytes: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Also require that block_size can plausibly hold block_count
+ elements at their minimum on-wire size, so a too-small size cannot
+ misalign the decoder. Reject before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} exceeds the {remaining} bytes remaining")
+ if min_bytes > 0 and block_count > block_size // min_bytes:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} is too small for {block_count} elements of >= {min_bytes} bytes")
+ decoder.skip(block_size)
Review Comment:
`_skip_block_bytes()` calls `decoder.skip(block_size)` directly. If the underlying reader rejects the seek (e.g., oversized offset, invalid seek, or other IO error), this can raise `OSError`/`ValueError`/`OverflowError` (or similar) instead of an Avro exception, leaking an unexpected error type to callers. Consider wrapping the skip and re-raising as `InvalidAvroBinaryEncoding` for consistent failure semantics on malformed sized blocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba commented on PR #3861:
URL: https://github.com/apache/avro/pull/3861#issuecomment-5213576272
Cherry-picked to [branch-1.12|https://github.com/apache/avro/commit/c702676903c38680171f41fa95d4e5c56d01efff].
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570577829
##########
lang/py/avro/io.py:
##########
@@ -599,6 +665,97 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
Review Comment:
The doc comment for `MAX_COLLECTION_ITEMS_ENV` says it overrides only the maximum number of *zero-byte* collection elements, but `_collection_limits()` uses this env var to cap both the zero-byte limit and the structural limit. Updating the comment avoids misleading users about what setting `AVRO_MAX_COLLECTION_ITEMS` will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570744997
##########
lang/py/avro/io.py:
##########
@@ -599,6 +665,99 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the collection element limits. When set to a
+#: non-negative integer it caps both the number of zero-byte-encoded collection
+#: elements (e.g. an array of nulls) and the structural cap on the total number of
+#: elements in any collection, allocated from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 1 - 8 # Integer.MAX_VALUE - 8
+
+
+def _collection_limits() -> Tuple[int, int]:
+ """Return ``(zero_byte_limit, structural_limit)``.
+
+ ``AVRO_MAX_COLLECTION_ITEMS``, when set to a non-negative integer, caps both
+ zero-byte-element collections and all other collections at that value.
+ Otherwise zero-byte elements use the tighter :data:`DEFAULT_MAX_COLLECTION_ITEMS`
+ and all collections use :data:`DEFAULT_MAX_COLLECTION_STRUCTURAL`.
+ """
+ value = os.environ.get(MAX_COLLECTION_ITEMS_ENV)
+ if value is None:
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ try:
+ parsed = int(value)
+ except ValueError:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring invalid {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ if parsed < 0:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring negative {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ return parsed, parsed
+
+
+def _max_collection_items() -> int:
+ """Return the configured zero-byte-element collection limit."""
+ return _collection_limits()[0]
+
+
+def _min_bytes_per_element(schema: avro.schema.Schema, visited: Optional[Set[int]] = None) -> int:
+ """
+ Return the minimum number of bytes a single value of ``schema`` can occupy
+ on the wire. Used to reject an array/map block count that could not possibly
+ be backed by the bytes remaining. A type that can encode to zero bytes
+ (``null``) returns 0, which disables the collection check for it (avoiding a
+ false positive on, e.g., an array of nulls).
+ """
+ if visited is None:
+ visited = set()
+ schema_type = schema.type
+ if schema_type == "null":
+ return 0
+ if schema_type == "float":
+ return 4
+ if schema_type == "double":
+ return 8
+ if schema_type == "fixed":
+ return getattr(schema, "size", 1)
+ if schema_type in ("record", "error"):
+ # Guard against self-referencing records (recursion would not terminate).
+ if id(schema) in visited:
+ return 0
Review Comment:
Fixed in a1920ee: `_min_bytes_per_element()` now returns 1 (not 0) when it detects a recursive record/error reference. A recursive value must terminate through a union (>= 1 byte branch index) or an empty array/map (>= 1 byte block count), so 1 is a safe conservative lower bound — recursive records are no longer treated as zero-byte elements, and unions containing them are not underestimated. Added a regression test covering the recursive record and a union containing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3610638326
##########
lang/py/avro/io.py:
##########
@@ -246,6 +286,11 @@ def read_long(self) -> int:
n = b & 0x7F
shift = 7
while (b & 0x80) != 0:
+ # A 64-bit value needs at most 10 bytes (shifts 0..63); reject an
+ # overlong varint rather than accepting a malformed, arbitrarily
+ # large value.
+ if shift >= 70:
+ raise avro.errors.InvalidAvroBinaryEncoding("Varint is too long")
b = ord(self.read(1))
n |= (b & 0x7F) << shift
shift += 7
Review Comment:
Oooh, I think we accept values with more than 10 bytes with left-padded zeros. I don't think the spec requires us to minimize the number of bytes that a varint uses, and I'm not sure Java checks (for example).
We probably should require eliminating left padded zero bits in the spec so that integers and longs are encoded deterministically! Can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3610597907
##########
lang/py/avro/io.py:
##########
@@ -393,12 +459,28 @@ def skip_double(self) -> None:
self.skip(8)
def skip_bytes(self) -> None:
- self.skip(self.read_long())
+ # The length prefix is attacker-controlled: a negative value would seek
+ # backwards (corrupting the decoder position, e.g. an infinite loop
+ # during schema resolution) and an oversized value past EOF. Validate it
+ # the same way ``read`` does before skipping.
+ n = self.read_long()
+ if n < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to skip, expected positive integer.")
Review Comment:
Redundant with skip(n) also checking negative bytes -- remove or align error messages?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567459235
##########
lang/py/avro/io.py:
##########
@@ -599,6 +644,90 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 8
Review Comment:
This was fixed in commit 8279901e00 — the constant is now `(1 << 31) - 1 - 8` (2147483639 = Integer.MAX_VALUE - 8).
##########
lang/py/avro/io.py:
##########
@@ -246,6 +286,11 @@ def read_long(self) -> int:
n = b & 0x7F
shift = 7
while (b & 0x80) != 0:
+ # A 64-bit value needs at most 10 bytes (shifts 0..63); reject an
+ # overlong varint rather than accepting a malformed, arbitrarily
+ # large value.
+ if shift >= 70:
+ raise avro.errors.InvalidAvroBinaryEncoding("Varint is too long")
b = ord(self.read(1))
n |= (b & 0x7F) << shift
shift += 7
Review Comment:
Fixed — read_long now rejects a 10th byte (shift == 63) with any payload bit above bit 63 set (`b & 0x7E`), so a 10-byte varint outside the 64-bit range is rejected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567513381
##########
lang/py/avro/io.py:
##########
@@ -795,22 +971,35 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
def skip_array(self, writers_schema: avro.schema.ArraySchema, decoder: BinaryDecoder) -> None:
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
decoder.skip(block_size)
else:
Review Comment:
Fixed — skip_array now routes the sized-block byte-size through _skip_block_bytes(), which rejects a negative size and one larger than bytes_remaining() before seeking.
##########
lang/py/avro/io.py:
##########
@@ -832,23 +1021,42 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, items_read, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_read += block_count
for i in range(block_count):
key = decoder.read_utf8()
read_items[key] = self.read_data(writers_schema.values, readers_schema.values, decoder)
block_count = decoder.read_long()
return read_items
def skip_map(self, writers_schema: avro.schema.MapSchema, decoder: BinaryDecoder) -> None:
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
decoder.skip(block_size)
else:
Review Comment:
Fixed — skip_map uses the same _skip_block_bytes() validation before skipping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567342974
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
+ except (OSError, ValueError, AttributeError):
+ return None
Review Comment:
Fixed — added `TypeError` to the caught exceptions so `bytes_remaining()` falls back to `None` (unknown) rather than propagating.
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
+ except (OSError, ValueError, AttributeError):
+ return None
+ finally:
+ # Always restore the original position, even if seeking to the end
+ # or reading it failed, so the reader is never left at EOF.
+ try:
+ reader.seek(pos)
+ except (OSError, ValueError, AttributeError):
+ pass
Review Comment:
Fixed — the finally restore now also swallows `TypeError` along with OSError/ValueError/AttributeError.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567510202
##########
lang/py/avro/io.py:
##########
@@ -599,6 +649,90 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 1 - 8 # Integer.MAX_VALUE - 8
+
+
+def _collection_limits() -> Tuple[int, int]:
+ """Return ``(zero_byte_limit, structural_limit)``.
+
+ ``AVRO_MAX_COLLECTION_ITEMS``, when set to a non-negative integer, caps both
+ zero-byte-element collections and all other collections at that value.
+ Otherwise zero-byte elements use the tighter :data:`DEFAULT_MAX_COLLECTION_ITEMS`
+ and all collections use :data:`DEFAULT_MAX_COLLECTION_STRUCTURAL`.
+ """
+ value = os.environ.get(MAX_COLLECTION_ITEMS_ENV)
+ if value is None:
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ try:
+ parsed = int(value)
+ except ValueError:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring invalid {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ if parsed < 0:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring negative {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ return parsed, parsed
+
+
+def _max_collection_items() -> int:
+ """Return the configured zero-byte-element collection limit."""
+ return _collection_limits()[0]
+
+
+def _min_bytes_per_element(schema: avro.schema.Schema, visited: Optional[Set[int]] = None) -> int:
+ """
+ Return the minimum number of bytes a single value of ``schema`` can occupy
+ on the wire. Used to reject an array/map block count that could not possibly
+ be backed by the bytes remaining. A type that can encode to zero bytes
+ (``null``) returns 0, which disables the collection check for it (avoiding a
+ false positive on, e.g., an array of nulls).
+ """
+ if visited is None:
+ visited = set()
+ schema_type = schema.type
+ if schema_type == "null":
+ return 0
+ if schema_type == "float":
+ return 4
+ if schema_type == "double":
+ return 8
+ if schema_type == "fixed":
+ return getattr(schema, "size", 1)
+ if schema_type in ("record", "error"):
+ # Guard against self-referencing records (recursion would not terminate).
+ if id(schema) in visited:
+ return 0
+ visited.add(id(schema))
+ total = 0
+ for field in getattr(schema, "fields", []):
+ total += _min_bytes_per_element(field.type, visited)
+ visited.discard(id(schema))
+ return total
+ # boolean, int, long, bytes, string, enum, union, array, map: all >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ return 1
Review Comment:
`_min_bytes_per_element()` treats `union` as always having a 1-byte minimum, but a union value is encoded as a branch index *plus* the selected branch payload. If all union branches have a positive minimum size (e.g., `["long","double"]`), returning `1` underestimates the true minimum and makes the bytes-remaining guard too permissive, allowing larger block counts than the remaining bytes could possibly back (reintroducing large allocation/iteration risk on truncated/hostile inputs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570456146
##########
lang/py/avro/errors.py:
##########
@@ -36,6 +36,10 @@ class InvalidAvroBinaryEncoding(AvroException):
"""For invalid numbers of bytes read."""
+class AvroCollectionSizeException(AvroException):
+ """Raised when a decoded array or map would exceed the configured item limit."""
Review Comment:
The new exception’s docstring says it’s raised when exceeding the “configured item limit”, but the code raises this for both the zero-byte element cap and the structural collection cap. Updating the wording avoids misleading users about what condition triggered the exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567551373
##########
lang/py/avro/io.py:
##########
@@ -599,6 +649,90 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 1 - 8 # Integer.MAX_VALUE - 8
+
+
+def _collection_limits() -> Tuple[int, int]:
+ """Return ``(zero_byte_limit, structural_limit)``.
+
+ ``AVRO_MAX_COLLECTION_ITEMS``, when set to a non-negative integer, caps both
+ zero-byte-element collections and all other collections at that value.
+ Otherwise zero-byte elements use the tighter :data:`DEFAULT_MAX_COLLECTION_ITEMS`
+ and all collections use :data:`DEFAULT_MAX_COLLECTION_STRUCTURAL`.
+ """
+ value = os.environ.get(MAX_COLLECTION_ITEMS_ENV)
+ if value is None:
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ try:
+ parsed = int(value)
+ except ValueError:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring invalid {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ if parsed < 0:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring negative {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ return parsed, parsed
+
+
+def _max_collection_items() -> int:
+ """Return the configured zero-byte-element collection limit."""
+ return _collection_limits()[0]
+
+
+def _min_bytes_per_element(schema: avro.schema.Schema, visited: Optional[Set[int]] = None) -> int:
+ """
+ Return the minimum number of bytes a single value of ``schema`` can occupy
+ on the wire. Used to reject an array/map block count that could not possibly
+ be backed by the bytes remaining. A type that can encode to zero bytes
+ (``null``) returns 0, which disables the collection check for it (avoiding a
+ false positive on, e.g., an array of nulls).
+ """
+ if visited is None:
+ visited = set()
+ schema_type = schema.type
+ if schema_type == "null":
+ return 0
+ if schema_type == "float":
+ return 4
+ if schema_type == "double":
+ return 8
+ if schema_type == "fixed":
+ return getattr(schema, "size", 1)
+ if schema_type in ("record", "error"):
+ # Guard against self-referencing records (recursion would not terminate).
+ if id(schema) in visited:
+ return 0
+ visited.add(id(schema))
+ total = 0
+ for field in getattr(schema, "fields", []):
+ total += _min_bytes_per_element(field.type, visited)
+ visited.discard(id(schema))
+ return total
+ # boolean, int, long, bytes, string, enum, union, array, map: all >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ return 1
Review Comment:
Fixed — _min_bytes_per_element now returns `1 + min(branch minimums)` for a union (the branch index plus the smallest branch payload), so a union whose branches all have a positive minimum is no longer underestimated as 1. It stays 1 when any branch is null/zero-byte.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567535733
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError, TypeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
Review Comment:
`bytes_remaining()` can return a negative value if the underlying stream is positioned past EOF (e.g., a seekable file-like object allows seeking beyond the end). This can produce confusing errors like “only -5 remain” and may cause large reads/collection checks to be rejected incorrectly. Clamp negative results to 0 so the method’s contract (“bytes still available to read”) stays non-negative.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567333230
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
+ except (OSError, ValueError, AttributeError):
+ return None
+ finally:
+ # Always restore the original position, even if seeking to the end
+ # or reading it failed, so the reader is never left at EOF.
+ try:
+ reader.seek(pos)
+ except (OSError, ValueError, AttributeError):
+ pass
Review Comment:
The `finally` restore path in `bytes_remaining()` can also raise `TypeError` (e.g., if `pos` is not a valid seek offset for a custom stream). Since this is best-effort cleanup, type errors should be swallowed the same way as `OSError`/`ValueError`/`AttributeError`.
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
+ except (OSError, ValueError, AttributeError):
+ return None
Review Comment:
`bytes_remaining()` can raise `TypeError` (e.g., if a non-standard file-like object returns a non-int from `tell()`, making `end - pos` invalid). This would escape as an unexpected exception and break decoding; the method should reliably fall back to returning `None` on type errors as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567430563
##########
lang/py/avro/io.py:
##########
@@ -832,23 +1016,37 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
Already addressed in commit c711ef1386: read_map now tracks a separate cumulative `items_read` counter (incremented by each block's count) and passes it to _ensure_collection_available instead of `len(read_items)`, so duplicate keys can't undercount. A duplicate-key regression test was added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567431240
##########
lang/py/avro/io.py:
##########
@@ -246,6 +286,11 @@ def read_long(self) -> int:
n = b & 0x7F
shift = 7
while (b & 0x80) != 0:
+ # A 64-bit value needs at most 10 bytes (shifts 0..63); reject an
+ # overlong varint rather than accepting a malformed, arbitrarily
+ # large value.
+ if shift >= 70:
+ raise avro.errors.InvalidAvroBinaryEncoding("Varint is too long")
b = ord(self.read(1))
n |= (b & 0x7F) << shift
shift += 7
Review Comment:
The new overlong-varint guard rejects encodings longer than 10 bytes, but a 10-byte varint can still represent values wider than 64 bits (the 10th byte must only carry 0 or 1 in its low 7 bits for a valid 64-bit zig-zag long). Without this, malformed inputs can still decode to out-of-range integers and drive extremely large length/count values.
##########
lang/py/avro/io.py:
##########
@@ -599,6 +644,90 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 8
Review Comment:
`DEFAULT_MAX_COLLECTION_STRUCTURAL` is documented as matching `Integer.MAX_VALUE - 8`, but `(1 << 31) - 8` evaluates to `2^31 - 8` (2147483640), which is one higher than `2^31 - 1 - 8` (2147483639). This off-by-one means the code and documentation/spec reference don’t align.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570527399
##########
lang/py/avro/errors.py:
##########
@@ -36,6 +36,10 @@ class InvalidAvroBinaryEncoding(AvroException):
"""For invalid numbers of bytes read."""
+class AvroCollectionSizeException(AvroException):
+ """Raised when a decoded array or map would exceed the configured item limit."""
Review Comment:
Fixed in 010a389: the docstring now states the exception covers both the zero-byte-element cap and the structural cap on the total number of elements, matching the two raise sites in `_ensure_collection_available`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba merged PR #3861:
URL: https://github.com/apache/avro/pull/3861
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567559186
##########
lang/py/avro/io.py:
##########
@@ -832,24 +1045,43 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, items_read, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_read += block_count
for i in range(block_count):
key = decoder.read_utf8()
read_items[key] = self.read_data(writers_schema.values, readers_schema.values, decoder)
block_count = decoder.read_long()
return read_items
def skip_map(self, writers_schema: avro.schema.MapSchema, decoder: BinaryDecoder) -> None:
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
- decoder.skip(block_size)
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
+ self._skip_block_bytes(decoder, block_size)
Review Comment:
Same issue as `skip_array()`: in the sized-block skip path the decoder seeks forward by the declared `block_size` without ensuring it can possibly contain `block_count` map entries (keys are >=1 byte plus the value’s minimum). A too-small `block_size` can leave the decoder positioned mid-block and cause subsequent reads to interpret leftover bytes as the next field/block header. Add a lower-bound check before skipping: for `min_bytes > 0`, reject when `block_count > block_size // min_bytes`.
##########
lang/py/avro/io.py:
##########
@@ -795,23 +978,53 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Reject both before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(
+ f"Block size {block_size} exceeds the {remaining} bytes remaining"
+ )
+ decoder.skip(block_size)
+
def skip_array(self, writers_schema: avro.schema.ArraySchema, decoder: BinaryDecoder) -> None:
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
- decoder.skip(block_size)
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
+ self._skip_block_bytes(decoder, block_size)
Review Comment:
In the sized-block skip path (negative block count), the code trusts the attacker-controlled `block_size` to reposition the decoder without verifying it can possibly contain `block_count` elements. If `block_size` is smaller than the minimum on-wire size for `block_count` elements, `skip_array()` can seek into the middle of the block payload and corrupt decoding of subsequent fields (and may accept malformed/truncated input when schema resolution skips this field). Add a lower-bound validation: for `min_bytes > 0`, reject when `block_count > block_size // min_bytes`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3569756934
##########
lang/py/avro/io.py:
##########
@@ -795,23 +994,55 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int, block_count: int, min_bytes: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Also require that block_size can plausibly hold block_count
+ elements at their minimum on-wire size, so a too-small size cannot
+ misalign the decoder. Reject before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} exceeds the {remaining} bytes remaining")
+ if min_bytes > 0 and block_count > block_size // min_bytes:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} is too small for {block_count} elements of >= {min_bytes} bytes")
+ decoder.skip(block_size)
Review Comment:
Fixed in 3eb04ed: `_skip_block_bytes()` now wraps `decoder.skip(block_size)` in a try/except that catches `OSError`/`ValueError`/`OverflowError` from the underlying reader's seek and re-raises them as `InvalidAvroBinaryEncoding`, so malformed sized blocks fail with a consistent Avro error. Added a `test_skip_block_bytes_wraps_seek_error` regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567579816
##########
lang/py/avro/io.py:
##########
@@ -795,23 +978,53 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Reject both before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(
+ f"Block size {block_size} exceeds the {remaining} bytes remaining"
+ )
+ decoder.skip(block_size)
+
def skip_array(self, writers_schema: avro.schema.ArraySchema, decoder: BinaryDecoder) -> None:
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
- decoder.skip(block_size)
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
+ self._skip_block_bytes(decoder, block_size)
Review Comment:
Fixed — _skip_block_bytes now also rejects a block_size that can't hold block_count elements at their minimum on-wire size (`block_count > block_size // min_bytes` when min_bytes > 0), so skip_array can't seek mid-element.
##########
lang/py/avro/io.py:
##########
@@ -832,24 +1045,43 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, items_read, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_read += block_count
for i in range(block_count):
key = decoder.read_utf8()
read_items[key] = self.read_data(writers_schema.values, readers_schema.values, decoder)
block_count = decoder.read_long()
return read_items
def skip_map(self, writers_schema: avro.schema.MapSchema, decoder: BinaryDecoder) -> None:
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
+ block_size = None
if block_count < 0:
+ block_count = -block_count
block_size = decoder.read_long()
- decoder.skip(block_size)
+ # Bound the (normalized) count on both the sized and unsized paths so
+ # a negative block count cannot bypass the collection limits.
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
+ items_skipped += block_count
+ if block_size is not None:
+ self._skip_block_bytes(decoder, block_size)
Review Comment:
Same fix applies to skip_map (min_bytes there includes the >=1-byte key), via the shared _skip_block_bytes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567637143
##########
lang/py/avro/io.py:
##########
@@ -832,24 +1054,43 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
Same as `read_array()`: the negative-count map block byte-size is currently skipped with `decoder.skip_long()`, which lacks the 10-byte/64-bit varint validation in `read_long()`. Reading it with `read_long()` (and discarding the value) ensures malformed overlong encodings are rejected rather than looped over.
##########
lang/py/avro/io.py:
##########
@@ -795,23 +981,59 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
In the negative-block-count path, the block byte-size is consumed via `decoder.skip_long()`, which does not enforce the new 10-byte/64-bit varint bounds from `read_long()`. A malformed overlong varint here can cause excessive looping/CPU and also bypasses strict encoding validation. Prefer consuming the value with `read_long()` (and ignore it) so malformed encodings are rejected consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565056559
##########
lang/py/avro/io.py:
##########
@@ -87,9 +87,10 @@
import collections
import datetime
import decimal
+import io
Review Comment:
## CodeQL / Module imports itself
The module 'io' imports itself.
The module 'avro.io' imports itself.
[Show more details](https://github.com/apache/avro/security/code-scanning/3370)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565214952
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,34 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ reader.seek(pos)
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position/size could not be determined.
+ return None
+ return end - pos
Review Comment:
`bytes_remaining()` can leave the underlying reader positioned at EOF if an exception occurs after `seek(0, SEEK_END)` (e.g., `tell()` or the restore `seek(pos)` fails). That can corrupt subsequent decoding. Also, the function assumes `tell()` returns an `int`; if it returns a non-int sentinel, `end - pos` will raise `TypeError`. Consider restoring the original position in a `finally` block and validating `pos`/`end` types before subtracting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3566596524
##########
lang/py/avro/io.py:
##########
@@ -795,24 +961,32 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
def skip_array(self, writers_schema: avro.schema.ArraySchema, decoder: BinaryDecoder) -> None:
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_size = decoder.read_long()
decoder.skip(block_size)
else:
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
`skip_array()` applies `_ensure_collection_available()` only for positive block counts. For negative block counts (the `-count` form with an explicit `block_size`), the code currently bypasses both the bytes-remaining validation and the cumulative item caps, so a malicious stream can evade the new collection limit checks when the decoder is skipping data (e.g., for schema resolution). Normalize negative counts and apply `_ensure_collection_available()`/`items_skipped` consistently before skipping the block bytes.
##########
lang/py/avro/io.py:
##########
@@ -832,25 +1006,34 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
key = decoder.read_utf8()
read_items[key] = self.read_data(writers_schema.values, readers_schema.values, decoder)
block_count = decoder.read_long()
return read_items
def skip_map(self, writers_schema: avro.schema.MapSchema, decoder: BinaryDecoder) -> None:
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_size = decoder.read_long()
decoder.skip(block_size)
else:
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
`skip_map()` has the same gap as `skip_array()`: negative block counts bypass `_ensure_collection_available()` and the cumulative `items_skipped` accounting. This makes the new size-limit enforcement inconsistent between read vs skip paths and allows the cap to be bypassed when skipping map blocks encoded with negative counts.
##########
lang/py/avro/test/test_io.py:
##########
@@ -715,6 +843,165 @@ def test_type_exception_record(self) -> None:
write_datum(datum_to_write, writers_schema)
+class TestDatumReaderCollectionSizeLimit(unittest.TestCase):
+ """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 them. A huge declared block count of such
+ elements is capped so a tiny payload cannot exhaust memory. The limit is
+ configurable via the ``AVRO_MAX_COLLECTION_ITEMS`` environment variable."""
+
+ @staticmethod
+ def _decode(schema_json: str, encoded: bytes) -> object:
+ schema = avro.schema.parse(schema_json)
+ reader = avro.io.DatumReader(schema)
+ with io.BytesIO(encoded) as bio:
+ return reader.read(avro.io.BinaryDecoder(bio))
+
+ @staticmethod
+ def _skip(schema_json: str, encoded: bytes) -> None:
+ schema = avro.schema.parse(schema_json)
+ reader = avro.io.DatumReader(schema)
+ with io.BytesIO(encoded) as bio:
+ reader.skip_data(schema, avro.io.BinaryDecoder(bio))
+
+ @staticmethod
+ def _array_block(count: int, *, negative: bool = False) -> bytes:
+ """One array/map block of ``count`` zero-byte elements + end marker."""
+ buf = io.BytesIO()
+ enc = avro.io.BinaryEncoder(buf)
+ if negative:
+ enc.write_long(-count) # negative count is followed by a block byte-size
+ enc.write_long(0)
+ else:
+ enc.write_long(count)
+ enc.write_long(0) # end-of-collection marker
+ return buf.getvalue()
+
+ def test_array_of_null_exceeds_default_limit(self) -> None:
+ # The reported exploit: a ~6 byte payload declaring 200,000,000 nulls.
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ self._array_block(200_000_000),
+ )
+
+ def test_array_of_null_within_configured_limit_still_reads(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ result = self._decode('{"type": "array", "items": "null"}', self._array_block(1000))
+ self.assertEqual(result, [None] * 1000)
+
+ def test_array_of_null_exceeds_configured_limit(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ self._array_block(1001),
+ )
+
+ def test_array_of_null_cumulative_across_blocks(self) -> None:
+ # Two blocks of 600 nulls each (1200 > 1000) must be rejected on the second.
+ buf = io.BytesIO()
+ enc = avro.io.BinaryEncoder(buf)
+ enc.write_long(600)
+ enc.write_long(600)
+ enc.write_long(0)
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ buf.getvalue(),
+ )
+
+ def test_array_of_null_negative_block_count(self) -> None:
+ # A negative count encodes abs(count) elements preceded by a block size;
+ # after normalization it must still be bounded.
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ self._array_block(200_000, negative=True),
+ )
+
+ def test_array_of_zero_length_fixed_exceeds_limit(self) -> None:
+ schema_json = '{"type": "array", "items": {"type": "fixed", "name": "empty", "size": 0}}'
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(avro.errors.AvroCollectionSizeException, self._decode, schema_json, self._array_block(2000))
+
+ def test_array_of_all_null_record_exceeds_limit(self) -> None:
+ schema_json = '{"type": "array", "items": {"type": "record", "name": "R", "fields": [{"name": "n", "type": "null"}]}}'
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(avro.errors.AvroCollectionSizeException, self._decode, schema_json, self._array_block(2000))
+
+ def test_map_of_null_rejected_by_available_bytes(self) -> None:
+ # Map entries always carry a >= 1 byte key, so a huge map<null> is bounded
+ # by the bytes-remaining check rather than the zero-byte cap.
+ self.assertRaises(
+ avro.errors.InvalidAvroBinaryEncoding,
+ self._decode,
+ '{"type": "map", "values": "null"}',
+ self._array_block(200_000_000),
+ )
+
+ def test_skip_array_of_null_respects_limit(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._skip,
+ '{"type": "array", "items": "null"}',
+ self._array_block(2000),
+ )
+
+ def test_invalid_env_override_falls_back_to_default(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "not-a-number"}):
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ self.assertEqual(avro.io._max_collection_items(), avro.io.DEFAULT_MAX_COLLECTION_ITEMS)
+
+ def test_negative_env_override_falls_back_to_default(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "-5"}):
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ self.assertEqual(avro.io._max_collection_items(), avro.io.DEFAULT_MAX_COLLECTION_ITEMS)
+
+ def test_env_override_is_honored(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "7"}):
+ self.assertEqual(avro.io._max_collection_items(), 7)
+ result = self._decode('{"type": "array", "items": "null"}', self._array_block(7))
+ self.assertEqual(result, [None] * 7)
+
+ def test_collection_limits_env_caps_both(self) -> None:
+ # When set, AVRO_MAX_COLLECTION_ITEMS caps both the zero-byte and the
+ # structural limit; unset, they differ (tighter zero-byte default).
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1234"}):
+ self.assertEqual(avro.io._collection_limits(), (1234, 1234))
+ os.environ.pop("AVRO_MAX_COLLECTION_ITEMS", None)
+ self.assertEqual(
+ avro.io._collection_limits(),
+ (avro.io.DEFAULT_MAX_COLLECTION_ITEMS, avro.io.DEFAULT_MAX_COLLECTION_STRUCTURAL),
+ )
Review Comment:
`test_collection_limits_env_caps_both()` mutates the real process environment via `os.environ.pop(...)` outside of a patch context. If a developer (or CI) runs tests with `AVRO_MAX_COLLECTION_ITEMS` already set, this test will delete it for the remainder of the test run, potentially causing unrelated tests to behave differently. Save/restore the original value (or keep the unset assertion inside a temporary env override) so the test is hermetic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567560563
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,43 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ except (OSError, ValueError, AttributeError, TypeError):
+ # Not seekable, or the position could not be determined.
+ return None
+ try:
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ return end - pos
Review Comment:
Fixed — bytes_remaining() now returns `max(0, end - pos)`, so a stream seeked past EOF reports 0 remaining rather than a negative count.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3568126296
##########
lang/py/avro/io.py:
##########
@@ -383,8 +436,15 @@ def skip_int(self) -> None:
def skip_long(self) -> None:
b = ord(self.read(1))
+ count = 1
while (b & 0x80) != 0:
+ # A 64-bit varint is at most 10 bytes; reject an overlong chain so a
+ # skipped long can't force scanning unbounded input (read_long caps
+ # the same way).
+ if count >= 10:
+ raise avro.errors.InvalidAvroBinaryEncoding("Varint is too long")
b = ord(self.read(1))
+ count += 1
Review Comment:
`skip_long()` now caps the number of bytes read, but it still doesn’t enforce the same 64-bit varint validity as `read_long()` (specifically the “10th byte must not carry payload bits above bit 63” check). That means malformed 10-byte varints that `read_long()` would reject can still be silently accepted when skipping (e.g., negative-block byte sizes in `read_array`/`read_map`). Consider reusing `read_long()` here so skip paths reject the same invalid encodings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3568152957
##########
lang/py/avro/io.py:
##########
@@ -383,8 +436,15 @@ def skip_int(self) -> None:
def skip_long(self) -> None:
b = ord(self.read(1))
+ count = 1
while (b & 0x80) != 0:
+ # A 64-bit varint is at most 10 bytes; reject an overlong chain so a
+ # skipped long can't force scanning unbounded input (read_long caps
+ # the same way).
+ if count >= 10:
+ raise avro.errors.InvalidAvroBinaryEncoding("Varint is too long")
b = ord(self.read(1))
+ count += 1
Review Comment:
Fixed — skip_long now tracks shift and applies the same 10th-byte check as read_long (`shift == 63 and (b & 0x7E) != 0` raises InvalidAvroBinaryEncoding), so a malformed 10-byte varint is rejected on the skip path too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570642275
##########
lang/py/avro/io.py:
##########
@@ -599,6 +665,97 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the maximum number of zero-byte-encoded
+#: collection elements (e.g. an array of nulls) to allocate from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
Review Comment:
Fixed in 152b60b: the `MAX_COLLECTION_ITEMS_ENV` comment now states that `AVRO_MAX_COLLECTION_ITEMS` caps both the zero-byte-element limit and the structural limit, matching `_collection_limits()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567396688
##########
lang/py/avro/io.py:
##########
@@ -832,23 +1016,37 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
Fixed — read_map now tracks a cumulative `items_read` counter (incremented by each block's count) and uses it for the limit check instead of `len(read_items)`, so duplicate keys can no longer undercount and bypass the cap. Added a duplicate-key regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on PR #3861:
URL: https://github.com/apache/avro/pull/3861#issuecomment-5019895781
Thanks for the detailed benchmarking, @arib06. Applied both suggestions:
- `_ensure_collection_available` now only calls `bytes_remaining()` when the block count exceeds a `_MAX_UNCHECKED_COLLECTION` threshold (1024), mirroring `_MAX_UNCHECKED_READ` in `read()`; the structural cap stays unconditional with no seek. As you noted this doesn't weaken the bound — with `min_bytes > 0` an under-threshold block still has to be paid for in real wire bytes, and the zero-byte path is untouched and keeps its tighter cumulative cap. Added a regression test for a small truncated collection to lock that in.
- Expanded the `_collection_limits()` docstring to call out explicitly that `AVRO_MAX_COLLECTION_ITEMS` pins both limits to the same value, so raising the zero-byte limit also lowers the structural cap (and how to avoid that by setting it above `DEFAULT_MAX_COLLECTION_STRUCTURAL`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567380249
##########
lang/py/avro/io.py:
##########
@@ -832,23 +1016,37 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
`read_map()` passes `len(read_items)` into `_ensure_collection_available()`, but `len(read_items)` counts only unique keys. Avro maps can legally contain duplicate keys (later entries overwrite earlier ones), so this can undercount the number of decoded key/value pairs and allow the structural limit to be exceeded (especially for non-seekable decoders where the bytes-remaining check can’t run). Track a separate cumulative `items_read` counter (like `skip_map()` does) and use that for the limit check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on PR #3861:
URL: https://github.com/apache/avro/pull/3861#issuecomment-4952729964
**Preallocation (AVRO-4292 follow-up) evaluated — no change needed here.** `read_array`/`read_map` build `read_items`/the dict via `.append()`, growing on demand rather than preallocating to the declared block count, so there is no up-front over-allocation to bound (unlike the Java/C++/C# readers, which sized a buffer to the count). The existing structural and zero-byte caps already bound the growth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570729511
##########
lang/py/avro/io.py:
##########
@@ -599,6 +665,99 @@ def write_timestamp_micros_long(self, datum: datetime.datetime) -> None:
#
# DatumReader/Writer
#
+#: Environment variable overriding the collection element limits. When set to a
+#: non-negative integer it caps both the number of zero-byte-encoded collection
+#: elements (e.g. an array of nulls) and the structural cap on the total number of
+#: elements in any collection, allocated from a single decode.
+MAX_COLLECTION_ITEMS_ENV = "AVRO_MAX_COLLECTION_ITEMS"
+
+#: Default maximum number of zero-byte-encoded collection elements to allocate.
+#: 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; without a cap a tiny payload can declare a huge
+#: block count and exhaust memory. A legitimate collection of zero-byte elements is
+#: small, so this default is generous while still rejecting pathological input. It
+#: can be raised (or lowered) with the ``AVRO_MAX_COLLECTION_ITEMS`` environment
+#: variable.
+DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000
+
+#: Default structural cap on the number of elements in any array or map (a
+#: collection larger than this is treated as malformed regardless of element
+#: type). Matches the historical ``Integer.MAX_VALUE - 8`` limit. Elements with a
+#: positive on-wire size are also bounded by the bytes remaining; this cap is an
+#: additional overflow/defense-in-depth guard. ``AVRO_MAX_COLLECTION_ITEMS``, when
+#: set, overrides both this and :data:`DEFAULT_MAX_COLLECTION_ITEMS`.
+DEFAULT_MAX_COLLECTION_STRUCTURAL = (1 << 31) - 1 - 8 # Integer.MAX_VALUE - 8
+
+
+def _collection_limits() -> Tuple[int, int]:
+ """Return ``(zero_byte_limit, structural_limit)``.
+
+ ``AVRO_MAX_COLLECTION_ITEMS``, when set to a non-negative integer, caps both
+ zero-byte-element collections and all other collections at that value.
+ Otherwise zero-byte elements use the tighter :data:`DEFAULT_MAX_COLLECTION_ITEMS`
+ and all collections use :data:`DEFAULT_MAX_COLLECTION_STRUCTURAL`.
+ """
+ value = os.environ.get(MAX_COLLECTION_ITEMS_ENV)
+ if value is None:
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ try:
+ parsed = int(value)
+ except ValueError:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring invalid {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ if parsed < 0:
+ warnings.warn(avro.errors.AvroWarning(f"Ignoring negative {MAX_COLLECTION_ITEMS_ENV} value: {value!r}"))
+ return DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL
+ return parsed, parsed
+
+
+def _max_collection_items() -> int:
+ """Return the configured zero-byte-element collection limit."""
+ return _collection_limits()[0]
+
+
+def _min_bytes_per_element(schema: avro.schema.Schema, visited: Optional[Set[int]] = None) -> int:
+ """
+ Return the minimum number of bytes a single value of ``schema`` can occupy
+ on the wire. Used to reject an array/map block count that could not possibly
+ be backed by the bytes remaining. A type that can encode to zero bytes
+ (``null``) returns 0, which disables the collection check for it (avoiding a
+ false positive on, e.g., an array of nulls).
+ """
+ if visited is None:
+ visited = set()
+ schema_type = schema.type
+ if schema_type == "null":
+ return 0
+ if schema_type == "float":
+ return 4
+ if schema_type == "double":
+ return 8
+ if schema_type == "fixed":
+ return getattr(schema, "size", 1)
+ if schema_type in ("record", "error"):
+ # Guard against self-referencing records (recursion would not terminate).
+ if id(schema) in visited:
+ return 0
Review Comment:
`_min_bytes_per_element()` returns `0` when it detects a recursive record/error schema (`id(schema) in visited`). A recursive reference is not actually a zero-byte-encoded value, so returning 0 can (a) incorrectly treat recursive records as “zero-byte elements” and apply the wrong cap, and (b) underestimate the minimum bytes for unions that include recursive records, weakening the bytes-remaining precheck and potentially allowing large truncated allocations/iterations.
Return a non-zero conservative lower bound (e.g., 1 byte) when recursion is detected so recursive records are never treated as zero-byte types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3570933260
##########
lang/py/avro/io.py:
##########
@@ -795,23 +1001,61 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
+ @staticmethod
+ def _skip_block_bytes(decoder: BinaryDecoder, block_size: int, block_count: int, min_bytes: int) -> None:
+ """Skip a sized-block's byte count, rejecting malformed sizes.
+
+ The block_size is attacker-controlled: a negative value would seek
+ backwards and an oversized value past EOF, either corrupting the decoder
+ position. Also require that block_size can plausibly hold block_count
+ elements at their minimum on-wire size, so a too-small size cannot
+ misalign the decoder. Reject before skipping.
+ """
+ if block_size < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Invalid negative block size: {block_size}")
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and block_size > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} exceeds the {remaining} bytes remaining")
+ if min_bytes > 0 and block_count > block_size // min_bytes:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Block size {block_size} is too small for {block_count} elements of >= {min_bytes} bytes")
Review Comment:
_skip_block_bytes() skips the declared block_size even when min_bytes==0 (always-zero-byte element types like null / fixed size 0 / empty records). In that case the only valid sized-block payload size is 0; allowing a positive block_size can skip into subsequent fields and corrupt decoder alignment during skip_data/schema resolution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565239808
##########
lang/py/avro/io.py:
##########
@@ -214,11 +222,34 @@ def read(self, n: int) -> bytes:
"""
if n < 0:
raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, expected positive integer.")
+ if n > self._MAX_UNCHECKED_READ:
+ remaining = self.bytes_remaining()
+ if remaining is not None and n > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes to read, but only {remaining} remain.")
read_bytes = self.reader.read(n)
if len(read_bytes) != n:
raise avro.errors.InvalidAvroBinaryEncoding(f"Read {len(read_bytes)} bytes, expected {n} bytes")
return read_bytes
+ def bytes_remaining(self) -> Optional[int]:
+ """
+ Return the number of bytes still available to read, or ``None`` when
+ that count is not known (a non-seekable reader, or one whose position
+ cannot be obtained). Used to reject a declared length or collection
+ block count that exceeds the data actually available before allocating
+ for it.
+ """
+ reader = self.reader
+ try:
+ pos = reader.tell()
+ reader.seek(0, os.SEEK_END)
+ end = reader.tell()
+ reader.seek(pos)
+ except (OSError, ValueError, AttributeError):
+ # Not seekable, or the position/size could not be determined.
+ return None
+ return end - pos
Review Comment:
Fixed in b891ac3: bytes_remaining() now restores the original position in a finally block, so a failure after seeking to the end never leaves the reader at EOF. Added tests for the restore on both success and failure. (The isinstance type-validation was dropped because reader is typed IO[bytes], so tell() returns int and mypy flags the check as unreachable; the try/except covers the runtime failure paths.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3567642666
##########
lang/py/avro/io.py:
##########
@@ -795,23 +981,59 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
Fixed — skip_long now enforces the same 10-byte varint cap as read_long (raising InvalidAvroBinaryEncoding), so skipping the negative-block byte-size can no longer scan an unbounded malformed varint.
##########
lang/py/avro/io.py:
##########
@@ -832,24 +1054,43 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ # Track decoded pairs separately: len(read_items) counts unique keys, so
+ # duplicate keys (later entries overwrite earlier ones) would undercount
+ # and let a multi-block map exceed the cumulative caps.
+ items_read = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
Review Comment:
Same fix — skip_long (used for the map negative-block byte-size) is now bounded to 10 bytes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3565150336
##########
lang/py/avro/io.py:
##########
@@ -87,9 +87,10 @@
import collections
import datetime
import decimal
+import io
Review Comment:
Fixed in 957d180: avro/io.py no longer imports the standard library 'io' (which CodeQL flagged as a self-import). It imports 'os' and uses os.SEEK_END instead (both equal 2).
##########
lang/py/avro/io.py:
##########
@@ -780,6 +841,22 @@ def read_enum(self, writers_schema: avro.schema.EnumSchema, readers_schema: avro
def skip_enum(self, writers_schema: avro.schema.EnumSchema, decoder: BinaryDecoder) -> None:
return decoder.skip_int()
+ @staticmethod
+ def _ensure_collection_available(decoder: BinaryDecoder, count: int, min_bytes_per_element: int) -> None:
+ """
+ Reject a collection (array or map) block whose declared element count
+ could not be backed by the bytes actually remaining, before iterating.
+ Skipped when the per-element minimum is zero (e.g. an array of nulls) or
+ when the reader cannot report how many bytes remain.
+ """
+ if count <= 0 or min_bytes_per_element <= 0:
+ return
+ remaining = decoder.bytes_remaining()
+ if remaining is not None and count * min_bytes_per_element > remaining:
+ raise avro.errors.InvalidAvroBinaryEncoding(
+ f"Collection claims {count} elements with at least {min_bytes_per_element} bytes each, but only {remaining} bytes are available."
+ )
Review Comment:
Fixed in 957d180: _ensure_collection_available now compares count against remaining // min_bytes_per_element instead of multiplying, avoiding a large intermediate product for an attacker-controlled count.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4296: [python] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3566776645
##########
lang/py/avro/io.py:
##########
@@ -795,24 +961,32 @@ def read_array(self, writers_schema: avro.schema.ArraySchema, readers_schema: av
The actual count in this case
is the absolute value of the count written.
"""
- read_items = []
+ read_items: List[object] = []
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
read_items.append(self.read_data(writers_schema.items, readers_schema.items, decoder))
block_count = decoder.read_long()
return read_items
def skip_array(self, writers_schema: avro.schema.ArraySchema, decoder: BinaryDecoder) -> None:
+ min_bytes = _min_bytes_per_element(writers_schema.items)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_size = decoder.read_long()
decoder.skip(block_size)
else:
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
Fixed in babb0504f0: skip_array normalizes a negative block count and applies _ensure_collection_available / items_skipped on both the sized and unsized paths.
##########
lang/py/avro/io.py:
##########
@@ -832,25 +1006,34 @@ def read_map(self, writers_schema: avro.schema.MapSchema, readers_schema: avro.s
The actual count in this case
is the absolute value of the count written.
"""
- read_items = {}
+ read_items: Dict[str, object] = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_count = -block_count
decoder.skip_long()
+ self._ensure_collection_available(decoder, len(read_items), block_count, min_bytes, zero_byte_limit, structural_limit)
for i in range(block_count):
key = decoder.read_utf8()
read_items[key] = self.read_data(writers_schema.values, readers_schema.values, decoder)
block_count = decoder.read_long()
return read_items
def skip_map(self, writers_schema: avro.schema.MapSchema, decoder: BinaryDecoder) -> None:
+ min_bytes = 1 + _min_bytes_per_element(writers_schema.values)
+ zero_byte_limit, structural_limit = _collection_limits()
+ items_skipped = 0
block_count = decoder.read_long()
while block_count != 0:
if block_count < 0:
block_size = decoder.read_long()
decoder.skip(block_size)
else:
+ self._ensure_collection_available(decoder, items_skipped, block_count, min_bytes, zero_byte_limit, structural_limit)
Review Comment:
Fixed in babb0504f0: skip_map got the same treatment. Added a regression test for a negative-count zero-byte skip block.
##########
lang/py/avro/test/test_io.py:
##########
@@ -715,6 +843,165 @@ def test_type_exception_record(self) -> None:
write_datum(datum_to_write, writers_schema)
+class TestDatumReaderCollectionSizeLimit(unittest.TestCase):
+ """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 them. A huge declared block count of such
+ elements is capped so a tiny payload cannot exhaust memory. The limit is
+ configurable via the ``AVRO_MAX_COLLECTION_ITEMS`` environment variable."""
+
+ @staticmethod
+ def _decode(schema_json: str, encoded: bytes) -> object:
+ schema = avro.schema.parse(schema_json)
+ reader = avro.io.DatumReader(schema)
+ with io.BytesIO(encoded) as bio:
+ return reader.read(avro.io.BinaryDecoder(bio))
+
+ @staticmethod
+ def _skip(schema_json: str, encoded: bytes) -> None:
+ schema = avro.schema.parse(schema_json)
+ reader = avro.io.DatumReader(schema)
+ with io.BytesIO(encoded) as bio:
+ reader.skip_data(schema, avro.io.BinaryDecoder(bio))
+
+ @staticmethod
+ def _array_block(count: int, *, negative: bool = False) -> bytes:
+ """One array/map block of ``count`` zero-byte elements + end marker."""
+ buf = io.BytesIO()
+ enc = avro.io.BinaryEncoder(buf)
+ if negative:
+ enc.write_long(-count) # negative count is followed by a block byte-size
+ enc.write_long(0)
+ else:
+ enc.write_long(count)
+ enc.write_long(0) # end-of-collection marker
+ return buf.getvalue()
+
+ def test_array_of_null_exceeds_default_limit(self) -> None:
+ # The reported exploit: a ~6 byte payload declaring 200,000,000 nulls.
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ self._array_block(200_000_000),
+ )
+
+ def test_array_of_null_within_configured_limit_still_reads(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ result = self._decode('{"type": "array", "items": "null"}', self._array_block(1000))
+ self.assertEqual(result, [None] * 1000)
+
+ def test_array_of_null_exceeds_configured_limit(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ self._array_block(1001),
+ )
+
+ def test_array_of_null_cumulative_across_blocks(self) -> None:
+ # Two blocks of 600 nulls each (1200 > 1000) must be rejected on the second.
+ buf = io.BytesIO()
+ enc = avro.io.BinaryEncoder(buf)
+ enc.write_long(600)
+ enc.write_long(600)
+ enc.write_long(0)
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ buf.getvalue(),
+ )
+
+ def test_array_of_null_negative_block_count(self) -> None:
+ # A negative count encodes abs(count) elements preceded by a block size;
+ # after normalization it must still be bounded.
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._decode,
+ '{"type": "array", "items": "null"}',
+ self._array_block(200_000, negative=True),
+ )
+
+ def test_array_of_zero_length_fixed_exceeds_limit(self) -> None:
+ schema_json = '{"type": "array", "items": {"type": "fixed", "name": "empty", "size": 0}}'
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(avro.errors.AvroCollectionSizeException, self._decode, schema_json, self._array_block(2000))
+
+ def test_array_of_all_null_record_exceeds_limit(self) -> None:
+ schema_json = '{"type": "array", "items": {"type": "record", "name": "R", "fields": [{"name": "n", "type": "null"}]}}'
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(avro.errors.AvroCollectionSizeException, self._decode, schema_json, self._array_block(2000))
+
+ def test_map_of_null_rejected_by_available_bytes(self) -> None:
+ # Map entries always carry a >= 1 byte key, so a huge map<null> is bounded
+ # by the bytes-remaining check rather than the zero-byte cap.
+ self.assertRaises(
+ avro.errors.InvalidAvroBinaryEncoding,
+ self._decode,
+ '{"type": "map", "values": "null"}',
+ self._array_block(200_000_000),
+ )
+
+ def test_skip_array_of_null_respects_limit(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1000"}):
+ self.assertRaises(
+ avro.errors.AvroCollectionSizeException,
+ self._skip,
+ '{"type": "array", "items": "null"}',
+ self._array_block(2000),
+ )
+
+ def test_invalid_env_override_falls_back_to_default(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "not-a-number"}):
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ self.assertEqual(avro.io._max_collection_items(), avro.io.DEFAULT_MAX_COLLECTION_ITEMS)
+
+ def test_negative_env_override_falls_back_to_default(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "-5"}):
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ self.assertEqual(avro.io._max_collection_items(), avro.io.DEFAULT_MAX_COLLECTION_ITEMS)
+
+ def test_env_override_is_honored(self) -> None:
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "7"}):
+ self.assertEqual(avro.io._max_collection_items(), 7)
+ result = self._decode('{"type": "array", "items": "null"}', self._array_block(7))
+ self.assertEqual(result, [None] * 7)
+
+ def test_collection_limits_env_caps_both(self) -> None:
+ # When set, AVRO_MAX_COLLECTION_ITEMS caps both the zero-byte and the
+ # structural limit; unset, they differ (tighter zero-byte default).
+ with unittest.mock.patch.dict(os.environ, {"AVRO_MAX_COLLECTION_ITEMS": "1234"}):
+ self.assertEqual(avro.io._collection_limits(), (1234, 1234))
+ os.environ.pop("AVRO_MAX_COLLECTION_ITEMS", None)
+ self.assertEqual(
+ avro.io._collection_limits(),
+ (avro.io.DEFAULT_MAX_COLLECTION_ITEMS, avro.io.DEFAULT_MAX_COLLECTION_STRUCTURAL),
+ )
Review Comment:
Fixed in babb0504f0: the 'unset' assertion now runs inside a patch.dict(os.environ) snapshot, so a pre-set AVRO_MAX_COLLECTION_ITEMS is restored and the test no longer mutates the real environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org