Copilot commented on code in PR #3858:
URL: https://github.com/apache/avro/pull/3858#discussion_r3567396079
##########
lang/c/src/value-read.c:
##########
@@ -38,6 +40,170 @@ 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) {
+ /* Safety net for a pathologically deep (or cyclic)
+ * schema. Return 0 (a valid conservative lower bound)
+ * rather than over-estimating: if the true minimum
+ * cannot be computed, treat the element as possibly
+ * zero-byte so the tighter zero-byte collection cap is
+ * applied instead of the much larger structural cap. */
+ return 0;
+ }
+ 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) {
+ /* Clamp to INT64_MAX and to SIZE_MAX: the block count
is
+ * later cast to size_t in the read loop, so a limit
above
+ * SIZE_MAX would permit a count that truncates on cast
+ * (notably on 32-bit) and weaken the bound. */
+ uint64_t v = (uint64_t) value;
+ if (v > (uint64_t) INT64_MAX) {
+ v = (uint64_t) INT64_MAX;
+ }
+ if (v > (uint64_t) SIZE_MAX) {
+ v = (uint64_t) SIZE_MAX;
+ }
+ *zero_byte = *structural = (int64_t) v;
+ return;
+ }
+ }
+ *zero_byte = AVRO_DEFAULT_MAX_COLLECTION_ITEMS;
+ *structural = AVRO_DEFAULT_MAX_COLLECTION_STRUCTURAL;
+}
+
+/*
+ * Reject a collection (array or map) block that could not be backed by the
+ * input, before appending.
+ *
+ * For elements with a positive minimum on-wire size, the declared count is
+ * checked against the bytes remaining. For zero-byte elements (e.g. an array
of
+ * nulls), which consume no input and so cannot be bounded by the bytes
+ * remaining, the cumulative count is checked against a configurable limit.
+ */
+static int
+ensure_collection_available(avro_reader_t reader, int64_t existing,
+ int64_t count, int64_t min_bytes)
+{
+ int64_t available;
+ int64_t zero_byte, structural;
+ if (count <= 0) {
+ return 0;
+ }
+ avro_collection_limits(&zero_byte, &structural);
+ if (min_bytes > 0) {
Review Comment:
ensure_collection_available() calls avro_collection_limits() on every block,
which re-reads/parses AVRO_MAX_COLLECTION_ITEMS (getenv + strtoll) inside the
decode loop. For arrays/maps with many blocks this adds avoidable overhead and
also makes the effective limit depend on env-var changes mid-decode.
Consider fetching the limits once per read_array_value/read_map_value
invocation (before the while-loop) and passing zero_byte/structural into
ensure_collection_available (or inlining the checks) so the env var is
consulted once per decode.
##########
lang/c/src/map.c:
##########
@@ -106,16 +106,31 @@ int avro_raw_map_get_or_create(avro_raw_map_t *map, const
char *key,
el = (char *) raw_entry + sizeof(avro_raw_map_entry_t);
is_new = 0;
} else {
+ char *key_copy;
+ size_t key_size;
i = map->elements.element_count;
avro_raw_map_entry_t *raw_entry =
(avro_raw_map_entry_t *)
avro_raw_array_append(&map->elements);
- raw_entry->key = avro_strdup(key);
- st_insert((st_table *) map->indices_by_key,
- (st_data_t) raw_entry->key, (st_data_t) i);
if (!raw_entry) {
- avro_str_free((char*)raw_entry->key);
+ /* avro_raw_array_append returns NULL on allocation
+ * failure; check before dereferencing it. */
+ return -ENOMEM;
+ }
+ /* Duplicate the key with a NULL-safe allocation: avro_strdup()
+ * memcpy()s into the buffer without checking it, so it would
+ * crash on OOM. Only index the entry once the copy succeeds; on
+ * failure, roll back the element we just appended so the map is
+ * not left with a half-initialized (NULL-key) entry. */
+ key_size = strlen(key) + 1;
+ key_copy = avro_str_alloc(key_size);
+ if (!key_copy) {
+ map->elements.element_count--;
return -ENOMEM;
}
+ memcpy(key_copy, key, key_size);
+ raw_entry->key = key_copy;
+ st_insert((st_table *) map->indices_by_key,
+ (st_data_t) raw_entry->key, (st_data_t) i);
Review Comment:
This code now avoids NULL dereferences on OOM from
avro_raw_array_append()/avro_strdup(), but st_insert() itself allocates an
st_table_entry via avro_new() without checking for NULL (see
lang/c/src/st.c:260-267). On allocation failure, st_insert() can still
dereference NULL and crash, so this path remains non-OOM-safe.
Consider making st_insert()/ADD_DIRECT() handle allocation failure (e.g.,
return an error code and leave the table unchanged), and then here roll back
element_count and free key_copy if the insert fails.
--
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]
