AVRO-4293: [c] Bound allocation when decoding length-prefixed values and collections by iemejia · Pull Request #3858 · apache/avro

iemejia · 2026-07-11T21:20:14Z

What is the purpose of the change

A bytes or string value is encoded as a length prefix followed by that many bytes of data, and an array or map block is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.

This applies the equivalent of the Java SDK fix AVRO-4241 to the C SDK and extends it to collections. It has two complementary parts.

1. Validate available bytes before allocating

When the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as null) is never falsely rejected. Sources that cannot report their remaining size are unaffected.

avro_reader_bytes_available() returns the number of bytes still readable from a memory-backed reader (or -1 when unknown, e.g. file readers). read_bytes/read_string consult it before allocating, and read_array_value/read_map_value reject a block whose element count could not be backed by the bytes remaining, computing min_bytes_per_element() from the element schema.

2. Cap collection allocation for zero-byte elements

Zero-byte elements (null, a zero-length fixed, or a record with only zero-byte fields) consume no input, so the available-bytes check cannot bound their count: a tiny payload such as {{"type":"array","items":"null"}} declaring a block count of 200,000,000 would otherwise drive an unbounded allocation. In addition to the available-bytes check, ensure_collection_available caps the cumulative count of zero-byte elements (AVRO_DEFAULT_MAX_COLLECTION_ITEMS = 10,000,000) and applies a structural cap to every collection (AVRO_DEFAULT_MAX_COLLECTION_STRUCTURAL = Integer.MAX_VALUE - 8) covering readers that cannot report bytes remaining. The limits and a min_bytes_per_element helper are exposed via avro_private.h so the datum skip path (datum_skip.c skip_array/skip_map) is bounded the same way. This also fixes a latent NULL-dereference in avro_raw_map_get_or_create, where the result of avro_raw_array_append was dereferenced before the NULL check. When set, the AVRO_MAX_COLLECTION_ITEMS environment variable caps both limits.

