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