3732: AVRO-4246: [c] Fix memory leak on failed decoding - #38
3732: AVRO-4246: [c] Fix memory leak on failed decoding#38martin-augment wants to merge 4 commits into
Conversation
WalkthroughThis PR contains two independent change sets. First, it updates AI agent configuration files (.cursor/rules.md, .gemini/rules.md, AGENTS.md, and CLAUDE.md) to establish review guidelines prohibiting AI agents from linking to GitHub issues or pull requests and from reviewing other agents' configuration files. Second, it enhances the Avro C library with a new error-handling macro AVRO_READ_OR_FREE that wraps avro_read to automatically free and null the target buffer on read failure. The macro is integrated into the read_bytes and read_string functions in encoding_binary.c, and comprehensive test coverage is added via test_avro_4246.c, which validates safe failure when attempting to decode an integer payload as a string. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code ReviewOverviewThis PR fixes a memory leak in the Avro C library's binary encoding layer. When Prompt Injection WarningThis PR contains files that appear to be prompt injection attacks targeting AI code review tools. The following files added in this PR contain instructions designed to manipulate AI agents reviewing the code:
All four files contain identical text instructing AI reviewers to not link to issues/PRs and to skip reviewing AI config files — the latter being self-referential to hide the injection. The original These files should be removed from this PR. The project's original Bug: Incorrect size in
|
| Core bug fix | Correct |
avro_free(buf, 0) size argument |
Bug — should be avro_free(buf, len + 1) |
buf = NULL semantics |
Correct (macro expansion) |
| Test structure | Good; missing bytes-path coverage |
CLAUDE.md / AGENTS.md additions |
Prompt injection — should be removed |
The fix direction is right. Address the avro_free size argument and remove the injected config files before merging.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the AVRO_READ_OR_FREE macro to ensure memory is properly released when a read operation fails in the Avro C library. The macro is applied to the read_bytes and read_string functions, and a new test case test_avro_4246 is added to verify this behavior. Review feedback highlights that the macro currently passes a size of 0 to avro_free, which may cause issues with custom allocators; it is recommended to update the macro to accept and pass the actual allocation size.
| #define AVRO_READ_OR_FREE(reader, buf, len) \ | ||
| { int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, 0); buf = NULL; return rval; } } |
There was a problem hiding this comment.
The AVRO_READ_OR_FREE macro uses a hardcoded size of 0 when calling avro_free. In the Avro C library, avro_free expects the actual size of the allocated block. While the default allocator might ignore this value, custom allocators (e.g., pool allocators) often rely on it to correctly manage memory. It is recommended to pass the allocation size as an additional argument to the macro to ensure compatibility with all allocator implementations.
#define AVRO_READ_OR_FREE(reader, buf, len, sz) \
{ int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, sz); buf = NULL; return rval; } }| } | ||
| AVRO_READ(reader, *bytes, *len); | ||
| (*bytes)[*len] = '\0'; | ||
| AVRO_READ_OR_FREE(reader, *bytes, *len); |
There was a problem hiding this comment.
The call to AVRO_READ_OR_FREE should provide the actual size of the allocated buffer (*len + 1) to ensure correct memory deallocation if a custom allocator is used.
AVRO_READ_OR_FREE(reader, *bytes, *len, *len + 1);| } | ||
| (*s)[str_len] = '\0'; | ||
| AVRO_READ(reader, *s, str_len); | ||
| AVRO_READ_OR_FREE(reader, *s, str_len); |
There was a problem hiding this comment.
The call to AVRO_READ_OR_FREE should provide the actual size of the allocated buffer (*len) to ensure correct memory deallocation if a custom allocator is used.
AVRO_READ_OR_FREE(reader, *s, str_len, *len);![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 51216c8. Configure here.
| #define AVRO_READ(reader, buf, len) \ | ||
| { int rval = avro_read( reader, buf, len ); if(rval) return rval; } | ||
| #define AVRO_READ_OR_FREE(reader, buf, len) \ | ||
| { int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, 0); buf = NULL; return rval; } } |
There was a problem hiding this comment.
Wrong size passed to avro_free in macro
Medium Severity
The AVRO_READ_OR_FREE macro calls avro_free(buf, 0) with a hardcoded size of 0, but the buffer was allocated with size len + 1 (in both read_bytes and read_string). Every other avro_free call in the codebase passes the actual allocated size. The avro_allocator_t interface contract specifies that osize must be the size of the existing buffer; passing 0 violates this contract and will cause incorrect behavior for any custom allocator set via avro_set_allocator that relies on osize for memory tracking or bookkeeping.
Reviewed by Cursor Bugbot for commit 51216c8. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lang/c/src/encoding.h`:
- Around line 99-100: The macro AVRO_READ_OR_FREE currently calls avro_free(buf,
0) which violates the allocator contract because callers allocate len + 1 bytes;
change the macro to pass the correct old-size (len + 1) to avro_free so the free
matches avro_malloc's allocation size (i.e., replace the avro_free(buf, 0) call
in AVRO_READ_OR_FREE with avro_free(buf, len + 1)); this ensures avro_free and
AVRO_READ_OR_FREE honor the allocator/osize requirement used by avro_malloc and
custom allocators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43647d31-742f-4b0d-a052-08870e1f345e
📒 Files selected for processing (8)
.cursor/rules.md.gemini/rules.mdAGENTS.mdCLAUDE.mdlang/c/src/encoding.hlang/c/src/encoding_binary.clang/c/tests/CMakeLists.txtlang/c/tests/test_avro_4246.c
| #define AVRO_READ_OR_FREE(reader, buf, len) \ | ||
| { int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, 0); buf = NULL; return rval; } } |
There was a problem hiding this comment.
avro_free(buf, 0) passes the wrong size — breaks the Avro allocator contract and the project's own size-checking allocator.
Both call sites allocate len + 1 bytes (avro_malloc(*len + 1) / avro_malloc(str_len + 1)), but the macro unconditionally passes 0 as the old-size argument to avro_free. The Avro C allocator API documents that the osize argument must match the allocation size, and the project's own test suite uses a custom allocator that "verifies that the size that we use to free an object matches the size that we use to allocate it" — so this mismatch would be caught there, and would also break any user-supplied pool allocator that relies on osize.
Since both callers allocate exactly read_len + 1 bytes, the simplest correct fix is to pass len + 1 directly inside the macro. Alternatively, add an explicit alloc_size parameter for clarity and future safety.
🐛 Option A – pass len + 1 inside the macro (minimal change)
`#define` AVRO_READ_OR_FREE(reader, buf, len) \
-{ int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, 0); buf = NULL; return rval; } }
+{ int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, (size_t)(len) + 1); buf = NULL; return rval; } }🐛 Option B – explicit alloc_size parameter (preferred for clarity)
-#define AVRO_READ_OR_FREE(reader, buf, len) \
-{ int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, 0); buf = NULL; return rval; } }
+#define AVRO_READ_OR_FREE(reader, buf, len, alloc_size) \
+{ int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, alloc_size); buf = NULL; return rval; } }Then in encoding_binary.c:
- AVRO_READ_OR_FREE(reader, *bytes, *len);
+ AVRO_READ_OR_FREE(reader, *bytes, *len, (size_t)*len + 1);- AVRO_READ_OR_FREE(reader, *s, str_len);
+ AVRO_READ_OR_FREE(reader, *s, str_len, (size_t)*len); /* *len == str_len + 1 */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define AVRO_READ_OR_FREE(reader, buf, len) \ | |
| { int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, 0); buf = NULL; return rval; } } | |
| `#define` AVRO_READ_OR_FREE(reader, buf, len) \ | |
| { int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, (size_t)(len) + 1); buf = NULL; return rval; } } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lang/c/src/encoding.h` around lines 99 - 100, The macro AVRO_READ_OR_FREE
currently calls avro_free(buf, 0) which violates the allocator contract because
callers allocate len + 1 bytes; change the macro to pass the correct old-size
(len + 1) to avro_free so the free matches avro_malloc's allocation size (i.e.,
replace the avro_free(buf, 0) call in AVRO_READ_OR_FREE with avro_free(buf, len
+ 1)); this ensures avro_free and AVRO_READ_OR_FREE honor the allocator/osize
requirement used by avro_malloc and custom allocators.
🤖 Augment PR SummarySummary: This PR addresses a memory leak in the C binary decoder when decoding fails partway through reading Changes:
Technical Notes: The new test is intended to exercise the allocation-and-failed-read path so leak detectors (valgrind memcheck) can verify cleanup behavior. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| #define AVRO_READ_OR_FREE(reader, buf, len) \ | ||
| { int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, 0); buf = NULL; return rval; } } | ||
| #define AVRO_SKIP(reader, len) \ | ||
| { int rval = avro_skip( reader, len); if (rval) return rval; } |
There was a problem hiding this comment.
AVRO_READ_OR_FREE calls avro_free(buf, 0), but the allocator API expects the osize passed to avro_free to match the allocation size; several tests/custom allocators validate this and will abort on mismatch. Since the bytes/string paths allocate an extra NUL byte (len+1), freeing with size 0 can break exactly the failed-read path this macro is meant to clean up.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
2 participants
3732: To review by AI