AVRO-4297: [ruby] Bound allocation when decoding length-prefixed values and collections#3862
AVRO-4297: [ruby] Bound allocation when decoding length-prefixed values and collections#3862iemejia wants to merge 18 commits into
Conversation
…th-prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data. - BinaryDecoder#bytes_remaining reports the bytes still readable when the reader can report its size (else nil). #read uses it to reject an over-large declared length above a threshold before allocating. - DatumReader#read_array/#read_map reject a block whose element count could not be backed by the bytes remaining, using min_bytes_per_element computed from the element schema so a zero-byte element type (e.g. null) is not falsely rejected. Mirrors the Java SDK's checks (AVRO-4241). Readers that cannot report their size are unaffected. Assisted-by: GitHub Copilot:claude-opus-4.8
The Lint CI step (rubocop) flagged three offenses in the new code: - Lint/IneffectiveAccessModifier: 'private' does not apply to a 'def self.' singleton method. Make min_bytes_per_element a private instance method (it uses no class state) and call it unqualified. - Lint/HashCompareByIdentity: use a hash with compare_by_identity and the schema object as the key instead of keying on schema.object_id. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens the Ruby Avro binary decoding path against malicious/truncated inputs that declare huge length/count prefixes by validating declared sizes against bytes remaining (when the underlying reader can report its size) before performing large allocations.
Changes:
- Added
BinaryDecoder#bytes_remainingand a thresholded pre-allocation check inBinaryDecoder#readfor large length-prefixed reads. - Added array/map block pre-validation in
DatumReader#read_arrayand#read_mapusing a computed minimum on-wire size per element schema. - Added Ruby tests covering over-limit bytes/array/map rejections and ensuring arrays of
nullare not falsely rejected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/ruby/lib/avro/io.rb | Adds bytes-remaining reporting and enforces length/count sanity checks to prevent oversized allocations during decoding. |
| lang/ruby/test/test_io.rb | Adds regression tests for rejecting over-declared lengths/counts while preserving valid decoding behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def read(len) | ||
| # Read n bytes | ||
| # Read n bytes. Reject a declared length that exceeds the bytes | ||
| # actually remaining before allocating for it, to guard against an | ||
| # out-of-memory attack from a malicious or truncated input. The check | ||
| # is only applied to larger reads; smaller reads and stream readers that |
There was a problem hiding this comment.
Fixed in d4eec59: BinaryDecoder#read now rejects a negative length (Ruby IO#read(-1) reads the rest of the stream, which would bypass the size check).
| when :float then 4 | ||
| when :double then 8 | ||
| when :fixed then schema.size | ||
| when :record, :error |
There was a problem hiding this comment.
Fixed in d4eec59: min_bytes_per_element now treats :request like :record/:error, so an array/map of request values whose fields encode to zero bytes is not falsely rejected.
…cord Review feedback: - BinaryDecoder#read now rejects a negative length. Ruby IO#read(-1) reads the rest of the stream, which would bypass the size check and allocate without bound. - min_bytes_per_element treats :request like :record/:error (request schemas are record-like), so an array/map of request values whose fields encode to zero bytes is not falsely rejected. Assisted-by: GitHub Copilot:claude-opus-4.8
| def bytes_remaining | ||
| return nil unless @reader.respond_to?(:size) && @reader.respond_to?(:tell) | ||
| @reader.size - @reader.tell | ||
| end |
There was a problem hiding this comment.
Fixed in 34859b4: bytes_remaining now checks that #size/#tell return Integers and rescues IOError/SystemCallError/NotImplementedError, returning nil when the size cannot be reported.
| return if count <= 0 || min_bytes_per_element <= 0 | ||
| remaining = decoder.bytes_remaining | ||
| if remaining && count * min_bytes_per_element > remaining | ||
| raise AvroError, "Collection claims #{count} elements with at least #{min_bytes_per_element} bytes each, but only #{remaining} bytes are available" | ||
| end |
There was a problem hiding this comment.
Fixed in 34859b4: ensure_collection_available compares count against remaining / min_bytes_per_element instead of multiplying, avoiding a large intermediate Integer.
Review feedback: - bytes_remaining now verifies #size/#tell return Integers and rescues IOError/SystemCallError/NotImplementedError, returning nil when the remaining size cannot be reported, so reads stay unaffected in that case. - ensure_collection_available compares count against remaining / min_bytes_per_element rather than multiplying, avoiding a large intermediate Integer for an attacker-controlled count. Assisted-by: GitHub Copilot:claude-opus-4.8
| size = @reader.size | ||
| pos = @reader.tell | ||
| return nil unless size.is_a?(Integer) && pos.is_a?(Integer) | ||
| size - pos |
There was a problem hiding this comment.
Fixed in ea1ac32: bytes_remaining clamps a negative result to 0 via [size - pos, 0].max, so a truncated file yields 'no bytes available' instead of a negative count.
| block_count = -block_count | ||
| _block_size = decoder.read_long | ||
| end | ||
| ensure_collection_available(decoder, block_count, min_bytes_per_element(writers_schema.items)) |
There was a problem hiding this comment.
Fixed in ea1ac32: read_array computes the per-element minimum once per invocation and reuses it across blocks.
| _block_size = decoder.read_long | ||
| end | ||
| # Map keys are strings (>= 1 byte length prefix) plus the value. | ||
| ensure_collection_available(decoder, block_count, 1 + min_bytes_per_element(writers_schema.values)) |
There was a problem hiding this comment.
Fixed in ea1ac32: read_map computes 1 + min-bytes once per invocation and reuses it across blocks.
…nimum Review feedback: - bytes_remaining clamps a negative result (size smaller than the current position, e.g. a truncated file) to 0 with [size - pos, 0].max, so callers see no bytes available rather than a confusing negative count. - read_array/read_map compute the per-element minimum once per invocation and reuse it across blocks, avoiding repeated schema traversal on the hot path. Assisted-by: GitHub Copilot:claude-opus-4.8
| def ensure_collection_available(decoder, count, min_bytes_per_element) | ||
| return if count <= 0 || min_bytes_per_element <= 0 | ||
| remaining = decoder.bytes_remaining | ||
| # Compare via integer division rather than multiplying, so a huge count | ||
| # does not create a large intermediate product. | ||
| if remaining && count > remaining / min_bytes_per_element | ||
| raise AvroError, "Collection claims #{count} elements with at least #{min_bytes_per_element} bytes each, but only #{remaining} bytes are available" | ||
| end | ||
| end |
There was a problem hiding this comment.
Fixed in 1186853: ensure_collection_available now guards the call with respond_to?(:bytes_remaining) and skips the check for a decoder that does not implement it, instead of raising NoMethodError.
…s_remaining Review feedback: a decoder that implements the read protocol but not #bytes_remaining (e.g. a custom decoder) would raise NoMethodError. Guard the call with respond_to?(:bytes_remaining) and skip the check for such a decoder instead of raising. Assisted-by: GitHub Copilot:claude-opus-4.8
| end | ||
|
|
||
| def test_read_bytes_within_stream_still_reads | ||
| payload = 'x' * (2 * 1024 * 1024) |
There was a problem hiding this comment.
Fixed in 1a3d3da: the payload is now 'x' * (Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1), so it still exercises the large-read path but with less time and memory.
| # report its size, that is rejected before allocating for it. | ||
| def test_read_bytes_rejects_length_beyond_stream | ||
| writer = StringIO.new | ||
| Avro::IO::BinaryEncoder.new(writer).write_long(100 * 1024 * 1024) |
There was a problem hiding this comment.
Fixed in 1a3d3da: the declared length is now Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1, tying the test directly to the threshold under test instead of a hard-coded 100MiB constant.
| count = 100_000 | ||
| writer = StringIO.new | ||
| encoder = Avro::IO::BinaryEncoder.new(writer) | ||
| encoder.write_long(count) # one block of `count` nulls (zero bytes each) | ||
| encoder.write_long(0) # end-of-array marker | ||
| result = decode('{"type":"array","items":"null"}', writer.string) | ||
| assert_equal([nil] * count, result) | ||
| end |
There was a problem hiding this comment.
Fixed in 1a3d3da: the test now asserts the decoded length and spot-checks the first and last elements are nil, instead of building a second large [nil] * count array.
… null assertion Review feedback: - Derive the oversized declared length and the within-limit payload from BinaryDecoder::MAX_UNCHECKED_READ + 1 instead of hard-coded 100MiB/2MiB constants, so the tests stay tied to the threshold under test and use less time and memory. - Assert the decoded array length and spot-check a couple of nil elements instead of building a second large [nil] * count array. Assisted-by: GitHub Copilot:claude-opus-4.8
Completes the available-bytes protection for collections. Elements whose schema
encodes to zero bytes (null, or a record with only zero-byte fields) consume no
input, so the bytes-remaining check cannot bound their count. A tiny payload
declaring a huge array block count of such elements (e.g.
{"type":"array","items":"null"} with a count of 200,000,000) therefore drove an
unbounded allocation.
ensure_collection_available now tracks the cumulative count across blocks and
enforces, per block: a structural cap on all collections
(DEFAULT_MAX_COLLECTION_STRUCTURAL = Integer.MAX_VALUE - 8, also covering
readers that cannot report their remaining bytes); a zero-byte item cap
(DEFAULT_MAX_COLLECTION_ITEMS = 10,000,000) when the per-element minimum is
zero; and the existing bytes-remaining check otherwise. AVRO_MAX_COLLECTION_ITEMS,
when set, caps both. read_array/read_map and the skip path (skip_blocks, used by
skip_array/skip_map) are all bounded the same way, so skipping a huge zero-byte
block cannot loop unboundedly. Rejections raise the dedicated
Avro::IO::CollectionSizeError (a subclass of AvroError).
Assisted-by: GitHub Copilot:claude-opus-4.8
iemejia
changed the title
read_long rejects overlong varints, but skip_long (used when projecting away a long field) looped over continuation bytes with no cap, so a malicious payload could force scanning unbounded input (CPU DoS) while skipping. Apply the same 10-byte limit and raise AvroError past it. Assisted-by: GitHub Copilot:claude-opus-4.8
read_long capped varints at 10 bytes, but a 10-byte encoding can still set bits beyond bit 63. The 10th byte contributes only bit 63, so reject it when any higher payload bit (b & 0x7E) is set. Assisted-by: GitHub Copilot:claude-opus-4.8
| def skip_long | ||
| b = byte! | ||
| count = 1 | ||
| while (b & 0x80) != 0 | ||
| # A 64-bit varint is at most 10 bytes; reject an overlong continuation | ||
| # chain so a skipped long can't force scanning unbounded input. | ||
| raise AvroError, "Varint is too long" if count >= 10 | ||
| b = byte! | ||
| count += 1 | ||
| end |
There was a problem hiding this comment.
Fixed — skip_long now rejects a 10th byte with payload bits above bit 63 (count == 10 && (b & 0x7E) != 0), consistent with read_long.
| if block_count < 0 | ||
| block_count = -block_count | ||
| _block_size = decoder.read_long | ||
| end | ||
| total = ensure_collection_available(decoder, total, block_count, min_bytes) |
There was a problem hiding this comment.
Fixed — read_array now raises on a negative per-block byte-size.
| if block_count < 0 | ||
| block_count = -block_count | ||
| _block_size = decoder.read_long | ||
| end | ||
| total = ensure_collection_available(decoder, total, block_count, min_bytes) |
There was a problem hiding this comment.
Fixed — read_map now raises on a negative per-block byte-size.
…block sizes Address review feedback: - skip_long now also rejects a 10th varint byte with payload bits above bit 63, matching read_long, so a skipped long can't accept an out-of-64-bit-range encoding read_long would reject. - read_array and read_map now reject a negative per-block byte-size in the negative-count form (as skip_blocks already does), instead of silently accepting malformed input. Assisted-by: GitHub Copilot:claude-opus-4.8
What is the purpose of the change
A
bytesorstringvalue is encoded as a length prefix followed by that many bytes of data, and anarrayormapblock is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.This applies the equivalent of the Java SDK fix AVRO-4241 to the Ruby SDK and extends it to collections. It has two complementary parts.
1. Validate available bytes before allocating
When the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as
null) is never falsely rejected. Sources that cannot report their remaining size are unaffected.BinaryDecoder#bytes_remainingreports the bytes still readable when the reader can report its size (elsenil).#readrejects an over-large declared length above a threshold, andDatumReader#read_array/#read_mapreject a block whose element count could not be backed by the bytes remaining, usingmin_bytes_per_elementfrom the element schema.2. Cap collection allocation for zero-byte elements
Zero-byte elements (
null, or a record with only zero-byte fields) consume no input, so the available-bytes check cannot bound their count: a tiny payload such as{{"type":"array","items":"null"}}declaring a block count of 200,000,000 would otherwise drive an unbounded allocation. In addition to the available-bytes check,ensure_collection_availabletracks the cumulative count across blocks and applies a structural cap to every collection (DEFAULT_MAX_COLLECTION_STRUCTURAL=Integer.MAX_VALUE - 8, also covering readers that cannot report their size) and a zero-byte item cap (DEFAULT_MAX_COLLECTION_ITEMS= 10,000,000).read_array/read_mapand the skip path (skip_blocks, used byskip_array/skip_map) are all bounded the same way. Rejections raise the dedicatedAvro::IO::CollectionSizeError(a subclass ofAvroError). When set, theAVRO_MAX_COLLECTION_ITEMSenvironment variable caps both limits.This is a sub-task of AVRO-4292 and resolves AVRO-4297.
Verifying this change
This change added tests and can be verified as follows:
lang/ruby/test/test_io.rbwith over-limitbytes/array/maprejection, anarray<null>huge-count rejection, a smallarray<null>that still decodes, and the skip path bounded.cd lang/ruby && bundle exec rake test(202 tests passing);bundle exec rubocop lib/avro/io.rb(clean).Documentation