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:27 UTC
[PR] AVRO-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
iemejia opened a new pull request, #3862:
URL: https://github.com/apache/avro/pull/3862
## 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 Ruby 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 when the reader can report its size (else `nil`). `#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-4297](https://issues.apache.org/jira/browse/AVRO-4297).
## Verifying this change
This change added tests and can be verified as follows:
- Extended `lang/ruby/test/test_io.rb` with over-limit `bytes`/`array`/`map` rejection and an array of nulls that must not be falsely rejected.
- Run: `cd lang/ruby && bundle exec rake test`
## 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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567432677
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -64,8 +75,14 @@ def read_long
b = byte!
n = b & 0x7F
shift = 7
+ # An Avro long is a 64-bit zig-zag varint, at most 10 bytes. Bound the
+ # continuation chain so a malicious input cannot force unbounded Integer
+ # growth (and heavy CPU/memory use) before any caller-side check runs.
+ count = 1
while (b & 0x80) != 0
+ raise AvroError, "Varint is too long" if count >= 10
b = byte!
+ count += 1
n |= (b & 0x7F) << shift
shift += 7
end
Review Comment:
Fixed — skip_long now enforces the same 10-byte bound as read_long and raises AvroError on an overlong continuation chain, so skipping a long field can't be used to force scanning unbounded input.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567536539
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -121,8 +181,13 @@ def skip_int
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
Review Comment:
Fixed — skip_long now rejects a 10th byte with payload bits above bit 63 (`count == 10 && (b & 0x7E) != 0`), consistent with read_long.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -312,12 +381,15 @@ def read_enum(writers_schema, readers_schema, decoder)
def read_array(writers_schema, readers_schema, decoder)
read_items = []
+ min_bytes = min_bytes_per_element(writers_schema.items)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
block_count = -block_count
_block_size = decoder.read_long
end
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
Review Comment:
Fixed — read_array now raises on a negative per-block byte-size.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -331,12 +403,16 @@ def read_array(writers_schema, readers_schema, decoder)
def read_map(writers_schema, readers_schema, decoder)
read_items = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + min_bytes_per_element(writers_schema.values)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
block_count = -block_count
_block_size = decoder.read_long
end
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
Review Comment:
Fixed — read_map now raises on a negative per-block byte-size.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567399307
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +120,43 @@ def read_string
end
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
+ # cannot report their size fall back to reading directly.
+ if len < 0
+ # A negative length would make IO#read return the rest of the stream,
+ # which bypasses the size check and can allocate without bound.
+ raise AvroError, "Cannot read a negative number of bytes: #{len}"
+ end
+ if len > MAX_UNCHECKED_READ
+ remaining = bytes_remaining
+ if remaining && len > remaining
+ raise AvroError, "Cannot read #{len} bytes, only #{remaining} remaining"
+ end
+ end
@reader.read(len)
end
Review Comment:
Fixed — `read` now raises AvroError if the underlying IO returns fewer than `len` bytes (or nil), so a truncated/partial read is rejected rather than silently yielding corrupt data.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -481,11 +546,14 @@ def skip_union(writers_schema, decoder)
end
Review Comment:
Fixed — `skip_union` now applies the same `[0, schemas.size)` bounds check as `read_union` and raises AvroError on an out-of-range/negative index.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567334035
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +564,112 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
Review Comment:
`collection_limits` parses `ENV['AVRO_MAX_COLLECTION_ITEMS']` every time a block is processed (via `ensure_collection_available`), which adds avoidable overhead for arrays/maps split into many blocks. Memoize the computed limits per `DatumReader` instance so the env var is read/parsed at most once per decode/skip operation (while still allowing changes between reader instances).
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567394511
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -481,11 +546,14 @@ def skip_union(writers_schema, decoder)
end
def skip_array(writers_schema, decoder)
- skip_blocks(decoder) { skip_data(writers_schema.items, decoder) }
+ min_bytes = min_bytes_per_element(writers_schema.items)
+ skip_blocks(decoder, min_bytes) { skip_data(writers_schema.items, decoder) }
Review Comment:
While the skip-path hardening is being expanded here (via `skip_blocks` bounds), `skip_union` still reads the union branch index and uses it directly to index `writers_schema.schemas`. In Ruby, negative indices are valid and can silently select the wrong branch during projection/skipping. Please add the same bounds check used by `read_union` and raise `AvroError` when the index is negative or >= branch count.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -64,8 +75,14 @@ def read_long
b = byte!
n = b & 0x7F
shift = 7
+ # An Avro long is a 64-bit zig-zag varint, at most 10 bytes. Bound the
+ # continuation chain so a malicious input cannot force unbounded Integer
+ # growth (and heavy CPU/memory use) before any caller-side check runs.
+ count = 1
while (b & 0x80) != 0
+ raise AvroError, "Varint is too long" if count >= 10
b = byte!
+ count += 1
n |= (b & 0x7F) << shift
shift += 7
end
Review Comment:
`read_long` now enforces the 10-byte max for Avro long encodings, but `BinaryDecoder#skip_long` still loops until it finds a byte without the continuation bit. If a long field is being *skipped* (e.g., during projection), a malicious payload can still supply an arbitrarily long continuation chain and force the decoder to scan large amounts of input (CPU/time DoS) instead of failing fast. Consider applying the same 10-byte bound (and raising `AvroError`) in `skip_long` 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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3566941889
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,105 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count here is corrupt/malicious data (the read/skip callers
+ # already normalized a legitimate negative block count to its absolute
+ # value); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
+
+ total += count
+ max_items, max_structural = collection_limits
+
+ # A structural cap covers all collections, including decoders that cannot
+ # report their remaining bytes.
+ if total > max_structural
+ raise CollectionSizeError, "Collection size #{total} exceeds the maximum allowed size of #{max_structural}"
+ end
+
+ if min_bytes_per_element <= 0
+ # Zero-byte elements (e.g. null) consume no input, so the
+ # bytes-remaining check cannot bound them; cap by item count.
+ if total > max_items
+ raise CollectionSizeError, "Collection of zero-byte elements (#{total}) exceeds the maximum allowed size of #{max_items}"
+ end
+ elsif decoder.respond_to?(:bytes_remaining)
+ # A decoder that implements the read protocol but not #bytes_remaining
+ # (e.g. a custom decoder) cannot report the remaining size; skip the
+ # byte check for it rather than raising NoMethodError.
+ 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 CollectionSizeError, "Collection claims #{count} elements with at least #{min_bytes_per_element} bytes each, but only #{remaining} bytes are available"
+ end
+ end
+
+ total
+ end
+
+ def skip_blocks(decoder, min_bytes = 0, &blk)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
- decoder.skip(decoder.read_long)
+ # A negative count declares abs(count) items preceded by a block
+ # byte-size. Bound the count too (so it can't bypass the caps), and
+ # reject a negative byte-size (which would seek the reader backwards)
+ # before skipping the whole block by its size.
+ block_count = -block_count
+ block_size = decoder.read_long
+ raise AvroError, "Invalid negative block size: #{block_size}" if block_size < 0
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
+ decoder.skip(block_size)
Review Comment:
Fixed in ebf1f8be6c: the negative-block skip path now rejects a block_size larger than the bytes remaining (when the reader can report them) before decoder.skip, so a truncated payload can't seek past EOF. Added a regression test.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,105 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count here is corrupt/malicious data (the read/skip callers
+ # already normalized a legitimate negative block count to its absolute
+ # value); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
Review Comment:
Fixed in ebf1f8be6c: read_long now bounds a 64-bit long to at most 10 bytes and raises AvroError on a longer continuation chain, so a malicious varint can't force unbounded Integer growth before any caller-side check runs. Added a 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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565092772
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +110,28 @@ def read_string
end
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
Review Comment:
`BinaryDecoder#read` accepts `len` directly from the stream (e.g., bytes/string length prefix). If a malicious input encodes a negative length, Ruby IO treats `read(-1)` as “read the rest of the stream”, which can cause an unexpected large allocation and bypass the new size check (since `len > MAX_UNCHECKED_READ` will be false). Reject negative lengths before calling `@reader.read`.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,6 +524,43 @@ def skip_record(writers_schema, decoder)
end
private
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error
Review Comment:
`min_bytes_per_element` treats `:record`/`:error` specially, but not `:request` even though request schemas are record-like and are handled alongside records elsewhere in this file. Overestimating `:request` as at-least-1-byte can falsely reject arrays/maps of request values (e.g., request records whose fields encode to 0 bytes). Include `:request` in the record-like branch.
--
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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565769636
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,70 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # 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)
Review Comment:
Fixed in 1a3d3daf77: 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.
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,70 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # 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)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_raise(Avro::AvroError) { decoder.read_bytes }
+ end
+
+ def test_read_bytes_within_stream_still_reads
+ payload = 'x' * (2 * 1024 * 1024)
Review Comment:
Fixed in 1a3d3daf77: 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.
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,70 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # 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)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_raise(Avro::AvroError) { decoder.read_bytes }
+ end
+
+ def test_read_bytes_within_stream_still_reads
+ payload = 'x' * (2 * 1024 * 1024)
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_bytes(payload)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_equal(payload, decoder.read_bytes)
+ end
+
+ # An array/map block declares an element count; a malicious or truncated input
+ # can declare far more elements than the remaining bytes could hold. The count
+ # is validated against the bytes remaining before iterating, using the minimum
+ # on-wire size of the element schema (so 0-byte elements like null are not
+ # falsely rejected).
+ def decode(schema_json, encoded)
+ schema = Avro::Schema.parse(schema_json)
+ reader = Avro::IO::DatumReader.new(schema)
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(encoded)))
+ end
+
+ def test_read_array_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::AvroError) do
+ decode('{"type":"array","items":"long"}', writer.string)
+ end
+ end
+
+ def test_read_map_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::AvroError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ def test_read_array_of_null_not_falsely_rejected
+ 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
Review Comment:
Fixed in 1a3d3daf77: 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.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3566582745
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,98 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count is corrupt/malicious data (it can also arise from
+ # overflow when negating a negative block count); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
Review Comment:
The comment about a negative count arising from overflow when negating a negative block count is not accurate in Ruby (Integer is arbitrary-precision, so negation doesn’t overflow). This can confuse future maintainers about when `count < 0` is actually possible here.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567461054
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -64,8 +75,14 @@ def read_long
b = byte!
n = b & 0x7F
shift = 7
+ # An Avro long is a 64-bit zig-zag varint, at most 10 bytes. Bound the
+ # continuation chain so a malicious input cannot force unbounded Integer
+ # growth (and heavy CPU/memory use) before any caller-side check runs.
+ count = 1
while (b & 0x80) != 0
+ raise AvroError, "Varint is too long" if count >= 10
b = byte!
+ count += 1
n |= (b & 0x7F) << shift
Review Comment:
Fixed — read_long now rejects the 10th byte if any payload bit above bit 63 is set (`count == 10 && (b & 0x7E) != 0`), so a 10-byte encoding 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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3566776696
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,98 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count is corrupt/malicious data (it can also arise from
+ # overflow when negating a negative block count); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
Review Comment:
Fixed in f60163dbac: reworded the comment — Ruby integers are arbitrary-precision, so negation can't overflow; a negative count here only means corrupt data (the read/skip callers already normalize a legitimate negative block 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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567361190
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +564,112 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
Review Comment:
Fixed — `collection_limits` is now memoized in `@collection_limits` on the reader instance, so the env var is parsed at most once per reader (a new reader still picks up a changed value).
--
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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565204720
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +110,40 @@ def read_string
end
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
+ # cannot report their size fall back to reading directly.
+ if len < 0
+ # A negative length would make IO#read return the rest of the stream,
+ # which bypasses the size check and can allocate without bound.
+ raise AvroError, "Cannot read a negative number of bytes: #{len}"
+ end
+ if len > MAX_UNCHECKED_READ
+ remaining = bytes_remaining
+ if remaining && len > remaining
+ raise AvroError, "Cannot read #{len} bytes, only #{remaining} remaining"
+ end
+ end
@reader.read(len)
end
+ # Number of bytes still available to read, or nil when the reader cannot
+ # report its size. Used to reject a declared length or collection block
+ # count that exceeds the data actually available before allocating for it.
+ def bytes_remaining
+ return nil unless @reader.respond_to?(:size) && @reader.respond_to?(:tell)
+ size = @reader.size
+ pos = @reader.tell
+ return nil unless size.is_a?(Integer) && pos.is_a?(Integer)
+ size - pos
Review Comment:
`bytes_remaining` can return a negative value when the underlying IO's reported size is smaller than the current position (e.g., file truncated while reading). That negative value can surface in error messages ("only -N remaining") and makes the availability checks harder to reason about. Clamp negative remaining to 0 (or return nil) so callers treat it as no bytes available.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -337,6 +375,8 @@ def read_map(writers_schema, readers_schema, decoder)
block_count = -block_count
_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))
Review Comment:
`min_bytes_per_element(writers_schema.values)` is recomputed for every map block. For maps split into many blocks and/or complex value schemas, this adds avoidable overhead. Cache the computed minimum once per `read_map` invocation and reuse it for subsequent blocks.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -318,6 +355,7 @@ def read_array(writers_schema, readers_schema, decoder)
block_count = -block_count
_block_size = decoder.read_long
end
+ ensure_collection_available(decoder, block_count, min_bytes_per_element(writers_schema.items))
Review Comment:
`min_bytes_per_element(writers_schema.items)` is recomputed for every array block. For arrays split into many blocks and/or complex item schemas (records), this adds avoidable overhead on the hot path. Cache the computed minimum once per `read_array` invocation and reuse it for subsequent 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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3566789378
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,99 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count here is corrupt/malicious data (the read/skip callers
+ # already normalized a legitimate negative block count to its absolute
+ # value); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
+
+ total += count
+ max_items, max_structural = collection_limits
+
+ # A structural cap covers all collections, including decoders that cannot
+ # report their remaining bytes.
+ if total > max_structural
+ raise CollectionSizeError, "Collection size #{total} exceeds the maximum allowed size of #{max_structural}"
+ end
+
+ if min_bytes_per_element <= 0
+ # Zero-byte elements (e.g. null) consume no input, so the
+ # bytes-remaining check cannot bound them; cap by item count.
+ if total > max_items
+ raise CollectionSizeError, "Collection of zero-byte elements (#{total}) exceeds the maximum allowed size of #{max_items}"
+ end
+ elsif decoder.respond_to?(:bytes_remaining)
+ # A decoder that implements the read protocol but not #bytes_remaining
+ # (e.g. a custom decoder) cannot report the remaining size; skip the
+ # byte check for it rather than raising NoMethodError.
+ 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 CollectionSizeError, "Collection claims #{count} elements with at least #{min_bytes_per_element} bytes each, but only #{remaining} bytes are available"
+ end
+ end
+
+ total
+ end
+
+ def skip_blocks(decoder, min_bytes = 0, &blk)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
+ # A negative count carries a byte size, so the whole block can be
+ # skipped directly without iterating.
decoder.skip(decoder.read_long)
else
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
block_count.times(&blk)
end
Review Comment:
`skip_blocks` applies `ensure_collection_available` only for positive block counts. For the negative-block form (count < 0), it skips the block by byte-size without updating `total` or applying the structural / zero-byte item caps (or `bytes_remaining` validation). This allows a huge collection encoded using the negative-block form (e.g., array<null> with block byte-size 0) to bypass the new bounds in the skip path.
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,122 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # report its size, that is rejected before allocating for it.
+ def test_read_bytes_rejects_length_beyond_stream
+ writer = StringIO.new
+ declared = Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1
+ Avro::IO::BinaryEncoder.new(writer).write_long(declared)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_raise(Avro::AvroError) { decoder.read_bytes }
+ end
+
+ def test_read_bytes_within_stream_still_reads
+ payload = 'x' * (Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1)
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_bytes(payload)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_equal(payload, decoder.read_bytes)
+ end
+
+ # An array/map block declares an element count; a malicious or truncated input
+ # can declare far more elements than the remaining bytes could hold. The count
+ # is validated against the bytes remaining before iterating, using the minimum
+ # on-wire size of the element schema (so 0-byte elements like null are not
+ # falsely rejected).
+ def decode(schema_json, encoded)
+ schema = Avro::Schema.parse(schema_json)
+ reader = Avro::IO::DatumReader.new(schema)
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(encoded)))
+ end
+
+ def test_read_array_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"long"}', writer.string)
+ end
+ end
+
+ def test_read_map_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ def test_read_array_of_null_not_falsely_rejected
+ 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(count, result.length)
+ assert_nil(result.first)
+ assert_nil(result.last)
+ end
+
+ def test_read_array_within_stream_still_reads
+ schema = Avro::Schema.parse('{"type":"array","items":"long"}')
+ writer = StringIO.new
+ Avro::IO::DatumWriter.new(schema).write([1, 2, 3], Avro::IO::BinaryEncoder.new(writer))
+ assert_equal([1, 2, 3], decode('{"type":"array","items":"long"}', writer.string))
+ end
+
+ # A zero-byte element type (null) consumes no input, so the bytes-remaining
+ # check cannot bound its block count; a tiny payload declaring a huge count
+ # must instead be rejected by the item cap before building the array.
+ def test_read_array_of_null_rejects_huge_count
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000) # ~4 byte payload
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"null"}', writer.string)
+ end
+ end
+
+ # INT64_MIN as a block count is the pathological negation case. Ruby integers
+ # do not overflow, so negating it yields 2**63, which the cap rejects. INT64_MIN
+ # zig-zag encodes as the 10-byte varint below, followed by a block byte-size (0)
+ # that the negative-block path reads.
+ def test_read_array_of_null_int64_min_block_count
+ payload = "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x01\x00".b
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"null"}', payload)
+ end
+ end
+
+ def test_read_map_rejects_huge_count
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ # The skip path (a writer field absent from the reader schema) must be bounded
+ # too, so skipping a huge zero-byte block cannot loop endlessly.
+ def test_skip_array_of_null_rejects_huge_count
+ writers_schema = Avro::Schema.parse(<<-JSON)
+ {"type":"record","name":"Foo","fields":[
+ {"name":"arr","type":{"type":"array","items":"null"}},
+ {"name":"val","type":"int"}]}
+ JSON
+ readers_schema = Avro::Schema.parse(<<-JSON)
+ {"type":"record","name":"Foo","fields":[{"name":"val","type":"int"}]}
+ JSON
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000) # arr block count
+ reader = Avro::IO::DatumReader.new(writers_schema, readers_schema)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(writer.string)))
+ end
+ end
Review Comment:
The skip-path regression tests cover only the positive block-count encoding. Since `skip_blocks` also supports the negative-block form (count < 0 followed by block byte-size), add a test case for a huge negative count with a 0-byte block (array<null>) to prevent bypasses/regressions in that branch.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567381488
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -481,11 +546,14 @@ def skip_union(writers_schema, decoder)
end
Review Comment:
DatumReader#skip_union indexes `writers_schema.schemas` with the decoded branch index without bounds checking. Since Ruby allows negative indexing, malformed data (e.g., -1) can silently select the wrong branch and skip the wrong amount, desynchronizing the decoder. Add the same bounds check used in read_union and raise AvroError on invalid indices.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +120,43 @@ def read_string
end
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
+ # cannot report their size fall back to reading directly.
+ if len < 0
+ # A negative length would make IO#read return the rest of the stream,
+ # which bypasses the size check and can allocate without bound.
+ raise AvroError, "Cannot read a negative number of bytes: #{len}"
+ end
+ if len > MAX_UNCHECKED_READ
+ remaining = bytes_remaining
+ if remaining && len > remaining
+ raise AvroError, "Cannot read #{len} bytes, only #{remaining} remaining"
+ end
+ end
@reader.read(len)
end
Review Comment:
BinaryDecoder#read can return a short string (or nil) when the underlying IO is truncated or performs partial reads. For bytes/string/fixed, that would silently accept corrupted/truncated data and can undermine the intended “reject before allocating” hardening for lengths <= MAX_UNCHECKED_READ. Consider raising when fewer than `len` bytes are actually read.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on PR #3862:
URL: https://github.com/apache/avro/pull/3862#issuecomment-4952730004
**Preallocation (AVRO-4292 follow-up) evaluated — no change needed here.** `read_array`/`read_map` append via `read_items << ...`, growing the array 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). 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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3566900455
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,105 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count here is corrupt/malicious data (the read/skip callers
+ # already normalized a legitimate negative block count to its absolute
+ # value); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
Review Comment:
The new collection/length caps rely on `decoder.read_long` for counts and sizes, but `BinaryDecoder#read_long` will currently consume an arbitrarily long varint (it loops until a byte without the continuation bit). A malicious input with a very long continuation chain can force huge Integer growth and heavy CPU/memory usage before `ensure_collection_available` ever runs. Consider enforcing the Avro 64-bit long maximum encoding length (10 bytes) in `read_long` and raising `AvroError` when exceeded.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,105 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count here is corrupt/malicious data (the read/skip callers
+ # already normalized a legitimate negative block count to its absolute
+ # value); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
+
+ total += count
+ max_items, max_structural = collection_limits
+
+ # A structural cap covers all collections, including decoders that cannot
+ # report their remaining bytes.
+ if total > max_structural
+ raise CollectionSizeError, "Collection size #{total} exceeds the maximum allowed size of #{max_structural}"
+ end
+
+ if min_bytes_per_element <= 0
+ # Zero-byte elements (e.g. null) consume no input, so the
+ # bytes-remaining check cannot bound them; cap by item count.
+ if total > max_items
+ raise CollectionSizeError, "Collection of zero-byte elements (#{total}) exceeds the maximum allowed size of #{max_items}"
+ end
+ elsif decoder.respond_to?(:bytes_remaining)
+ # A decoder that implements the read protocol but not #bytes_remaining
+ # (e.g. a custom decoder) cannot report the remaining size; skip the
+ # byte check for it rather than raising NoMethodError.
+ 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 CollectionSizeError, "Collection claims #{count} elements with at least #{min_bytes_per_element} bytes each, but only #{remaining} bytes are available"
+ end
+ end
+
+ total
+ end
+
+ def skip_blocks(decoder, min_bytes = 0, &blk)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
- decoder.skip(decoder.read_long)
+ # A negative count declares abs(count) items preceded by a block
+ # byte-size. Bound the count too (so it can't bypass the caps), and
+ # reject a negative byte-size (which would seek the reader backwards)
+ # before skipping the whole block by its size.
+ block_count = -block_count
+ block_size = decoder.read_long
+ raise AvroError, "Invalid negative block size: #{block_size}" if block_size < 0
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
+ decoder.skip(block_size)
Review Comment:
In the negative-block skip path, `block_size` is trusted and passed to `decoder.skip` without validating it against the actual bytes remaining. With a truncated/malicious payload this can seek past EOF and later fail with a less clear `EOFError` (or leave the decoder positioned far beyond the end). When `bytes_remaining` is available, reject a `block_size` larger than the remaining bytes before seeking.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567430613
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -481,11 +546,14 @@ def skip_union(writers_schema, decoder)
end
def skip_array(writers_schema, decoder)
- skip_blocks(decoder) { skip_data(writers_schema.items, decoder) }
+ min_bytes = min_bytes_per_element(writers_schema.items)
+ skip_blocks(decoder, min_bytes) { skip_data(writers_schema.items, decoder) }
Review Comment:
Already addressed in commit db48317693: skip_union now applies the same `[0, schemas.size)` bounds check as read_union and raises AvroError on a negative/out-of-range index.
--
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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565199971
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +110,33 @@ def read_string
end
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
+ # cannot report their size fall back to reading directly.
+ if len < 0
+ # A negative length would make IO#read return the rest of the stream,
+ # which bypasses the size check and can allocate without bound.
+ raise AvroError, "Cannot read a negative number of bytes: #{len}"
+ end
+ if len > MAX_UNCHECKED_READ
+ remaining = bytes_remaining
+ if remaining && len > remaining
+ raise AvroError, "Cannot read #{len} bytes, only #{remaining} remaining"
+ end
+ end
@reader.read(len)
end
+ # Number of bytes still available to read, or nil when the reader cannot
+ # report its size. Used to reject a declared length or collection block
+ # count that exceeds the data actually available before allocating for it.
+ def bytes_remaining
+ return nil unless @reader.respond_to?(:size) && @reader.respond_to?(:tell)
+ @reader.size - @reader.tell
+ end
Review 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.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,6 +529,43 @@ def skip_record(writers_schema, decoder)
end
private
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # 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 or when the reader cannot
+ # report how many bytes remain.
+ def ensure_collection_available(decoder, count, min_bytes_per_element)
+ 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
Review Comment:
Fixed in 34859b4: ensure_collection_available compares count against remaining / min_bytes_per_element instead of multiplying, avoiding a large intermediate Integer.
--
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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565707315
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,6 +541,45 @@ def skip_record(writers_schema, decoder)
end
private
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # 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 or when the reader cannot
+ # report how many bytes remain.
+ 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
Review Comment:
`ensure_collection_available` unconditionally calls `decoder.bytes_remaining`. Any decoder that implements the read protocol (`read_long`, `read_string`, etc.) but does not implement `bytes_remaining` will now raise `NoMethodError`, even though the intended behavior for “cannot report remaining size” is to skip the check. Guard this call with `respond_to?` (or rescue `NoMethodError`) and treat the remaining size as unknown (`nil`).
--
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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565742239
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,70 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # 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)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_raise(Avro::AvroError) { decoder.read_bytes }
+ end
+
+ def test_read_bytes_within_stream_still_reads
+ payload = 'x' * (2 * 1024 * 1024)
Review Comment:
This test allocates a 2MiB payload just to exercise the "large read" path. Using a payload that’s only 1 byte over BinaryDecoder::MAX_UNCHECKED_READ keeps the test faster and lower-memory while still validating the threshold behavior.
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,70 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # 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)
Review Comment:
This uses a hard-coded 100MiB declared length; the exact value isn’t important as long as it exceeds BinaryDecoder::MAX_UNCHECKED_READ. Using MAX_UNCHECKED_READ + 1 makes the test more directly tied to the behavior under test and avoids an unnecessarily large constant.
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,70 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # 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)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_raise(Avro::AvroError) { decoder.read_bytes }
+ end
+
+ def test_read_bytes_within_stream_still_reads
+ payload = 'x' * (2 * 1024 * 1024)
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_bytes(payload)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_equal(payload, decoder.read_bytes)
+ end
+
+ # An array/map block declares an element count; a malicious or truncated input
+ # can declare far more elements than the remaining bytes could hold. The count
+ # is validated against the bytes remaining before iterating, using the minimum
+ # on-wire size of the element schema (so 0-byte elements like null are not
+ # falsely rejected).
+ def decode(schema_json, encoded)
+ schema = Avro::Schema.parse(schema_json)
+ reader = Avro::IO::DatumReader.new(schema)
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(encoded)))
+ end
+
+ def test_read_array_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::AvroError) do
+ decode('{"type":"array","items":"long"}', writer.string)
+ end
+ end
+
+ def test_read_map_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::AvroError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ def test_read_array_of_null_not_falsely_rejected
+ 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
Review Comment:
`assert_equal([nil] * count, result)` builds a second large array (and the count itself is fairly high), which adds avoidable time/memory to the test suite. It’s sufficient to assert the decoded length and spot-check a couple of elements are nil.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia closed pull request #3862: AVRO-4297: [ruby] Bound allocation when decoding length-prefixed values and collections
URL: https://github.com/apache/avro/pull/3862
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567430608
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -64,8 +75,14 @@ def read_long
b = byte!
n = b & 0x7F
shift = 7
+ # An Avro long is a 64-bit zig-zag varint, at most 10 bytes. Bound the
+ # continuation chain so a malicious input cannot force unbounded Integer
+ # growth (and heavy CPU/memory use) before any caller-side check runs.
+ count = 1
while (b & 0x80) != 0
+ raise AvroError, "Varint is too long" if count >= 10
b = byte!
+ count += 1
n |= (b & 0x7F) << shift
Review Comment:
`BinaryDecoder#read_long` now caps varints to 10 bytes, but it still accepts 10-byte encodings that set bits beyond the 64-bit range (the 10th byte may legally contain only 0x00 or 0x01). This allows decoding malformed Avro longs outside the spec’s 64-bit domain; add a range check when `count == 10` to reject out-of-range 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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565239843
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +110,40 @@ def read_string
end
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
+ # cannot report their size fall back to reading directly.
+ if len < 0
+ # A negative length would make IO#read return the rest of the stream,
+ # which bypasses the size check and can allocate without bound.
+ raise AvroError, "Cannot read a negative number of bytes: #{len}"
+ end
+ if len > MAX_UNCHECKED_READ
+ remaining = bytes_remaining
+ if remaining && len > remaining
+ raise AvroError, "Cannot read #{len} bytes, only #{remaining} remaining"
+ end
+ end
@reader.read(len)
end
+ # Number of bytes still available to read, or nil when the reader cannot
+ # report its size. Used to reject a declared length or collection block
+ # count that exceeds the data actually available before allocating for it.
+ def bytes_remaining
+ return nil unless @reader.respond_to?(:size) && @reader.respond_to?(:tell)
+ size = @reader.size
+ pos = @reader.tell
+ return nil unless size.is_a?(Integer) && pos.is_a?(Integer)
+ size - pos
Review 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.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -318,6 +355,7 @@ def read_array(writers_schema, readers_schema, decoder)
block_count = -block_count
_block_size = decoder.read_long
end
+ ensure_collection_available(decoder, block_count, min_bytes_per_element(writers_schema.items))
Review Comment:
Fixed in ea1ac32: read_array computes the per-element minimum once per invocation and reuses it across blocks.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -337,6 +375,8 @@ def read_map(writers_schema, readers_schema, decoder)
block_count = -block_count
_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))
Review Comment:
Fixed in ea1ac32: read_map computes 1 + min-bytes once per invocation and reuses it across 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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3567512151
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -312,12 +381,15 @@ def read_enum(writers_schema, readers_schema, decoder)
def read_array(writers_schema, readers_schema, decoder)
read_items = []
+ min_bytes = min_bytes_per_element(writers_schema.items)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
block_count = -block_count
_block_size = decoder.read_long
end
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
Review Comment:
In `DatumReader#read_array`, the negative-block form reads a block byte-size but does not validate it. A negative block size is malformed (and `skip_blocks` already rejects it), so the read path should also raise rather than silently accepting invalid encodings.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -121,8 +181,13 @@ def skip_int
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
Review Comment:
`BinaryDecoder#skip_long` bounds varints to 10 bytes, but unlike `read_long` it does not reject 10-byte encodings that set payload bits beyond the 64-bit range. This means malformed Avro longs that `read_long` would reject can still be silently accepted when the value is being skipped during projection, which can lead to inconsistent validation between read vs skip paths.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -331,12 +403,16 @@ def read_array(writers_schema, readers_schema, decoder)
def read_map(writers_schema, readers_schema, decoder)
read_items = {}
+ # Map keys are strings (>= 1 byte length prefix) plus the value.
+ min_bytes = 1 + min_bytes_per_element(writers_schema.values)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
block_count = -block_count
_block_size = decoder.read_long
end
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
Review Comment:
In `DatumReader#read_map`, the negative-block form reads a block byte-size but does not validate it. A negative block size is malformed (and `skip_blocks` already rejects it), so the read path should also raise rather than silently accepting 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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565162077
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +110,33 @@ def read_string
end
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
+ # cannot report their size fall back to reading directly.
+ if len < 0
+ # A negative length would make IO#read return the rest of the stream,
+ # which bypasses the size check and can allocate without bound.
+ raise AvroError, "Cannot read a negative number of bytes: #{len}"
+ end
+ if len > MAX_UNCHECKED_READ
+ remaining = bytes_remaining
+ if remaining && len > remaining
+ raise AvroError, "Cannot read #{len} bytes, only #{remaining} remaining"
+ end
+ end
@reader.read(len)
end
+ # Number of bytes still available to read, or nil when the reader cannot
+ # report its size. Used to reject a declared length or collection block
+ # count that exceeds the data actually available before allocating for it.
+ def bytes_remaining
+ return nil unless @reader.respond_to?(:size) && @reader.respond_to?(:tell)
+ @reader.size - @reader.tell
+ end
Review Comment:
`bytes_remaining` assumes `#size` and `#tell` are both supported and return numeric values if the methods exist. Some IO-like objects respond to these methods but return `nil` (or raise), which would cause `TypeError`/`IOError` here and break reads that are supposed to be “unaffected” when remaining size cannot be reported. Make this method resilient by returning `nil` when size/position can’t be obtained.
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,6 +529,43 @@ def skip_record(writers_schema, decoder)
end
private
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # 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 or when the reader cannot
+ # report how many bytes remain.
+ def ensure_collection_available(decoder, count, min_bytes_per_element)
+ 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
Review Comment:
`ensure_collection_available` computes `count * min_bytes_per_element`, which can create an additional very large Integer when `count` is huge. Comparing via division avoids allocating that intermediate product while preserving the same 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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565150638
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -103,10 +110,28 @@ def read_string
end
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
Review 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).
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,6 +524,43 @@ def skip_record(writers_schema, decoder)
end
private
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error
Review 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.
--
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-4297: [ruby] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3565728631
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,6 +541,45 @@ def skip_record(writers_schema, decoder)
end
private
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # 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 or when the reader cannot
+ # report how many bytes remain.
+ 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
Review Comment:
Fixed in 1186853c69: 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.
--
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-4297: [ruby] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3862:
URL: https://github.com/apache/avro/pull/3862#discussion_r3566893761
##########
lang/ruby/lib/avro/io.rb:
##########
@@ -496,12 +550,99 @@ def skip_record(writers_schema, decoder)
end
private
- def skip_blocks(decoder, &blk)
+ # Minimum number of bytes a single value of the given schema can occupy on
+ # the wire. Used to reject an array/map block count that could not be
+ # backed by the bytes remaining. A type that can encode to zero bytes
+ # (null) returns 0, which disables the check for it (so an array of nulls
+ # is not falsely rejected).
+ def min_bytes_per_element(schema, visited = nil)
+ visited ||= {}.compare_by_identity
+ case schema.type_sym
+ when :null then 0
+ when :float then 4
+ when :double then 8
+ when :fixed then schema.size
+ when :record, :error, :request
+ return 0 if visited[schema]
+ visited[schema] = true
+ total = schema.fields.sum { |field| min_bytes_per_element(field.type, visited) }
+ visited.delete(schema)
+ total
+ else
+ # boolean, int, long, bytes, string, enum, union, array, map: >= 1 byte
+ # (a union encodes at least a 1-byte branch index).
+ 1
+ end
+ end
+
+ # Reject a collection (array or map) block that could drive an unbounded
+ # allocation, before iterating. A block whose declared element count could
+ # not be backed by the bytes actually remaining is rejected; a block of
+ # zero-byte elements (where the bytes-remaining check does not apply) is
+ # bounded by a cumulative item cap; and every collection is bounded by a
+ # structural cap. Returns the running total across blocks.
+ #
+ # Both limits default to the same values as the other Avro SDKs and can be
+ # overridden (to a single value capping both) via the
+ # AVRO_MAX_COLLECTION_ITEMS environment variable.
+ DEFAULT_MAX_COLLECTION_ITEMS = 10_000_000 # Zero-byte element cap.
+ DEFAULT_MAX_COLLECTION_STRUCTURAL = 2147483639 # Integer.MAX_VALUE - 8.
+
+ def collection_limits
+ env = Integer(ENV['AVRO_MAX_COLLECTION_ITEMS'], exception: false)
+ if env && env >= 0
+ [env, env]
+ else
+ [DEFAULT_MAX_COLLECTION_ITEMS, DEFAULT_MAX_COLLECTION_STRUCTURAL]
+ end
+ end
+
+ def ensure_collection_available(decoder, total, count, min_bytes_per_element)
+ # A negative count here is corrupt/malicious data (the read/skip callers
+ # already normalized a legitimate negative block count to its absolute
+ # value); reject it explicitly.
+ raise CollectionSizeError, "Invalid negative collection block count: #{count}" if count < 0
+
+ total += count
+ max_items, max_structural = collection_limits
+
+ # A structural cap covers all collections, including decoders that cannot
+ # report their remaining bytes.
+ if total > max_structural
+ raise CollectionSizeError, "Collection size #{total} exceeds the maximum allowed size of #{max_structural}"
+ end
+
+ if min_bytes_per_element <= 0
+ # Zero-byte elements (e.g. null) consume no input, so the
+ # bytes-remaining check cannot bound them; cap by item count.
+ if total > max_items
+ raise CollectionSizeError, "Collection of zero-byte elements (#{total}) exceeds the maximum allowed size of #{max_items}"
+ end
+ elsif decoder.respond_to?(:bytes_remaining)
+ # A decoder that implements the read protocol but not #bytes_remaining
+ # (e.g. a custom decoder) cannot report the remaining size; skip the
+ # byte check for it rather than raising NoMethodError.
+ 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 CollectionSizeError, "Collection claims #{count} elements with at least #{min_bytes_per_element} bytes each, but only #{remaining} bytes are available"
+ end
+ end
+
+ total
+ end
+
+ def skip_blocks(decoder, min_bytes = 0, &blk)
+ total = 0
block_count = decoder.read_long
while block_count != 0
if block_count < 0
+ # A negative count carries a byte size, so the whole block can be
+ # skipped directly without iterating.
decoder.skip(decoder.read_long)
else
+ total = ensure_collection_available(decoder, total, block_count, min_bytes)
block_count.times(&blk)
end
Review Comment:
Fixed in 547dd67900: skip_blocks now normalizes a negative block count and applies ensure_collection_available on the sized (negative) path too, so the byte-sized block form can't bypass the caps; it also rejects a negative block byte-size (which decoder.skip would otherwise seek backwards).
##########
lang/ruby/test/test_io.rb:
##########
@@ -42,6 +42,122 @@ def test_bytes
check_default('"bytes"', '"foo"', "foo")
end
+ # A bytes/string value declares a length prefix; a malicious or truncated
+ # input can declare far more bytes than actually exist. On a reader that can
+ # report its size, that is rejected before allocating for it.
+ def test_read_bytes_rejects_length_beyond_stream
+ writer = StringIO.new
+ declared = Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1
+ Avro::IO::BinaryEncoder.new(writer).write_long(declared)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_raise(Avro::AvroError) { decoder.read_bytes }
+ end
+
+ def test_read_bytes_within_stream_still_reads
+ payload = 'x' * (Avro::IO::BinaryDecoder::MAX_UNCHECKED_READ + 1)
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_bytes(payload)
+ reader = StringIO.new(writer.string)
+ decoder = Avro::IO::BinaryDecoder.new(reader)
+ assert_equal(payload, decoder.read_bytes)
+ end
+
+ # An array/map block declares an element count; a malicious or truncated input
+ # can declare far more elements than the remaining bytes could hold. The count
+ # is validated against the bytes remaining before iterating, using the minimum
+ # on-wire size of the element schema (so 0-byte elements like null are not
+ # falsely rejected).
+ def decode(schema_json, encoded)
+ schema = Avro::Schema.parse(schema_json)
+ reader = Avro::IO::DatumReader.new(schema)
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(encoded)))
+ end
+
+ def test_read_array_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"long"}', writer.string)
+ end
+ end
+
+ def test_read_map_rejects_count_beyond_stream
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(1_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ def test_read_array_of_null_not_falsely_rejected
+ 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(count, result.length)
+ assert_nil(result.first)
+ assert_nil(result.last)
+ end
+
+ def test_read_array_within_stream_still_reads
+ schema = Avro::Schema.parse('{"type":"array","items":"long"}')
+ writer = StringIO.new
+ Avro::IO::DatumWriter.new(schema).write([1, 2, 3], Avro::IO::BinaryEncoder.new(writer))
+ assert_equal([1, 2, 3], decode('{"type":"array","items":"long"}', writer.string))
+ end
+
+ # A zero-byte element type (null) consumes no input, so the bytes-remaining
+ # check cannot bound its block count; a tiny payload declaring a huge count
+ # must instead be rejected by the item cap before building the array.
+ def test_read_array_of_null_rejects_huge_count
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000) # ~4 byte payload
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"null"}', writer.string)
+ end
+ end
+
+ # INT64_MIN as a block count is the pathological negation case. Ruby integers
+ # do not overflow, so negating it yields 2**63, which the cap rejects. INT64_MIN
+ # zig-zag encodes as the 10-byte varint below, followed by a block byte-size (0)
+ # that the negative-block path reads.
+ def test_read_array_of_null_int64_min_block_count
+ payload = "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x01\x00".b
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"array","items":"null"}', payload)
+ end
+ end
+
+ def test_read_map_rejects_huge_count
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ decode('{"type":"map","values":"long"}', writer.string)
+ end
+ end
+
+ # The skip path (a writer field absent from the reader schema) must be bounded
+ # too, so skipping a huge zero-byte block cannot loop endlessly.
+ def test_skip_array_of_null_rejects_huge_count
+ writers_schema = Avro::Schema.parse(<<-JSON)
+ {"type":"record","name":"Foo","fields":[
+ {"name":"arr","type":{"type":"array","items":"null"}},
+ {"name":"val","type":"int"}]}
+ JSON
+ readers_schema = Avro::Schema.parse(<<-JSON)
+ {"type":"record","name":"Foo","fields":[{"name":"val","type":"int"}]}
+ JSON
+ writer = StringIO.new
+ Avro::IO::BinaryEncoder.new(writer).write_long(200_000_000) # arr block count
+ reader = Avro::IO::DatumReader.new(writers_schema, readers_schema)
+ assert_raise(Avro::IO::CollectionSizeError) do
+ reader.read(Avro::IO::BinaryDecoder.new(StringIO.new(writer.string)))
+ end
+ end
Review Comment:
Fixed in 547dd67900: added test_skip_array_of_null_negative_block_rejects_huge_count, which skips a record's array<null> field encoded with a huge negative block count and asserts CollectionSizeError.
--
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