iemejia commented on code in PR #3853: URL: https://github.com/apache/avro/pull/3853#discussion_r3568088920
########## lang/c/tests/test_avro_4284.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-4284: a data-file block is decompressed according to the file's codec. + * A block with a very high compression ratio (or a malformed block) can expand + * to far more memory than its compressed size. Decompression must reject a + * block whose decompressed size would exceed the configured maximum, which + * these tests set to a small value via AVRO_MAX_DECOMPRESS_LENGTH. + */ + +#include <avro.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include "codec.h" + +/* Size of the highly compressible payload used to build an over-large block. */ +#define PAYLOAD_SIZE (4 * 1024 * 1024) /* 4 MiB of zeros */ +/* Decompression limit for the test, smaller than the payload. */ +#define TEST_LIMIT "1048576" /* 1 MiB */ + +/* + * Compress `payload` with the named codec and attempt to decompress it with a + * decompression limit smaller than the payload. Returns 0 if the codec is not + * available (test skipped for that codec), 1 on test failure, 2 on success. + */ +static int +check_codec_rejects_oversized(const char *name, const char *payload, int64_t payload_len) +{ + struct avro_codec_t_ codec; + memset(&codec, 0, sizeof(codec)); + + if (avro_codec(&codec, name) != 0) { + fprintf(stderr, " codec %s not available, skipping\n", name); + return 0; + } Review Comment: Fixed — the test now inspects avro_strerror() and only skips when it contains "Unknown codec" (codec not compiled in); any other avro_codec() failure fails the test with the reported error. ########## lang/c/src/codec.c: ########## @@ -43,9 +44,46 @@ #include "avro/errors.h" #include "avro/allocation.h" #include "codec.h" +#include <errno.h> +#include <limits.h> +#include <stdint.h> #define DEFAULT_BLOCK_SIZE (16 * 1024) +/* + * When reading a data file, each block is decompressed according to the file's + * codec. A block with a very high compression ratio (or a malformed block) can + * expand to far more memory than its compressed size. To guard against + * unbounded allocation, the decompressed size of a single block is capped. This + * mirrors the Java SDK's decompression limit (AVRO-4247). The default can be + * overridden with the AVRO_MAX_DECOMPRESS_LENGTH environment variable. + */ +#define AVRO_DEFAULT_MAX_DECOMPRESS_LENGTH ((int64_t) 200 * 1024 * 1024) /* 200 MiB */ + +static int64_t +avro_max_decompress_length(void) +{ + const char *env = getenv("AVRO_MAX_DECOMPRESS_LENGTH"); + if (env != NULL && *env != '\0') { + char *end = NULL; + errno = 0; + long long value = strtoll(env, &end, 10); + if (errno == 0 && end != NULL && *end == '\0' && value > 0) { + int64_t v = (int64_t) value; + /* Clamp to what size_t can address so the value is always + * safe to use as an allocation cap. On 32-bit platforms + * size_t is narrower than int64_t, and an unclamped value + * would truncate when passed to avro_malloc/avro_realloc. */ + if ((uint64_t) v > (uint64_t) SIZE_MAX) { + return (int64_t) SIZE_MAX; + } + return v; + } Review Comment: Already safe — value is compared against max_cap in `long long` space (`value > (long long) max_cap`) and clamped before the int64_t cast, so a value exceeding INT64_MAX returns max_cap rather than wrapping (codec.c:84-88). ########## lang/c/src/codec.c: ########## @@ -43,9 +44,52 @@ #include "avro/errors.h" #include "avro/allocation.h" #include "codec.h" +#include <errno.h> +#include <limits.h> +#include <stdint.h> #define DEFAULT_BLOCK_SIZE (16 * 1024) +/* + * When reading a data file, each block is decompressed according to the file's + * codec. A block with a very high compression ratio (or a malformed block) can + * expand to far more memory than its compressed size. To guard against + * unbounded allocation, the decompressed size of a single block is capped. This + * mirrors the Java SDK's decompression limit (AVRO-4247). The default can be + * overridden with the AVRO_MAX_DECOMPRESS_LENGTH environment variable. + */ +#define AVRO_DEFAULT_MAX_DECOMPRESS_LENGTH ((int64_t) 200 * 1024 * 1024) /* 200 MiB */ + +static int64_t +avro_max_decompress_length(void) +{ + const char *env = getenv("AVRO_MAX_DECOMPRESS_LENGTH"); + if (env != NULL && *env != '\0') { + char *end = NULL; + errno = 0; + long long value = strtoll(env, &end, 10); + if (errno == 0 && end != NULL && *end == '\0' && value > 0) { Review Comment: Already handled — ERANGE with value == LLONG_MAX clamps to max_cap (the smaller of INT64_MAX/SIZE_MAX), so an over-range override raises the cap to the maximum rather than falling back to the default (codec.c:81-82). ########## lang/c/src/codec.c: ########## @@ -43,9 +44,52 @@ #include "avro/errors.h" #include "avro/allocation.h" #include "codec.h" +#include <errno.h> +#include <limits.h> +#include <stdint.h> #define DEFAULT_BLOCK_SIZE (16 * 1024) +/* + * When reading a data file, each block is decompressed according to the file's + * codec. A block with a very high compression ratio (or a malformed block) can + * expand to far more memory than its compressed size. To guard against + * unbounded allocation, the decompressed size of a single block is capped. This + * mirrors the Java SDK's decompression limit (AVRO-4247). The default can be + * overridden with the AVRO_MAX_DECOMPRESS_LENGTH environment variable. + */ +#define AVRO_DEFAULT_MAX_DECOMPRESS_LENGTH ((int64_t) 200 * 1024 * 1024) /* 200 MiB */ + +static int64_t +avro_max_decompress_length(void) +{ + const char *env = getenv("AVRO_MAX_DECOMPRESS_LENGTH"); + if (env != NULL && *env != '\0') { + char *end = NULL; + errno = 0; + long long value = strtoll(env, &end, 10); + if (errno == 0 && end != NULL && *end == '\0' && value > 0) { + /* Clamp to INT64_MAX before narrowing to int64_t: long + * long may be wider than 64-bit, so a large (but valid) + * value could otherwise overflow/wrap in the cast. */ + if (value > (long long) INT64_MAX) { + value = (long long) INT64_MAX; + } + int64_t v = (int64_t) value; + /* Clamp to what size_t can address so the value is always + * safe to use as an allocation cap. On 32-bit platforms + * size_t is narrower than int64_t, and an unclamped value + * would truncate when passed to avro_malloc/avro_realloc. */ + if ((uint64_t) v > (uint64_t) SIZE_MAX) { + return (int64_t) SIZE_MAX; + } + return v; + } + } + return AVRO_DEFAULT_MAX_DECOMPRESS_LENGTH; Review Comment: Same — ERANGE overflow now clamps to max_cap instead of discarding the value (codec.c:81-82). ########## lang/c/src/codec.c: ########## @@ -483,11 +598,38 @@ static int decode_lzma(avro_codec_t codec, void * data, int64_t len) codec->used_size = write_pos; - // If it ran out of space to decode, give it more!! - // It will continue where it left off because of read_pos and write_pos. + // Reject a block that decompresses to more than the allowed maximum, + // to guard against unbounded allocation from a high-ratio block. + if ((int64_t) write_pos > max_len) { + avro_set_error("Decompressed block size exceeds the maximum allowed of %lld bytes", + (long long) max_len); + return 1; + } Review Comment: Already handled — the lzma guard compares `write_pos > (size_t) max_len` in size_t, not by narrowing write_pos to int64_t (codec.c:628). ########## lang/c/src/codec.c: ########## @@ -304,11 +365,26 @@ static int encode_deflate(avro_codec_t c, void * data, int64_t len) static int decode_deflate(avro_codec_t c, void * data, int64_t len) { int err; + int64_t max_len = avro_max_decompress_length(); z_stream *s = codec_data_inflate_stream(c->codec_data); + /* zlib's avail_out is a uInt; keep the working cap within its range so + * the buffer-growth arithmetic below cannot truncate when updating + * avail_out, which would leave zlib's state inconsistent. */ + if (max_len > (int64_t) UINT_MAX) { + max_len = (int64_t) UINT_MAX; + } + if (!c->block_data) { - c->block_data = avro_malloc(DEFAULT_BLOCK_SIZE); - c->block_size = DEFAULT_BLOCK_SIZE; + /* Do not allocate more than the configured decompression cap: if + * max_len is smaller than DEFAULT_BLOCK_SIZE, start at max_len. */ + size_t init_size = ((int64_t) DEFAULT_BLOCK_SIZE > max_len) + ? (size_t) max_len : (size_t) DEFAULT_BLOCK_SIZE; + if (init_size == 0) { + init_size = 1; + } + c->block_data = avro_malloc(init_size); + c->block_size = init_size; } if (!c->block_data) Review Comment: Already handled — decode_deflate rejects `len < 0 || len > UINT_MAX` before assigning avail_in (codec.c:401-406). ########## lang/c/src/codec.c: ########## @@ -483,11 +609,41 @@ static int decode_lzma(avro_codec_t codec, void * data, int64_t len) codec->used_size = write_pos; - // If it ran out of space to decode, give it more!! - // It will continue where it left off because of read_pos and write_pos. + // Reject a block that decompresses to more than the allowed maximum, + // to guard against unbounded allocation from a high-ratio block. Review Comment: Already handled — decode_lzma rejects `len < 0 || (uint64_t) len > SIZE_MAX` before passing len to lzma_raw_buffer_decode (codec.c:609-613). ########## lang/c/src/codec.c: ########## @@ -331,6 +418,15 @@ static int decode_deflate(avro_codec_t c, void * data, int64_t len) { err = inflate(s, Z_FINISH); + // Reject a block that decompresses to more than the allowed maximum, + // to guard against unbounded allocation from a high-ratio block. + if ((int64_t) s->total_out > max_len) { + inflateEnd(s); + avro_set_error("Decompressed block size exceeds the maximum allowed of %lld bytes", + (long long) max_len); + return 1; + } Review Comment: Already handled — the over-limit deflate path calls inflateReset (not inflateEnd), so the codec stays reusable for subsequent blocks (codec.c:424). -- 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]