This folds in and supersedes the standalone collection-limit change (AVRO-4279, #3848), so this PR is the single complete fix for collection/length-prefixed allocation DoS in the C SDK.

This is a sub-task of AVRO-4292 and resolves AVRO-4293.

Verifying this change

This change added tests and can be verified as follows:

Added lang/c/tests/test_avro_4293.c covering over-limit string/bytes/array/map rejection, a within-limit value, an array<null> with a huge block count that must be rejected without allocating, and a small array<null> that still decodes.
Run: cd lang/c && mkdir build && cd build && cmake .. && make && ctest

Documentation

Does this pull request introduce a new feature? (no — hardening / robustness)
If yes, how is the feature documented? (not applicable)

…prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data, causing a correspondingly huge allocation or an unbounded append loop. - avro_reader_bytes_available() returns the bytes still readable from a memory-backed reader (or -1 when unknown). read_bytes/read_string consult it to reject an over-large declared length before allocating. - read_array_value/read_map_value reject a block whose element count could not be backed by the bytes remaining, using min_bytes_per_element() computed from the element schema so a zero-byte element type (e.g. null) is not falsely rejected. The comparison divides to avoid overflow. Mirrors the Java SDK's checks (AVRO-4241). Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Hardens the C Avro binary decoder against malicious/truncated inputs that declare large bytes/string lengths or array/map block counts by rejecting impossible sizes before allocations when the reader can report remaining bytes (memory-backed readers).

Changes:

Added avro_reader_bytes_available() for memory readers and used it to pre-check declared bytes/string lengths.
Added array/map block-count validation based on min_bytes_per_element() to reject blocks that cannot be backed by remaining bytes.
Added a new regression test (test_avro_4293) and wired it into the C test suite.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
lang/c/tests/test_avro_4293.c	Adds tests intended to cover early rejection for oversized length/count cases and validates non-rejection for zero-byte elements (`null`).
lang/c/tests/CMakeLists.txt	Registers the new `test_avro_4293` in the test suite.
lang/c/src/value-read.c	Adds per-element minimum sizing and collection block-count validation before appending/allocating decoded elements.
lang/c/src/io.c	Implements `avro_reader_bytes_available()` for memory-backed readers (returns -1 for unknown/file).
lang/c/src/encoding.h	Declares `avro_reader_bytes_available()` for internal use.
lang/c/src/encoding_binary.c	Adds pre-allocation checks for `read_bytes` and `read_string` using remaining-byte reporting.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iemejia · 2026-07-11T22:28:23Z

+	if (schema == NULL || depth > 64) {
+		return 0;
+	}


Fixed in 896a688: min_bytes_per_element() now returns 1 (not 0) when the depth guard trips, so the collection check stays enabled for a crafted deep/recursive schema; a valid recursive value always encodes to at least 1 byte.

iemejia · 2026-07-11T22:28:25Z

+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdint.h>
+#include <avro.h>


Fixed in 896a688: the test now includes <errno.h>.

iemejia · 2026-07-11T22:28:26Z

+	rc = try_decode(iface, oversized, sizeof(oversized));
+	if (rc == 0) {
+		fprintf(stderr, "%s: FAIL - oversized length was accepted\n", label);
+	} else {
+		fprintf(stderr, "%s: oversized length rejected as expected: %s\n",
+			label, avro_strerror());
+		ret = EXIT_SUCCESS;
+	}


Fixed in 896a688: the test now asserts the rejection code is EINVAL (what the availability checks return), so it no longer passes on the pre-fix ENOSPC-after-allocation behavior.

… EINVAL Review feedback: - min_bytes_per_element() returned 0 when the depth guard tripped, which made ensure_collection_available() skip the check and could bypass the guard for a crafted deep/recursive schema. Return 1 instead so the check stays enabled; a valid recursive value always encodes to at least 1 byte. - The test now includes <errno.h> and asserts the rejection code is EINVAL (the availability checks' error) rather than accepting any non-zero return, which would also pass on the pre-fix ENOSPC-after-allocation behavior. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

iemejia · 2026-07-11T23:06:25Z

+		for (i = 0; i < n; i++) {
+			avro_schema_t  field =
+			    avro_schema_record_field_get_by_index(schema, i);
+			total += min_bytes_per_element(field, depth + 1);
+		}


Fixed in b549013: the record field-minima sum now saturates to INT64_MAX instead of overflowing, so a wrapped negative total can no longer disable the collection check.

iemejia · 2026-07-11T23:06:26Z

+	/* Map keys are strings (>= 1 byte length prefix) plus the value. */
+	min_bytes = 1 + min_bytes_per_element(
+	    map_schema ? avro_schema_map_values(map_schema) : NULL, 0);


Fixed in b549013: the map computation saturates the key's +1 (only adds when below INT64_MAX), so a maxed-out value minimum cannot wrap.

iemejia · 2026-07-11T23:06:27Z

+	reader = avro_reader_memory(valid, sizeof(valid));
+	rc = avro_value_read(reader, &decoded);


Fixed in b549013: check_accepts_valid() now checks avro_reader_memory() for NULL before use.

iemejia · 2026-07-11T23:06:28Z

+	reader = avro_reader_memory(null_array, sizeof(null_array));
+	rc = avro_value_read(reader, &decoded);


Fixed in b549013: check_accepts_null_array() now checks avro_reader_memory() for NULL before use.

Review feedback: - min_bytes_per_element() now saturates the record field-minima sum to INT64_MAX instead of overflowing (a wrapped negative total would disable the collection check), and the map computation saturates the +1 for the key. - The valid-string and null-array tests now check avro_reader_memory() for NULL before use, so an allocation failure cannot dereference a NULL reader. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Review feedback (consistency with the C++ fix): the depth cutoff was applied before checking the schema type, so a zero-byte leaf type (e.g. null) nested under deeply chained records returned 1 instead of 0, enabling the collection check and potentially rejecting valid data. Move the depth guard into the AVRO_RECORD case; leaf types now return their true minimum (null -> 0) regardless of nesting depth. A cyclic link still terminates because it resolves through a record, which is depth-guarded. Assisted-by: GitHub Copilot:claude-opus-4.8

Completes the available-bytes protection for collections and supersedes the separate collection-limit change. Elements whose schema encodes to zero bytes (null, a zero-length fixed, or a record with only zero-byte fields) consume no input, so the bytes-remaining check cannot bound their count. A tiny payload declaring a huge array block count of such elements (e.g. {"type":"array","items":"null"} with a count of 200,000,000) therefore drove an unbounded avro_raw_array growth and exhausted memory. ensure_collection_available now enforces the bytes-remaining check plus a zero-byte cap (AVRO_DEFAULT_MAX_COLLECTION_ITEMS = 10,000,000) and a structural cap on all collections (AVRO_DEFAULT_MAX_COLLECTION_STRUCTURAL = Integer.MAX_VALUE - 8) covering readers that cannot report bytes remaining. AVRO_MAX_COLLECTION_ITEMS, when set, caps both. The limits and a min_bytes_per_element helper are exposed via avro_private.h so the datum skip path (datum_skip.c skip_array/skip_map) is bounded the same way, preventing an unbounded skip loop. Also fix a latent NULL-dereference in avro_raw_map_get_or_create: the result of avro_raw_array_append was dereferenced before the NULL check; reorder so the check happens first, returning ENOMEM cleanly on allocation failure. Assisted-by: GitHub Copilot:claude-opus-4.8

iemejia · 2026-07-12T13:05:58Z

This PR now also includes the collection block-count cap for [c], so it is the single complete fix for collection allocation DoS in this SDK. In addition to validating available bytes before allocating length-prefixed values, it bounds the number of array/map items per block:

a heap-aware cap for zero-byte-element collections (e.g. array<null>), which otherwise bypass the available-bytes check because each element reads 0 bytes;
a structural cap for all collections;
bounded skip paths so projection/skip cannot loop unboundedly.

With this, the standalone collection-limit change for [c] (AVRO-4279, #3848) is redundant and is being closed as superseded by this PR.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

iemejia · 2026-07-12T17:32:04Z

+			       *len, available);
+		return EINVAL;
+	}
 	*bytes = (char *) avro_malloc(*len + 1);


Fixed in cdf28ee: read_bytes now rejects a length above SIZE_MAX - 1 and computes the allocation as (size_t) *len + 1, so the +1 cannot overflow the size_t allocation size.

iemejia · 2026-07-12T17:32:06Z

 	*len = str_len + 1;
 	*s = (char *) avro_malloc(*len);
 	if (!*s) {


Fixed in cdf28ee: read_string applies the same SIZE_MAX - 1 bound and does the +1 in size_t before allocating.

iemejia · 2026-07-12T17:32:07Z

+	reader = avro_reader_memory(buf, len);
+	rc = avro_skip_data(reader, schema);
+	avro_reader_free(reader);
+	avro_schema_decref(schema);


Fixed in cdf28ee: check_skip_null_collection_rejected() now checks avro_reader_memory() for NULL (freeing the schema) before use.

iemejia · 2026-07-12T17:32:08Z

+	/* With a lowered limit, boundary behavior: 1000 accepted, 1001 rejected. */
+	setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1);
+	if (check_null_array_accepted(1000, "array<null> 1000 within limit") != EXIT_SUCCESS) {
+		unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+		return EXIT_FAILURE;
+	}
+	if (check_null_collection_rejected(array_null, 1001, 0,
+					   "array<null> 1001 over limit") != EXIT_SUCCESS) {
+		unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+		return EXIT_FAILURE;
+	}
+	if (check_null_array_cumulative_rejected(600, "array<null> cumulative 600+600") != EXIT_SUCCESS) {
+		unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+		return EXIT_FAILURE;
+	}
+	/* Skipping a huge zero-byte array is bounded too. */
+	if (check_skip_null_collection_rejected(array_null, 5000,
+						"skip array<null> over limit") != EXIT_SUCCESS) {
+		unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+		return EXIT_FAILURE;
+	}
+	unsetenv("AVRO_MAX_COLLECTION_ITEMS");


Fixed in cdf28ee: the env var is now set/unset through portable helpers that use setenv/unsetenv, or _putenv_s under MSVC (empty value unsets).

iemejia · 2026-07-12T17:32:09Z

+	/* A backed non-zero-byte array is bounded by the structural cap. */
+	setenv("AVRO_MAX_COLLECTION_ITEMS", "5", 1);
+	if (check_structural_cap_rejected("array<long> over structural cap") != EXIT_SUCCESS) {
+		unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+		return EXIT_FAILURE;
+	}
+	unsetenv("AVRO_MAX_COLLECTION_ITEMS");


Fixed in cdf28ee: same portable helper is used here (and for every other set/unset in the test).

Folds in AVRO-4275. A negative block count was negated with block_count * -1; for INT64_MIN this is signed-integer overflow (undefined behavior, CWE-190). On common platforms the result stays negative, and the subsequent cast to size_t in the loop yields an enormous iteration count, driving unbounded allocation. The zig-zag encoding of INT64_MIN is a valid 10-byte varint, so this is reachable from malformed input. Reject INT64_MIN before negating and use -block_count elsewhere, across all three decoder paths: the value reader (value-read.c), the datum consumer (consume-binary.c), and the datum skipper (datum_skip.c). Adds test_avro_4275 covering INT64_MIN rejection and correct handling of valid negative block counts for both arrays and maps. Assisted-by: GitHub Copilot:claude-opus-4.8

iemejia · 2026-07-12T16:28:15Z

This PR now also folds in the INT64_MIN block-count fix from AVRO-4275: a negative block count whose absolute value is INT64_MIN cannot be negated in int64_t (block_count * -1 is signed-overflow UB, CWE-190), which drove an unbounded loop after the cast to size_t. The value reader, the datum consumer (consume-binary.c) and the datum skipper (datum_skip.c) now reject it, with test_avro_4275 covering both arrays and maps. AVRO-4275 / #3842 is therefore superseded by this PR.

Addresses review feedback: - read_bytes/read_string bound the declared length to SIZE_MAX - 1 before computing the avro_malloc size, and do the +1 (NUL terminator) arithmetic in size_t. Previously a huge declared length could overflow *len + 1 when converted to size_t, undersizing the buffer and risking an out-of-bounds read/write (notably on 32-bit). - test_avro_4293: check_skip_null_collection_rejected() now checks avro_reader_memory() for NULL before use, and the collection-limit env var is set/unset through portable helpers (setenv/unsetenv, or _putenv_s under MSVC) so the test compiles on Windows toolchains. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

iemejia · 2026-07-12T18:43:53Z

+		raw_entry->key = avro_strdup(key);
+		st_insert((st_table *) map->indices_by_key,
+			  (st_data_t) raw_entry->key, (st_data_t) i);
 		el = ((char *) raw_entry) + sizeof(avro_raw_map_entry_t);
 		is_new = 1;


Fixed in 36b71c2: the key is now copied via avro_str_alloc() with an explicit NULL check (avro_strdup() memcpy()s into its buffer without checking, so it crashes on OOM). The entry is indexed only once the copy succeeds; on failure the just-appended element is rolled back (element_count--), so the map isn't left with a NULL-key entry.

iemejia · 2026-07-12T18:43:54Z

+/* An array of nulls: null elements occupy zero bytes, so a large declared
+ * count is legitimate and must not be rejected. */


Fixed in 36b71c2: reworded — the comment no longer says a large null-array count 'must not be rejected' (the zero-byte item cap does bound it); it verifies a moderate count is not rejected by the available-bytes check.

Addresses review feedback: - avro_raw_map_get_or_create duplicated the key with avro_strdup(), which memcpy()s into its buffer without a NULL check and so crashes on OOM. Copy the key via avro_str_alloc() with an explicit NULL check, index it only once the copy succeeds, and on failure roll back the just-appended element so the map is not left with a half-initialized (NULL-key) entry. - check_accepts_null_array: reword the comment so it no longer says a large null-array count "must not be rejected" (the zero-byte item cap does bound it); it verifies a moderate count is not rejected by the available-bytes check. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-13T01:13:44Z

+			enum_schema = avro_value_get_schema(dest);
+			symbol_count =
+			    avro_schema_enum_number_of_symbols(enum_schema);
+			if (val < 0 || val >= symbol_count) {


Fixed — the AVRO_ENUM case now checks is_avro_enum(enum_schema) and returns EINVAL for a NULL/non-enum schema before calling avro_schema_enum_number_of_symbols(), so a negative error code can't make the range check accept out-of-range indices.

Already handled — the enum path checks is_avro_enum(enum_schema) and rejects a non-enum/NULL schema before the range check, so a negative number_of_symbols can't make it accept out-of-range values (value-read.c:548).

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

read_value's AVRO_ENUM case called avro_schema_enum_number_of_symbols() on avro_value_get_schema(dest) without confirming it is an enum schema. A custom value interface returning NULL or a non-enum schema yields a negative error code, against which the range check would accept out-of-range indices. Reject a non-enum schema (is_avro_enum) up front. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-13T01:31:57Z

+	if ((uint64_t) *len > (uint64_t) (SIZE_MAX - 1)) {
+		avro_set_error("Bytes length %" PRId64
+			       " exceeds the maximum allocatable size", *len);
+		return EINVAL;
+	}


Fixed — read_bytes now also rejects len > INT64_MAX - 1 (matching read_string), so the AVRO_BYTES caller's len + 1 in value-read.c can't overflow on 64-bit platforms where SIZE_MAX > INT64_MAX.

Already handled — read_bytes has the same *len > INT64_MAX - 1 bound before the +1 used by the AVRO_BYTES caller (encoding_binary.c:153-154).

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

read_bytes bounded the length to SIZE_MAX-1 but, on 64-bit platforms where SIZE_MAX > INT64_MAX, still allowed len == INT64_MAX. The AVRO_BYTES caller in value-read.c then computes len + 1 (avro_wrapped_alloc_new), which overflows. Add the INT64_MAX - 1 bound, matching read_string. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-13T04:45:07Z

+/* Decodes buf against the given schema and returns the avro_value_read rc.
+ * A non-zero rc means the read was rejected. */
+static int try_decode(avro_value_iface_t *iface, const char *buf, size_t buf_size)


Fixed — the header now states it returns the avro_value_read rc (0 ok, >0 rejected) or -1 on harness setup failure, and that callers distinguish the two.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

try_decode() returns -1 on harness setup failures in addition to the avro_value_read rc. Document that so a -1 is not mistaken for a read rejection. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

github-actions Bot added the C label Jul 11, 2026

iemejia requested a review from Copilot July 11, 2026 21:23

Copilot started reviewing on behalf of iemejia July 11, 2026 21:24 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 22:34

Copilot started reviewing on behalf of iemejia July 11, 2026 22:35 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 23:07

Copilot started reviewing on behalf of iemejia July 11, 2026 23:08 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia added 2 commits July 12, 2026 07:14

iemejia mentioned this pull request Jul 12, 2026

AVRO-4279: [c] Bound collection size when decoding arrays and maps #3848

Closed

iemejia changed the title ~~AVRO-4293: [c] Validate available bytes before allocating for length-prefixed values~~ AVRO-4293: [c] Bound allocation when decoding length-prefixed values and collections Jul 12, 2026

iemejia requested a review from Copilot July 12, 2026 14:32

Copilot started reviewing on behalf of iemejia July 12, 2026 14:32 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia mentioned this pull request Jul 12, 2026

AVRO-4275: [C] Fix signed integer overflow in block_count negation #3842

Closed

iemejia requested a review from Copilot July 12, 2026 17:34

Copilot started reviewing on behalf of iemejia July 12, 2026 17:34 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 18:46

iemejia requested a review from Copilot July 13, 2026 00:29

Copilot started reviewing on behalf of iemejia July 13, 2026 00:29 View session