Copilot commented on code in PR #3848:
URL: https://github.com/apache/avro/pull/3848#discussion_r3564362093
##########
lang/c/src/value-read.c:
##########
@@ -38,6 +41,55 @@ static int
read_value(avro_reader_t reader, avro_value_t *dest);
+/*
+ * The block count of an array or map is read from the (potentially untrusted
or
+ * truncated) input and drives allocation of the resulting collection. To guard
+ * against unbounded memory allocation from a very large or malformed block
+ * count, the number of items in a single decoded array or map is capped. This
+ * mirrors the Java SDK's collection item limit. The default can be overridden
+ * with the AVRO_MAX_COLLECTION_ITEMS environment variable.
+ */
+
+#define AVRO_DEFAULT_MAX_COLLECTION_ITEMS ((int64_t) 2147483639) /*
Integer.MAX_VALUE - 8, matching the Java SDK */
+
+static int64_t
+avro_max_collection_items(void)
+{
+ const char *env = getenv("AVRO_MAX_COLLECTION_ITEMS");
+ if (env != NULL && *env != '\0') {
+ char *end = NULL;
+ long long value;
+ errno = 0;
+ value = strtoll(env, &end, 10);
+ /* Reject trailing garbage and out-of-range values (strtoll
returns
+ * LLONG_MAX/LLONG_MIN with errno == ERANGE on overflow). */
+ if (errno == 0 && end != NULL && *end == '\0' &&
+ value > 0 && value <= (long long) INT64_MAX) {
+ return (int64_t) value;
+ }
Review Comment:
`AVRO_MAX_COLLECTION_ITEMS` is accepted up to `INT64_MAX`, but the decode
loops cast `block_count` to `size_t`. On 32-bit platforms, allowing a
configured limit > `SIZE_MAX` can cause truncation in `(size_t) block_count`,
leading to incorrect decoding (reading fewer items than declared) or other
unexpected behavior. Consider rejecting env values above `SIZE_MAX` so
`block_count` is always safe to cast after the limit check.
##########
lang/c/tests/test_avro_4279.c:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+/*
+ * AVRO-4279: the block count of an array or map is read from the input and
+ * drives allocation of the resulting collection. A very large or malformed
+ * block count must be rejected instead of attempting an unbounded allocation.
+ * The limit is configurable via the AVRO_MAX_COLLECTION_ITEMS environment
+ * variable, which these tests set to a small value.
+ */
+
+#include <avro.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+static avro_value_iface_t *
+iface_for(const char *json)
+{
+ avro_schema_t schema = NULL;
+ if (avro_schema_from_json_length(json, strlen(json), &schema)) {
+ fprintf(stderr, "Cannot parse schema %s: %s\n", json,
avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+ avro_value_iface_t *iface = avro_generic_class_from_schema(schema);
+ avro_schema_decref(schema);
+ if (iface == NULL) {
+ fprintf(stderr, "Cannot create value interface: %s\n",
avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+ return iface;
+}
+
+/* Attempt to decode `bytes` with the given interface. Returns the rc of the
+ * read (0 on success, non-zero on error). */
+static int
+try_read(avro_value_iface_t *iface, const char *bytes, size_t len)
+{
+ avro_value_t value;
+ if (avro_generic_value_new(iface, &value) != 0) {
+ fprintf(stderr, "Cannot create value: %s\n", avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+ avro_reader_t reader = avro_reader_memory(bytes, (int64_t) len);
+ if (reader == NULL) {
+ fprintf(stderr, "Cannot create memory reader\n");
+ avro_value_decref(&value);
+ exit(EXIT_FAILURE);
+ }
+ int rc = avro_value_read(reader, &value);
+ avro_reader_free(reader);
+ avro_value_decref(&value);
+ return rc;
+}
+
+static void
+expect_rejected(avro_value_iface_t *iface, const char *bytes, size_t len,
const char *label)
+{
+ if (try_read(iface, bytes, len) == 0) {
+ fprintf(stderr, "%s: expected read to be rejected but it
succeeded\n", label);
+ exit(EXIT_FAILURE);
+ }
+}
+
+static void
+expect_accepted(avro_value_iface_t *iface, const char *bytes, size_t len,
const char *label)
+{
+ if (try_read(iface, bytes, len) != 0) {
+ fprintf(stderr, "%s: expected read to succeed but it failed:
%s\n",
+ label, avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+}
+
+int main(void)
+{
+ /* Cap collections at 10 items for the duration of the test. */
+#ifdef _WIN32
+ _putenv_s("AVRO_MAX_COLLECTION_ITEMS", "10");
+#else
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "10", 1);
+#endif
+
+ avro_value_iface_t *array_iface =
+ iface_for("{\"type\": \"array\", \"items\": \"null\"}");
+ avro_value_iface_t *map_iface =
+ iface_for("{\"type\": \"map\", \"values\": \"null\"}");
+
+ /* A single block declaring 11 items (zigzag(11) = 0x16) exceeds the
+ * configured limit of 10 and must be rejected. The null items occupy no
+ * bytes, so a tiny input could otherwise drive a large allocation. */
+ {
+ const char bytes[] = { 0x16, 0x00 };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array over
limit");
+ expect_rejected(map_iface, bytes, sizeof(bytes), "map over
limit");
+ }
+
+ /* A negative count (unsigned varint 0x15 -> -11) uses its absolute
value
+ * (11) as the item count and is followed by a block size (0x00). It
must
+ * still be bounded, for both arrays and maps. */
+ {
+ const char bytes[] = { 0x15, 0x00 };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array
negative count");
+ expect_rejected(map_iface, bytes, sizeof(bytes), "map negative
count");
+ }
+
+ /* A block count of INT64_MIN (zigzag of 2^64-1: nine 0xFF bytes then
+ * 0x01) cannot be negated without signed-overflow UB and must be
rejected
+ * outright. */
+ {
+ const char bytes[] = {
+ (char) 0xFF, (char) 0xFF, (char) 0xFF, (char) 0xFF,
+ (char) 0xFF, (char) 0xFF, (char) 0xFF, (char) 0xFF,
+ (char) 0xFF, 0x01
+ };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array
INT64_MIN count");
+ expect_rejected(map_iface, bytes, sizeof(bytes), "map INT64_MIN
count");
+ }
+
+ /* Two blocks of 6 items (zigzag(6) = 0x0c) exceed the limit
cumulatively
+ * even though neither block does on its own. */
+ {
+ const char bytes[] = { 0x0c, 0x0c };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array
cumulative");
+ }
+
+ /* Three items (zigzag(3) = 0x06) then the end-of-array marker (0x00) is
+ * within the limit and must decode successfully. */
+ {
+ const char bytes[] = { 0x06, 0x00 };
+ expect_accepted(array_iface, bytes, sizeof(bytes), "array
within limit");
+ }
Review Comment:
The within-limit success case is only asserted for arrays. Adding a small
within-limit map decode assertion here would validate that the new
per-block/cumulative limit logic doesn’t reject valid maps when under the
configured limit.
##########
lang/c/tests/test_avro_4279.c:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+/*
+ * AVRO-4279: the block count of an array or map is read from the input and
+ * drives allocation of the resulting collection. A very large or malformed
+ * block count must be rejected instead of attempting an unbounded allocation.
+ * The limit is configurable via the AVRO_MAX_COLLECTION_ITEMS environment
+ * variable, which these tests set to a small value.
+ */
+
+#include <avro.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+static avro_value_iface_t *
+iface_for(const char *json)
+{
+ avro_schema_t schema = NULL;
+ if (avro_schema_from_json_length(json, strlen(json), &schema)) {
+ fprintf(stderr, "Cannot parse schema %s: %s\n", json,
avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+ avro_value_iface_t *iface = avro_generic_class_from_schema(schema);
+ avro_schema_decref(schema);
+ if (iface == NULL) {
+ fprintf(stderr, "Cannot create value interface: %s\n",
avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+ return iface;
+}
+
+/* Attempt to decode `bytes` with the given interface. Returns the rc of the
+ * read (0 on success, non-zero on error). */
+static int
+try_read(avro_value_iface_t *iface, const char *bytes, size_t len)
+{
+ avro_value_t value;
+ if (avro_generic_value_new(iface, &value) != 0) {
+ fprintf(stderr, "Cannot create value: %s\n", avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+ avro_reader_t reader = avro_reader_memory(bytes, (int64_t) len);
+ if (reader == NULL) {
+ fprintf(stderr, "Cannot create memory reader\n");
+ avro_value_decref(&value);
+ exit(EXIT_FAILURE);
+ }
+ int rc = avro_value_read(reader, &value);
+ avro_reader_free(reader);
+ avro_value_decref(&value);
+ return rc;
+}
+
+static void
+expect_rejected(avro_value_iface_t *iface, const char *bytes, size_t len,
const char *label)
+{
+ if (try_read(iface, bytes, len) == 0) {
+ fprintf(stderr, "%s: expected read to be rejected but it
succeeded\n", label);
+ exit(EXIT_FAILURE);
+ }
+}
+
+static void
+expect_accepted(avro_value_iface_t *iface, const char *bytes, size_t len,
const char *label)
+{
+ if (try_read(iface, bytes, len) != 0) {
+ fprintf(stderr, "%s: expected read to succeed but it failed:
%s\n",
+ label, avro_strerror());
+ exit(EXIT_FAILURE);
+ }
+}
+
+int main(void)
+{
+ /* Cap collections at 10 items for the duration of the test. */
+#ifdef _WIN32
+ _putenv_s("AVRO_MAX_COLLECTION_ITEMS", "10");
+#else
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "10", 1);
+#endif
+
+ avro_value_iface_t *array_iface =
+ iface_for("{\"type\": \"array\", \"items\": \"null\"}");
+ avro_value_iface_t *map_iface =
+ iface_for("{\"type\": \"map\", \"values\": \"null\"}");
+
+ /* A single block declaring 11 items (zigzag(11) = 0x16) exceeds the
+ * configured limit of 10 and must be rejected. The null items occupy no
+ * bytes, so a tiny input could otherwise drive a large allocation. */
+ {
+ const char bytes[] = { 0x16, 0x00 };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array over
limit");
+ expect_rejected(map_iface, bytes, sizeof(bytes), "map over
limit");
+ }
+
+ /* A negative count (unsigned varint 0x15 -> -11) uses its absolute
value
+ * (11) as the item count and is followed by a block size (0x00). It
must
+ * still be bounded, for both arrays and maps. */
+ {
+ const char bytes[] = { 0x15, 0x00 };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array
negative count");
+ expect_rejected(map_iface, bytes, sizeof(bytes), "map negative
count");
+ }
+
+ /* A block count of INT64_MIN (zigzag of 2^64-1: nine 0xFF bytes then
+ * 0x01) cannot be negated without signed-overflow UB and must be
rejected
+ * outright. */
+ {
+ const char bytes[] = {
+ (char) 0xFF, (char) 0xFF, (char) 0xFF, (char) 0xFF,
+ (char) 0xFF, (char) 0xFF, (char) 0xFF, (char) 0xFF,
+ (char) 0xFF, 0x01
+ };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array
INT64_MIN count");
+ expect_rejected(map_iface, bytes, sizeof(bytes), "map INT64_MIN
count");
+ }
+
+ /* Two blocks of 6 items (zigzag(6) = 0x0c) exceed the limit
cumulatively
+ * even though neither block does on its own. */
+ {
+ const char bytes[] = { 0x0c, 0x0c };
+ expect_rejected(array_iface, bytes, sizeof(bytes), "array
cumulative");
+ }
Review Comment:
The cumulative-limit test only exercises arrays. Since `read_map_value()`
has its own block loop and now also applies the cumulative check, it would be
good to add a map equivalent here so regressions in the map path get caught
under the low `AVRO_MAX_COLLECTION_ITEMS` test configuration.
--
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]
