Skip to content

3527: Avro-4193: [c] segfault cannot handle empty byte array #15

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 2 commits into
base: main
Choose a base branch
from
Open

3527: Avro-4193: [c] segfault cannot handle empty byte array #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89fb9db
AVRO-4193 support for empty bytes
steven-aerts Oct 24, 2025
2d708a2
Merge branch 'main' into pr-3527-2025-10-27-11-18-01
martin-augment Oct 27, 2025
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
4 changes: 2 additions & 2 deletions lang/c/src/datum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ static avro_datum_t avro_bytes_private(char *bytes, int64_t size,
avro_datum_t avro_bytes(const char *bytes, int64_t size)
{
char *bytes_copy = (char *) avro_malloc(size);
if (!bytes_copy) {
if (!bytes_copy && size) {
Copy link
Copy Markdown

@augmentcode augmentcode bot Oct 27, 2025

Choose a reason for hiding this comment

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

Allowing avro_malloc(0) to return NULL means the subsequent memcpy may be invoked with NULL pointers when size == 0, which can be undefined behavior on some platforms. Please ensure the copy isn’t performed when size is zero (related issue also in avro_bytes_set).

🤖 React with 👍 or 👎 to let us know if the comment was useful.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Oct 27, 2025

Choose a reason for hiding this comment

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

value:useful; category:bug; feedback:The AI reviewer is correct that now if the bytes_copy is NULL and size is 0 the if check won't be true and memcpy() will be called with a NULL argument as a destination. The size should be checked for 0 as a first step in this function and return early.

avro_set_error("Cannot copy bytes content");
return NULL;
}
Expand Down Expand Up @@ -197,7 +197,7 @@ int avro_bytes_set(avro_datum_t datum, const char *bytes, const int64_t size)
{
int rval;
char *bytes_copy = (char *) avro_malloc(size);
if (!bytes_copy) {
if (!bytes_copy && size) {
Copy link
Copy Markdown

@augmentcode augmentcode bot Oct 27, 2025

Choose a reason for hiding this comment

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

As in avro_bytes, the later memcpy can receive NULL pointers when size == 0 due to the relaxed allocation check, which may be undefined behavior; avoid invoking the copy in the zero-length case. (Related to the earlier occurrence in this file)

🤖 React with 👍 or 👎 to let us know if the comment was useful.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Oct 27, 2025

Choose a reason for hiding this comment

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

value:useful; category:bug; feedback:The AI reviewer is correct that now if the bytes_copy is NULL and size is 0 the if check won't be true and memcpy() will be called with a NULL argument as a destination. The size should be checked for 0 as a first step in this function and return early.

avro_set_error("Cannot copy bytes content");
return ENOMEM;
}
Expand Down
4 changes: 2 additions & 2 deletions lang/c/src/value-json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ encode_utf8_bytes(const void *src, size_t src_len,

// Allocate a new buffer for the UTF-8 string and fill it in.
uint8_t *dest8 = (uint8_t *) avro_malloc(utf8_len);
if (dest8 == NULL) {
if (dest8 == NULL && utf8_len) {
avro_set_error("Cannot allocate JSON bytes buffer");
return ENOMEM;
}
Expand Down Expand Up @@ -126,7 +126,7 @@ avro_value_to_json_t(const avro_value_t *value)
return NULL;
}

json_t *result = json_stringn_nocheck((const char *) encoded, encoded_size);
json_t *result = json_stringn_nocheck((const char *) encoded ? encoded : "", encoded_size);
avro_free(encoded, encoded_size);
if (result == NULL) {
avro_set_error("Cannot allocate JSON bytes");
Expand Down
53 changes: 42 additions & 11 deletions lang/c/tests/test_avro_data.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,20 @@ test_allocator(void *ud, void *ptr, size_t osize, size_t nsize)
AVRO_UNUSED(osize);

if (nsize == 0) {
size_t *size = ((size_t *) ptr) - 1;
if (osize != *size) {
fprintf(stderr,
"Error freeing %p:\n"
"Size passed to avro_free (%" PRIsz ") "
"doesn't match size passed to "
"avro_malloc (%" PRIsz ")\n",
ptr, osize, *size);
abort();
//exit(EXIT_FAILURE);
if (ptr) {
size_t *size = ((size_t *) ptr) - 1;
if (osize != *size) {
fprintf(stderr,
"Error freeing %p:\n"
"Size passed to avro_free (%" PRIsz ") "
"doesn't match size passed to "
"avro_malloc (%" PRIsz ")\n",
ptr, osize, *size);
abort();
//exit(EXIT_FAILURE);
}
free(size);
}
free(size);
return NULL;
} else {
size_t real_size = nsize + sizeof(size_t);
Expand Down Expand Up @@ -214,6 +216,34 @@ static int test_bytes(void)
return 0;
}

static int test_empty_bytes(void)
{
char bytes[] = { };
avro_schema_t writer_schema = avro_schema_bytes();
avro_datum_t datum;
avro_datum_t expected_datum;

datum = avro_givebytes(bytes, sizeof(bytes), NULL);
write_read_check(writer_schema, datum, NULL, NULL, "bytes");
test_json(datum, "\"\"");
avro_datum_decref(datum);
avro_schema_decref(writer_schema);

datum = avro_givebytes(NULL, 0, NULL);
avro_givebytes_set(datum, bytes, sizeof(bytes), NULL);
expected_datum = avro_givebytes(bytes, sizeof(bytes), NULL);
if (!avro_datum_equal(datum, expected_datum)) {
fprintf(stderr,
"Expected equal bytes instances.\n");
exit(EXIT_FAILURE);
}
avro_datum_decref(datum);
avro_datum_decref(expected_datum);

avro_schema_decref(writer_schema);
return 0;
}
Comment on lines +219 to +245
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Good test coverage for empty bytes, but double schema decref.

The test properly exercises empty byte array handling including JSON encoding. However, there's a double decref of writer_schema at lines 230 and 243.

Apply this diff to remove the duplicate decref:

 	write_read_check(writer_schema, datum, NULL, NULL, "bytes");
 	test_json(datum, "\"\"");
 	avro_datum_decref(datum);
-	avro_schema_decref(writer_schema);

 	datum = avro_givebytes(NULL, 0, NULL);
 	avro_givebytes_set(datum, bytes, sizeof(bytes), NULL);

Note: The existing test_bytes function (lines 184-217) has the same issue with double decref at lines 195 and 215.

🤖 Prompt for AI Agents 
In lang/c/tests/test_avro_data.c around lines 219-245, the test_empty_bytes
function calls avro_schema_decref(writer_schema) twice (once after the first
datum block and again before return); remove the second/deeper duplicate decref
so writer_schema is decref'd only once. Also apply the same fix to the earlier
test_bytes function (around lines 184-217) where
avro_schema_decref(writer_schema) is duplicated (remove the later duplicate) to
ensure each schema is decref'd exactly once.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Oct 27, 2025

Choose a reason for hiding this comment

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

value:useful; category:bug; feedback:prevented a double decrement of the reference count of a schema. This won't lead to double free but it is still a bug.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Oct 27, 2025

Choose a reason for hiding this comment

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

@martin-augment, thank you for confirming! I appreciate the clarification that while it wouldn't cause a double free, preventing the incorrect reference count decrement is still valuable for maintaining proper memory management semantics in the codebase. Glad I could help catch this before it was merged! 🐰

    (\(\
    ( -.-)
    o_(")(")
    [Happy rabbit - glad to help!]


static int test_int32(void)
{
int i;
Expand Down Expand Up @@ -657,6 +687,7 @@ int main(void)
{
"string", test_string}, {
"bytes", test_bytes}, {
"empty_bytes", test_empty_bytes}, {
"int", test_int32}, {
"long", test_int64}, {
"float", test_float}, {
Expand Down