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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
