Copilot commented on code in PR #3853:
URL: https://github.com/apache/avro/pull/3853#discussion_r3567492247
##########
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:
`avro_max_decompress_length()` ignores `AVRO_MAX_DECOMPRESS_LENGTH` values
that overflow `strtoll()` (errno == ERANGE), silently falling back to the 200
MiB default. Since the intention is to allow large configured limits (with
clamping), it would be better to treat ERANGE as “too large” and clamp to
`INT64_MAX`/`SIZE_MAX` instead of discarding the override.
##########
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:
`decode_deflate()` clamps `max_len` because zlib’s `avail_out` is a `uInt`,
but it still assigns the compressed input length (`int64_t len`) directly into
zlib’s `avail_in` (`uInt`). If `len` exceeds `UINT_MAX`, it will truncate and
can lead to incorrect/incomplete decompression. Add an explicit `len` range
check and cast when assigning `avail_in` (and `avail_out` for consistency).
--
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]
