iemejia commented on code in PR #3858:
URL: https://github.com/apache/avro/pull/3858#discussion_r3567440398
##########
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:
Fixed — read_array_value/read_map_value now call avro_collection_limits()
once before the block loop and pass zero_byte/structural into
ensure_collection_available, so the env var is parsed once per decode (and the
limit is stable across blocks).
##########
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:
Good point, but this is a pre-existing limitation of the vendored st.c hash
table (st_insert -> avro_new without a NULL check), which is used pervasively
across the C SDK; changing st_insert's contract to return an allocation error
is a broader change beyond this collection-allocation DoS fix. This PR removed
the NULL-deref on avro_raw_array_append/avro_strdup in map.c; I've filed the
st_insert OOM-safety as a separate follow-up rather than expanding the scope
here.
--
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]
