Copilot commented on code in PR #3858:
URL: https://github.com/apache/avro/pull/3858#discussion_r3566908339
##########
lang/c/src/value-read.c:
##########
@@ -38,6 +40,158 @@ static int
read_value(avro_reader_t reader, avro_value_t *dest);
+/*
+ * Minimum number of bytes a single value of the given schema can occupy on the
+ * wire. Used to reject an array/map block count that could not be backed by
the
+ * bytes remaining. A type that can encode to zero bytes (null) returns 0,
which
+ * disables the collection check for it (so an array of nulls is not falsely
+ * rejected). A recursion guard breaks self-referencing schemas.
+ */
+static int64_t
+min_bytes_per_element(avro_schema_t schema, int depth)
+{
+ if (schema == NULL) {
+ return 0;
+ }
+ switch (avro_typeof(schema)) {
+ case AVRO_NULL:
+ return 0;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ case AVRO_FIXED:
+ return avro_schema_fixed_size(schema);
+ case AVRO_RECORD: {
+ size_t n = avro_schema_record_size(schema);
+ int64_t total = 0;
+ size_t i;
+ if (depth > 64) {
+ /* A cyclic or pathologically deep record. Return 1 (not
+ * 0) so the collection check stays enabled; a valid
+ * recursive value always encodes to >= 1 byte. The
depth
+ * guard is applied only here, so zero-byte leaf types
+ * such as null still return 0 regardless of depth. */
+ return 1;
+ }
Review Comment:
The depth guard in min_bytes_per_element() returns 1 when depth > 64. That
can misclassify a deeply-nested but still zero-byte record (e.g.,
record-of-record-of-…-of-null) as non-zero-byte, which bypasses the tighter
zero-byte collection cap and falls back to the much larger structural cap for
readers that cannot report bytes remaining (file readers). This undermines the
intended protection against huge allocations for zero-byte elements.
Consider making the guard return 0 (treat as possibly zero-byte) so the
zero-byte collection limit is applied conservatively when the minimum size
cannot be computed reliably.
##########
lang/c/src/value-read.c:
##########
@@ -38,6 +40,158 @@ static int
read_value(avro_reader_t reader, avro_value_t *dest);
+/*
+ * Minimum number of bytes a single value of the given schema can occupy on the
+ * wire. Used to reject an array/map block count that could not be backed by
the
+ * bytes remaining. A type that can encode to zero bytes (null) returns 0,
which
+ * disables the collection check for it (so an array of nulls is not falsely
+ * rejected). A recursion guard breaks self-referencing schemas.
+ */
+static int64_t
+min_bytes_per_element(avro_schema_t schema, int depth)
+{
+ if (schema == NULL) {
+ return 0;
+ }
+ switch (avro_typeof(schema)) {
+ case AVRO_NULL:
+ return 0;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ case AVRO_FIXED:
+ return avro_schema_fixed_size(schema);
+ case AVRO_RECORD: {
+ size_t n = avro_schema_record_size(schema);
+ int64_t total = 0;
+ size_t i;
+ if (depth > 64) {
+ /* A cyclic or pathologically deep record. Return 1 (not
+ * 0) so the collection check stays enabled; a valid
+ * recursive value always encodes to >= 1 byte. The
depth
+ * guard is applied only here, so zero-byte leaf types
+ * such as null still return 0 regardless of depth. */
+ return 1;
+ }
+ for (i = 0; i < n; i++) {
+ avro_schema_t field =
+ avro_schema_record_field_get_by_index(schema, i);
+ int64_t field_min = min_bytes_per_element(field, depth
+ 1);
+ /* Saturate rather than overflow: a wrapped (negative)
+ * total would disable the collection check. */
+ if (field_min > INT64_MAX - total) {
+ return INT64_MAX;
+ }
+ total += field_min;
+ }
+ return total;
+ }
+ case AVRO_LINK:
+ return min_bytes_per_element(avro_schema_link_target(schema),
+ depth + 1);
+ default:
+ /* boolean, int, long, bytes, string, enum, union, array, map:
+ * all encode to at least one byte. */
+ return 1;
+ }
+}
+
+/* Non-static wrapper so the skip path (datum_skip.c) can compute the minimum
+ * on-wire size of a collection's element schema. */
+int64_t
+avro_min_bytes_per_element(avro_schema_t schema)
+{
+ return min_bytes_per_element(schema, 0);
+}
+
+/*
+ * Maximum number of zero-byte-encoded collection elements (e.g. an array of
+ * nulls) to allocate from a single decode. Such elements consume no input, so
+ * the bytes-remaining check cannot bound their count; without a cap a tiny
+ * payload can declare a huge block count and exhaust memory. Overridable via
+ * the AVRO_MAX_COLLECTION_ITEMS environment variable.
+ */
+#define AVRO_DEFAULT_MAX_COLLECTION_ITEMS ((int64_t) 10000000)
+
+/*
+ * Structural cap on the number of elements in any array or map (an overflow /
+ * defense-in-depth guard), matching the historical Integer.MAX_VALUE - 8
limit.
+ * Non-zero-byte elements are also bounded by the bytes remaining.
+ */
+#define AVRO_DEFAULT_MAX_COLLECTION_STRUCTURAL ((int64_t) 2147483639)
+
+/*
+ * Populate the zero-byte and structural collection limits. The
+ * AVRO_MAX_COLLECTION_ITEMS environment variable, when a non-negative integer,
+ * caps both; otherwise the tighter zero-byte default and the structural
default
+ * are used. Shared with the skip path (see avro_collection_limits).
+ */
+void
+avro_collection_limits(int64_t *zero_byte, int64_t *structural)
+{
+ const char *env = getenv("AVRO_MAX_COLLECTION_ITEMS");
+ if (env != NULL && *env != '\0') {
+ char *end;
+ long long value = strtoll(env, &end, 10);
+ if (*end == '\0' && value >= 0) {
+ *zero_byte = *structural = (int64_t) value;
+ return;
+ }
+ }
+ *zero_byte = AVRO_DEFAULT_MAX_COLLECTION_ITEMS;
+ *structural = AVRO_DEFAULT_MAX_COLLECTION_STRUCTURAL;
+}
Review Comment:
avro_collection_limits() assigns the parsed AVRO_MAX_COLLECTION_ITEMS
directly to int64_t without clamping. On platforms where size_t is smaller than
int64_t (e.g., 32-bit), a configured limit larger than SIZE_MAX can later lead
to truncation when block_count is cast to size_t in
read_array_value/read_map_value loops, causing incorrect decoding and
potentially weakening the intended allocation bound.
Clamping the effective limit to the maximum representable size_t (and to
INT64_MAX) avoids these truncation paths while still honoring the env var for
reasonable values.
--
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]
