Skip to content

AVRO-4284: [c] Enforce a maximum decompressed block size#3853

Open
iemejia wants to merge 13 commits into
apache:mainfrom
iemejia:AVRO-4284-c-decompress-limit
Open

AVRO-4284: [c] Enforce a maximum decompressed block size#3853
iemejia wants to merge 13 commits into
apache:mainfrom
iemejia:AVRO-4284-c-decompress-limit

Conversation

@iemejia

@iemejia iemejia commented Jul 11, 2026

Copy link
Copy Markdown
Member

What is the purpose of the change

The deflate and lzma codecs cap buffer growth and reject once the output would exceed the limit; snappy rejects an over-large declared length up front. Exceeding the limit fails decoding with a descriptive error.

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) could expand to far more memory than its compressed size (a decompression bomb). This enforces a configurable maximum decompressed size while reading each block, mirroring the Java SDK's decompression limit (AVRO-4247). The limit defaults to 200 MiB and can be overridden with the AVRO_MAX_DECOMPRESS_LENGTH environment variable.

This is part of the umbrella issue AVRO-4283.

Verifying this change

This change added tests and can be verified as follows:

  • Added tests/test_avro_4284.c (registered in tests/CMakeLists.txt) exercising deflate, snappy and lzma over-limit rejection.
  • Run: ctest -R test_avro_4284 (also passes under the valgrind memcheck variant)

Documentation

  • Does this pull request introduce a new feature? (no — hardening/robustness)
  • If yes, how is the feature documented? (not applicable; the new AVRO_MAX_DECOMPRESS_LENGTH environment variable is documented in code comments)

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) could expand to far more memory than its compressed size. Enforce a
configurable maximum decompressed size across the deflate, snappy and lzma
codecs, mirroring the Java SDK's decompression limit (AVRO-4247): snappy
rejects an over-large declared length up front, while deflate and lzma cap
buffer growth and reject once the output would exceed the limit. The limit
defaults to 200 MiB and can be overridden with the AVRO_MAX_DECOMPRESS_LENGTH
environment variable.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the C datafile block decompression path against decompression bombs by enforcing a configurable maximum decompressed block size (default 200 MiB, overridable via AVRO_MAX_DECOMPRESS_LENGTH) across the deflate, snappy, and lzma codecs, and adds a regression test covering over-limit rejection.

Changes:

  • Add AVRO_MAX_DECOMPRESS_LENGTH support and enforce the maximum decompressed block size in lang/c/src/codec.c for snappy/deflate/lzma decoders.
  • Add a new C test test_avro_4284.c to validate over-limit rejection behavior.
  • Register the new test in the C test CMake configuration.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
lang/c/src/codec.c Adds env-configured max decompression length and enforces it during snappy/deflate/lzma decoding.
lang/c/tests/test_avro_4284.c New test that sets a small max length and asserts codecs reject an oversized decompressed block.
lang/c/tests/CMakeLists.txt Registers the new test target.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lang/c/src/codec.c Outdated
Comment on lines 171 to 174
@@ -147,6 +173,12 @@ static int decode_snappy(avro_codec_t c, void * data, int64_t len)
return 1;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aeb326 — decode_snappy rejects a block shorter than the 4-byte CRC suffix before computing len - 4, and the decompression limit is cached in a local instead of being re-parsed in the error path.

Comment thread lang/c/src/codec.c Outdated
Comment on lines +398 to +401
c->block_data = avro_realloc(c->block_data, c->block_size, new_size);
s->next_out = c->block_data + s->total_out;
s->avail_out += c->block_size;
c->block_size = c->block_size * 2;
s->avail_out += (uInt)(new_size - c->block_size);
c->block_size = new_size;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aeb326 — decode_deflate reallocs into a temporary pointer and fails cleanly (freeing nothing it cannot) on OOM instead of null-dereferencing and losing the original buffer.

