Copilot commented on code in PR #3853:
URL: https://github.com/apache/avro/pull/3853#discussion_r3564679447
##########
lang/c/src/codec.c:
##########
@@ -46,6 +47,31 @@
#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;
+ long long value = strtoll(env, &end, 10);
+ if (end != NULL && *end == '\0' && value > 0) {
+ return (int64_t) value;
+ }
+ }
+ return AVRO_DEFAULT_MAX_DECOMPRESS_LENGTH;
Review Comment:
`AVRO_MAX_DECOMPRESS_LENGTH` is parsed as an `int64_t` and returned without
clamping to the maximum representable allocation size. On 32-bit platforms, a
value larger than `size_t` can later be truncated when passed to
`avro_malloc/avro_realloc` (which take `size_t`), potentially causing
under-allocation and unsafe writes. Clamp the parsed value to `(size_t)-1` (and
keep it > 0) so it is always safe to use as an allocation cap.
##########
lang/c/src/codec.c:
##########
@@ -304,6 +347,7 @@ 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);
Review Comment:
In `decode_deflate`, `max_len` can be overridden via an environment
variable, but zlib’s `avail_out`/`avail_in` are `uInt`. If `max_len` is set
above the `uInt` range, the resize path can truncate the added capacity when
casting to `uInt`, which can lead to `avail_out` failing to grow (e.g.,
truncating to 0) and repeated `Z_BUF_ERROR` loops. Clamp `max_len` to
`(uInt)-1` for the deflate codec to keep zlib state consistent.
##########
lang/c/src/codec.c:
##########
@@ -339,13 +392,30 @@ static int decode_deflate(avro_codec_t c, void * data,
int64_t len)
return 1;
}
- // The buffer was not big enough. resize it.
+ // The buffer was not big enough. resize it, without growing
beyond
+ // the configured maximum decompressed size.
if (err == Z_BUF_ERROR)
{
- c->block_data = avro_realloc(c->block_data,
c->block_size, c->block_size * 2);
- s->next_out = c->block_data + s->total_out;
- s->avail_out += c->block_size;
- c->block_size = c->block_size * 2;
+ int64_t new_size = c->block_size * 2;
+ if (new_size > max_len) {
+ new_size = max_len;
+ }
Review Comment:
`int64_t new_size = c->block_size * 2;` can overflow (undefined behavior) if
`AVRO_MAX_DECOMPRESS_LENGTH` is configured very large and the buffer has
already grown near `INT64_MAX/2`. Compute the doubled size using a
checked/clamped pattern based on `max_len` to avoid signed overflow.
##########
lang/c/src/codec.c:
##########
@@ -483,11 +554,33 @@ 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;
+ }
+
+ // If it ran out of space to decode, give it more (without
growing
+ // beyond the configured maximum decompressed size).
if (ret == LZMA_BUF_ERROR) {
- codec->block_data = avro_realloc(codec->block_data,
codec->block_size, codec->block_size * 2);
- codec->block_size = codec->block_size * 2;
+ int64_t new_size = codec->block_size * 2;
+ if (new_size > max_len) {
+ new_size = max_len;
+ }
Review Comment:
In `decode_lzma`, `int64_t new_size = codec->block_size * 2;` can overflow
(undefined behavior) when the max-decompress cap is configured very large and
the buffer has already grown near `INT64_MAX/2`. Use a checked/clamped growth
calculation based on `max_len` to avoid signed overflow.
--
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]
