Skip to content

3622: [C] Fix negative length validation in binary decoding #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martin-augment wants to merge 1 commit into
base: main
Choose a base branch
from
Open

3622: [C] Fix negative length validation in binary decoding #27

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lang/c/src/encoding_binary.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ static int read_bytes(avro_reader_t reader, char **bytes, int64_t * len)
int rval;
check_prefix(rval, read_long(reader, len),
"Cannot read bytes length: ");
if (*len < 0) {
Copy link
Copy Markdown

@augmentcode augmentcode bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see negative-length validation added for read_bytes/read_string. Note that skip_bytes (and thus skip_string) still accepts a decoded negative length and passes it to AVRO_SKIP, which could behave badly on malformed input.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:security; feedback:The Augment AI reviewer is correct! The same kind of problem as the fixed ones could be seen in the skip_bytes() method. It should be fixed the same way as the others. Prevents a security attack by crafting an Avro file with negative length for a field with a Bytes schema.

avro_set_error("Invalid bytes length: %" PRId64, *len);
return EINVAL;
}
*bytes = (char *) avro_malloc(*len + 1);
Copy link
Copy Markdown

@augmentcode augmentcode bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avro_malloc is a thin wrapper over realloc and doesn’t guard size overflows; if *len is extremely large, *len + 1 can overflow (UB) and/or truncate when converted to size_t. Consider adding an upper-bound check (also applies to read_string’s str_len + 1) before computing the allocation size.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-to-have; category:security; feedback:The Augment AI reviewer is correct! A specially crafted Avro file with a really big length value will lead to trying to allocate a lot of memory and possible crash due to out-of-memory error.

if (!*bytes) {
avro_set_error("Cannot allocate buffer for bytes value");
Expand Down Expand Up @@ -175,6 +179,10 @@ static int read_string(avro_reader_t reader, char **s, int64_t *len)
int rval;
check_prefix(rval, read_long(reader, &str_len),
"Cannot read string length: ");
if (str_len < 0) {
avro_set_error("Invalid string length: %" PRId64, str_len);
return EINVAL;
}
*len = str_len + 1;
*s = (char *) avro_malloc(*len);
if (!*s) {
Expand Down