Comment thread lang/c/src/codec.c Outdated
Comment on lines +560 to +561
codec->block_data = avro_realloc(codec->block_data, codec->block_size, new_size);
codec->block_size = new_size;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aeb326 — decode_lzma applies the same temporary-pointer + explicit OOM error pattern.

Comment thread lang/c/tests/test_avro_4284.c Outdated
Comment on lines +61 to +67
int64_t compressed_len = codec.used_size;
char *compressed = (char *) malloc(compressed_len);
if (compressed == NULL) {
avro_codec_reset(&codec);
return 1;
}
memcpy(compressed, codec.block_data, compressed_len);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aeb326 — the test casts used_size to size_t for malloc/memcpy and guards against a non-positive length.

- decode_snappy rejects a block shorter than the 4-byte CRC suffix before
  computing len - 4 (which would underflow), and caches the decompression
  limit instead of re-parsing the environment in the error path.
- decode_deflate and decode_lzma realloc into a temporary pointer and fail
  cleanly on OOM instead of null-dereferencing and leaking the old buffer.
- The test casts the codec's used_size to size_t for malloc/memcpy and guards
  against a non-positive length.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread lang/c/src/codec.c Outdated
Comment on lines +185 to +189
if ((int64_t) outlen > max_len) {
avro_set_error("Decompressed block size %llu exceeds the maximum allowed of %lld bytes",
(unsigned long long) outlen, (long long) max_len);
return 1;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The comparison now casts both operands to uint64_t ((uint64_t) outlen > (uint64_t) max_len), so a declared length above INT64_MAX no longer wraps to a negative int64_t and slip past the cap. Pushed in 65d4dcf.

Comment on lines +73 to +85
memcpy(compressed, codec.block_data, (size_t) compressed_len);

int rc = avro_codec_decode(&codec, compressed, compressed_len);

free(compressed);
avro_codec_reset(&codec);

if (rc == 0) {
fprintf(stderr, " codec %s: expected decompression to be rejected but it succeeded\n", name);
return 1;
}
fprintf(stderr, " codec %s: over-limit block rejected as expected\n", name);
return 2;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test now captures avro_strerror() after the failed decode and asserts the message contains "exceeds the maximum", so it confirms the rejection is specifically the size-limit error and not an unrelated failure. Pushed in 65d4dcf.

- decode_snappy compared (int64_t) outlen > max_len. A declared length above
  INT64_MAX would wrap to a negative int64_t and slip past the cap; compare as
  unsigned 64-bit instead.
- The test now captures avro_strerror() and asserts the rejection is
  specifically the 'exceeds the maximum' size-limit error, not an unrelated
  failure.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comment thread lang/c/src/codec.c
Comment on lines +63 to +71
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;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. avro_max_decompress_length() now clamps the parsed value to SIZE_MAX (and checks errno), so it is always safe as an allocation cap and cannot truncate when passed to avro_malloc/avro_realloc on 32-bit. Pushed in d280eaf.

Comment thread lang/c/src/codec.c
Comment on lines 349 to 352
int err;
int64_t max_len = avro_max_decompress_length();
z_stream *s = codec_data_inflate_stream(c->codec_data);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. decode_deflate caps its working max_len to UINT_MAX, so the avail_out update (a uInt) can no longer truncate and leave zlib's state inconsistent. Pushed in d280eaf.

Comment thread lang/c/src/codec.c Outdated
Comment on lines +399 to +402
int64_t new_size = c->block_size * 2;
if (new_size > max_len) {
new_size = max_len;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The deflate growth now doubles with a checked pattern (compare block_size against max_len/2 before multiplying), avoiding signed overflow. The UINT_MAX cap above also bounds block_size well below the overflow range. Pushed in d280eaf.

Comment thread lang/c/src/codec.c Outdated
Comment on lines +568 to +571
int64_t new_size = codec->block_size * 2;
if (new_size > max_len) {
new_size = max_len;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. decode_lzma uses the same checked doubling (compare block_size against max_len/2 before multiplying) to avoid signed overflow. Pushed in d280eaf.

- avro_max_decompress_length() clamps the parsed value to SIZE_MAX (and now
  checks errno), so a configured limit larger than size_t cannot truncate when
  passed to avro_malloc/avro_realloc on 32-bit platforms.
- decode_deflate caps its working limit to UINT_MAX so the avail_out update
  (a uInt) cannot truncate and leave zlib's state inconsistent.
- decode_deflate and decode_lzma now double the output buffer with a checked
  pattern (compare against max_len/2 before multiplying) to avoid signed
  overflow when the buffer has already grown near INT64_MAX/2.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comment on lines +103 to +107
#ifdef _WIN32
_putenv_s("AVRO_MAX_DECOMPRESS_LENGTH", TEST_LIMIT);
#else
setenv("AVRO_MAX_DECOMPRESS_LENGTH", TEST_LIMIT, 1);
#endif

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the test now checks the setenv/_putenv_s return value and exits with failure if the env var can't be set.

Comment thread lang/c/src/codec.c Outdated
Comment on lines 372 to 374
if (!c->block_data) {
c->block_data = avro_malloc(DEFAULT_BLOCK_SIZE);
c->block_size = DEFAULT_BLOCK_SIZE;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — decode_zlib now allocates the initial buffer as min(DEFAULT_BLOCK_SIZE, max_len) so a small AVRO_MAX_DECOMPRESS_LENGTH is respected up front.

Comment thread lang/c/src/codec.c
Comment on lines +559 to 562
int64_t max_len = avro_max_decompress_length();
lzma_ret ret;
lzma_filter* filters = codec_data_lzma_filters(codec->codec_data);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — decode_lzma likewise allocates min(DEFAULT_BLOCK_SIZE, max_len) for the initial buffer.

…check setenv

Address review feedback:
- decode_zlib/decode_lzma allocated the initial output buffer at
  DEFAULT_BLOCK_SIZE regardless of the configured cap; if
  AVRO_MAX_DECOMPRESS_LENGTH is set below DEFAULT_BLOCK_SIZE the initial
  allocation exceeded the limit. Allocate min(DEFAULT_BLOCK_SIZE, max_len).
- test_avro_4284: check the setenv/_putenv_s return value and fail early if the
  environment variable could not be set, so the test can't silently run with the
  default limit.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread lang/c/src/codec.c
Comment on lines +70 to +81
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;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the long long value from strtoll is now clamped to INT64_MAX before the cast to int64_t, so on a platform where long long is wider than 64-bit a large value can't overflow/wrap the cast before the SIZE_MAX clamp.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

…arrowing

avro_max_decompress_length cast the strtoll (long long) result to int64_t before
range-validating it. long long may be wider than 64-bit, so a large value could
overflow/wrap in the cast (potentially becoming negative before the SIZE_MAX
clamp). Clamp the long long value to INT64_MAX before narrowing to int64_t.

Assisted-by: GitHub Copilot:claude-opus-4.8
Previously an out-of-range value (strtoll ERANGE) fell back to the 200 MiB
default, so setting the env var just above LLONG_MAX surprisingly *reduced* the
effective limit. Treat positive overflow (ERANGE with LLONG_MAX) as a request
for the maximum and clamp to the largest usable cap (min of INT64_MAX and
SIZE_MAX). Also simplified the in-range clamp to the same max_cap.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

decode_deflate assigned the int64_t compressed block length directly into zlib's
uInt avail_in, which would truncate for a length above UINT_MAX and cause
incorrect/incomplete decompression. Reject a negative or > UINT_MAX length and
cast explicitly.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread lang/c/src/codec.c
Comment on lines 609 to +613

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — decode_lzma now rejects a compressed len that is negative or > SIZE_MAX before passing it to lzma_raw_buffer_decode's size_t input-size parameter.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already handled — decode_lzma rejects len < 0 || (uint64_t) len > SIZE_MAX before passing len to lzma_raw_buffer_decode (codec.c:609-613).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

decode_lzma passed the int64_t compressed length directly to
lzma_raw_buffer_decode's size_t input-size parameter; a negative value converts
to a huge size_t and a value above SIZE_MAX truncates. Reject a negative or
> SIZE_MAX length before decoding.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread lang/c/src/codec.c
Comment on lines +423 to +428
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;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the over-limit rejection paths now call inflateReset(s) instead of inflateEnd(s), so the codec's zlib stream stays allocated and reusable for subsequent avro_codec_decode() calls (we're only refusing the oversized output, the stream itself is fine).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already handled — the over-limit deflate path calls inflateReset (not inflateEnd), so the codec stays reusable for subsequent blocks (codec.c:424).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

lang/c/src/codec.c:414

  • decode_deflate() sets zlib's avail_out from c->block_size without bounding it by max_len. If the codec already has a larger output buffer (e.g., after a previous larger block or if the env var is lowered at runtime), zlib can write more than the configured cap before the post-inflate total_out check runs, undermining the decompression-limit guarantee. Also, assigning int64_t block_size to uInt avail_out relies on implicit truncation. Set avail_out to min(block_size, max_len) and cast explicitly to uInt.
	s->next_in = data;
	s->avail_in = (uInt) len;

	s->next_out = c->block_data;
	s->avail_out = c->block_size;

lang/c/src/codec.c:617

  • decode_lzma() passes codec->block_size as the output buffer size to liblzma without bounding it by max_len. If the codec has a pre-existing buffer larger than the configured cap (e.g., after an earlier larger block or if AVRO_MAX_DECOMPRESS_LENGTH is reduced at runtime), liblzma can decode more than max_len into memory before the write_pos check triggers. Cap the out_size passed to lzma_raw_buffer_decode() to max_len (and cast len once to size_t after the range check).
	do
	{
		ret = lzma_raw_buffer_decode(filters, NULL, data,
			&read_pos, len, codec->block_data, &write_pos,
			codec->block_size);

Comment on lines +49 to +52
if (avro_codec(&codec, name) != 0) {
fprintf(stderr, " codec %s not available, skipping\n", name);
return 0;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

iemejia added 2 commits July 13, 2026 03:58
The over-limit rejection paths called inflateEnd(s), permanently tearing down
the codec's shared zlib inflate stream, so a subsequent avro_codec_decode() with
the same codec would fail. Use inflateReset(s) instead so the codec stays
reusable after an over-large block is rejected (the stream itself is healthy;
we're only refusing the output).

Assisted-by: GitHub Copilot:claude-opus-4.8
…d in

check_codec_rejects_oversized() skipped on any avro_codec() failure, which could
hide a real regression (a compiled-in codec whose initialization fails). Skip
only when the error is "Unknown codec ..." (the not-compiled-in case); any other
initialization failure now fails the test with the reported error.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread lang/c/src/codec.c
Comment on lines +193 to +197
/* The block carries a trailing 4-byte CRC; a length below that is
* malformed and would underflow len - 4. */
if (len < 4) {
avro_set_error("Snappy block too small (%lld bytes), expected at least 4",
(long long) len);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — decode_snappy now rejects len - 4 > SIZE_MAX and uses a single size_t inlen = (size_t)(len - 4) for both snappy_uncompressed_length/snappy_uncompress and the trailing-CRC offset, so no truncation occurs on narrow-size_t platforms.

…nversion

decode_snappy passed `len - 4` (int64_t) to snappy_uncompressed_length /
snappy_uncompress (which take size_t) and used it in pointer arithmetic. On
platforms where size_t is narrower than int64_t, a large len-4 would truncate
and could cause out-of-bounds reads. Reject a len-4 that exceeds SIZE_MAX and
use a single `size_t inlen` for the payload length and CRC offset.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants