Copilot commented on code in PR #3842:
URL: https://github.com/apache/avro/pull/3842#discussion_r3563756500
##########
lang/c/src/consume-binary.c:
##########
@@ -101,7 +105,11 @@ read_map(avro_reader_t reader, const avro_encoding_t * enc,
while (block_count != 0) {
if (block_count < 0) {
- block_count = block_count * -1;
+ if (block_count == INT64_MIN) {
+ avro_set_error("Invalid map block count");
+ return EINVAL;
+ }
Review Comment:
`read_map()` invokes `map_start_block` with the raw `block_count` before
validating it, so an `INT64_MIN` block count can reach consumer callbacks
despite the later negation guard. Reject `INT64_MIN` immediately after reading
`block_count` (before the first `map_start_block` call). The same early-guard
pattern should also be applied to the subsequent `map_start_block` call after
reading the next block count.
##########
lang/c/src/consume-binary.c:
##########
@@ -58,7 +58,11 @@ read_array(avro_reader_t reader, const avro_encoding_t * enc,
while (block_count != 0) {
if (block_count < 0) {
- block_count = block_count * -1;
+ if (block_count == INT64_MIN) {
+ avro_set_error("Invalid array block count");
+ return EINVAL;
Review Comment:
In `read_array()`, the `INT64_MIN` validation happens only when processing
the block (inside the loop), but `array_start_block` is called with the raw
`block_count` before that validation. This lets an `INT64_MIN` block count
reach consumer callbacks prior to rejection, undermining the intended
CWE-190/UB mitigation. Reject `INT64_MIN` immediately after reading
`block_count` and before the first `array_start_block` call.
##########
lang/c/src/consume-binary.c:
##########
@@ -58,7 +58,11 @@ read_array(avro_reader_t reader, const avro_encoding_t * enc,
while (block_count != 0) {
if (block_count < 0) {
- block_count = block_count * -1;
+ if (block_count == INT64_MIN) {
+ avro_set_error("Invalid array block count");
+ return EINVAL;
+ }
+ block_count = -block_count;
Review Comment:
`read_array()` calls `array_start_block` for subsequent blocks (after
reading the next `block_count`) without validating `INT64_MIN` first, so a
malformed stream can still deliver `INT64_MIN` to consumer callbacks. Add the
same `INT64_MIN` guard immediately after the `read_long()` at the end of the
loop, before invoking `array_start_block`.
##########
lang/c/src/consume-binary.c:
##########
@@ -101,7 +105,11 @@ read_map(avro_reader_t reader, const avro_encoding_t * enc,
while (block_count != 0) {
if (block_count < 0) {
- block_count = block_count * -1;
+ if (block_count == INT64_MIN) {
+ avro_set_error("Invalid map block count");
+ return EINVAL;
+ }
+ block_count = -block_count;
Review Comment:
`read_map()` still calls `map_start_block` after reading the *next*
`block_count` (near the end of the loop) without checking for `INT64_MIN`
first. Even with the negation guard here, a malformed stream can supply
`INT64_MIN` as a subsequent block count and reach consumer callbacks before
rejection; the same early `INT64_MIN` check should be added immediately after
that `read_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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
