Copilot commented on code in PR #3848: URL: https://github.com/apache/avro/pull/3848#discussion_r3564296872
########## lang/c/tests/test_avro_4279.c: ########## @@ -0,0 +1,143 @@ +/* + * 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; + avro_generic_value_new(iface, &value); + avro_reader_t reader = avro_reader_memory(bytes, (int64_t) len); + int rc = avro_value_read(reader, &value); + avro_reader_free(reader); + avro_value_decref(&value); + return rc; +} Review Comment: `avro_generic_value_new()` and `avro_reader_memory()` can fail and return non-zero/NULL. `try_read()` currently ignores these failures, which can lead to dereferencing an uninitialized value or a NULL reader (and a test crash) instead of returning a clean error code. ########## lang/c/src/value-read.c: ########## @@ -16,6 +16,7 @@ */ #include <avro/platform.h> +#include <stdint.h> #include <stdlib.h> #include <string.h> Review Comment: `avro_max_collection_items()` uses `errno` semantics of `strtoll`, but `<errno.h>` isn't included. Adding an `errno` check (see other comment) requires including this header to avoid build failures on some platforms. ########## lang/c/src/value-read.c: ########## @@ -38,6 +39,50 @@ 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 = strtoll(env, &end, 10); + if (end != NULL && *end == '\0' && value > 0) { + return (int64_t) value; + } + } + return AVRO_DEFAULT_MAX_COLLECTION_ITEMS; +} Review Comment: `strtoll()` overflow/underflow isn't checked. If `AVRO_MAX_COLLECTION_ITEMS` is set to an out-of-range value, `strtoll` can return `LLONG_MAX`/`LLONG_MIN` with `errno=ERANGE`, unintentionally producing an extreme limit. Also, on 32-bit platforms `size_t` is smaller than `int64_t`; allowing a limit larger than `SIZE_MAX` can lead to truncation when casting `block_count` to `size_t` in the decode loops. Validate `errno==0` and ensure the parsed value fits within both `INT64_MAX` and `SIZE_MAX` before accepting it. ########## lang/c/tests/test_avro_4279.c: ########## @@ -0,0 +1,143 @@ +/* + * 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; + avro_generic_value_new(iface, &value); + avro_reader_t reader = avro_reader_memory(bytes, (int64_t) len); + 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. */ + { + const char bytes[] = { 0x15, 0x00 }; + expect_rejected(array_iface, bytes, sizeof(bytes), "array negative count"); + } Review Comment: The "negative count" case is only asserted for arrays. Since `read_map_value()` has separate negative-block handling (reads `block_size` and then applies the cap), it's worth asserting the same rejection for maps too to avoid regressions specific to the map path. -- 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]
