Skip to content

AVRO-4285: [c++] Enforce a maximum decompressed block size#3854

Open
iemejia wants to merge 10 commits into
apache:mainfrom
iemejia:AVRO-4285-cpp-decompress-limit
Open

AVRO-4285: [c++] Enforce a maximum decompressed block size#3854
iemejia wants to merge 10 commits into
apache:mainfrom
iemejia:AVRO-4285-cpp-decompress-limit

Conversation

@iemejia

@iemejia iemejia commented Jul 11, 2026

Copy link
Copy Markdown
Member

What is the purpose of the change

DataFileReaderBase::readDataBlock bounds decompression across codecs: snappy and zstandard reject an over-large declared/frame size up front, while deflate and the zstd streaming path cap buffer growth and reject once the output would exceed the limit. Exceeding the limit throws an avro::Exception.

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 test/DecompressionLimitTests.cc (registered via unittest(DecompressionLimitTests)) covering deflate/snappy/zstandard over-limit rejection and within-limit decoding.
  • Run: ctest -R DecompressionLimitTests

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, DataFileReaderBase::readDataBlock decompresses
each block 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 zstandard codecs, mirroring the Java SDK's
decompression limit (AVRO-4247): snappy and zstd reject an over-large
declared/frame size up front, while deflate and the zstd streaming path 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
@github-actions github-actions Bot added the C++ Pull Requests for C++ binding label Jul 11, 2026
@iemejia iemejia requested a review from Copilot July 11, 2026 14:47

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 Avro C++ data-file reader against decompression bombs by enforcing a configurable maximum decompressed block size (default 200 MiB, overridable via AVRO_MAX_DECOMPRESS_LENGTH) across supported codecs.

Changes:

  • Add AVRO_MAX_DECOMPRESS_LENGTH handling and enforce maximum decompressed size during Snappy/Zstandard/Deflate block decoding.
  • Update Zstandard decompression API to take an explicit maxLength and reject oversized frames.
  • Add a new Boost unit test suite to validate over-limit rejection and within-limit decoding, and register it in CMake.

Reviewed changes

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

Show a summary per file
File Description
lang/c++/impl/DataFile.cc Adds env-configured decompression limit and enforces it in Snappy/Zstd/Deflate decode paths.
lang/c++/impl/ZstdDecompressWrapper.hh Changes Zstd wrapper API to require a maxLength for bounded decompression.
lang/c++/impl/ZstdDecompressWrapper.cc Enforces max decompressed size for both known-size and streaming Zstd frames.
lang/c++/test/DecompressionLimitTests.cc Adds tests for over-limit rejection and within-limit reads across codecs.
lang/c++/CMakeLists.txt Registers the new DecompressionLimitTests test executable.

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

Comment thread lang/c++/impl/DataFile.cc
Comment on lines 19 to 23
#include "DataFile.hh"
#include "Compiler.hh"
#include <cstdlib>
#include <cstring>
#include "Exception.hh"

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 66809e0 — added and to DataFile.cc.

Comment thread lang/c++/impl/DataFile.cc
Comment on lines +66 to +77
size_t maxDecompressLength() {
const char *env = std::getenv("AVRO_MAX_DECOMPRESS_LENGTH");
if (env != nullptr && *env != '\0') {
errno = 0;
char *end = nullptr;
unsigned long long value = std::strtoull(env, &end, 10);
if (errno == 0 && end != nullptr && *end == '\0' && value > 0) {
return static_cast<size_t>(value);
}
}
return defaultMaxDecompressLength;
}

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 66809e0 — maxDecompressLength rejects a leading sign and clamps to std::numeric_limits<size_t>::max(), so a huge configured value cannot truncate on 32-bit builds.

Comment thread lang/c++/impl/ZstdDecompressWrapper.cc Outdated
Comment on lines +46 to +51
uncompressed.append(tmp.data(), out.pos);
// Reject a block that decompresses to more than the allowed maximum.
if (uncompressed.size() > maxLength) {
throw Exception(
"Decompressed block size exceeds the maximum allowed of {} bytes", maxLength);
}

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 66809e0 — the streaming zstd path now checks the limit before appending (out.pos > maxLength - uncompressed.size()), so the buffer never grows past the limit.

Comment on lines +65 to +73
try {
DataFileWriter<std::string> writer(path.c_str(), schema, 64 * 1024 * 1024, codec);
writer.write(big);
writer.close();
} catch (const Exception &) {
// Codec not available in this build; nothing to exercise.
std::filesystem::remove(path);
return;
}

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 66809e0 — the test no longer swallows writer exceptions (real failures surface); snappy/zstd cases are gated on the SNAPPY_CODEC_AVAILABLE / ZSTD_CODEC_AVAILABLE macros instead.

…ests

- Include <cerrno> and <limits> in DataFile.cc (used by maxDecompressLength).
- maxDecompressLength rejects a leading sign and clamps to size_t's max so a
  huge configured value cannot truncate on 32-bit builds.
- The streaming (unknown content size) Zstandard path checks the limit before
  appending, so the buffer never grows past the configured maximum.
- The decompression-limit test no longer swallows writer exceptions (real
  failures now surface); snappy/zstd cases are gated on the codec-availability
  macros instead.

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 5 out of 5 changed files in this pull request and generated 4 comments.

Comment thread lang/c++/impl/DataFile.cc
Comment on lines +68 to +85
size_t maxDecompressLength() {
const char *env = std::getenv("AVRO_MAX_DECOMPRESS_LENGTH");
if (env != nullptr && *env != '\0') {
errno = 0;
char *end = nullptr;
unsigned long long value = std::strtoull(env, &end, 10);
// Reject a leading sign (strtoull would otherwise wrap it) and clamp to
// what size_t can represent so a huge value does not truncate on 32-bit
// (or smaller size_t) builds.
if (errno == 0 && end != nullptr && *end == '\0' && value > 0 && env[0] != '-') {
if (value > std::numeric_limits<size_t>::max()) {
return std::numeric_limits<size_t>::max();
}
return static_cast<size_t>(value);
}
}
return defaultMaxDecompressLength;
}

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. maxDecompressLength() now skips leading whitespace before rejecting an explicit sign, so a value like " -5" is caught (strtoull accepts leading whitespace and would otherwise wrap the negative). Pushed in 710bbe2.

Comment thread lang/c++/impl/DataFile.cc
Comment on lines +600 to +611
{
// Reject an over-large block before allocating for it, based on the
// uncompressed length declared in the Snappy block header.
size_t declared = 0;
if (snappy::GetUncompressedLength(reinterpret_cast<const char *>(compressed_.data()),
len - 4, &declared) &&
declared > maxDecompressLength()) {
throw Exception(
"Decompressed block size {} exceeds the maximum allowed of {} bytes",
declared, maxDecompressLength());
}
}

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 Snappy path now evaluates maxDecompressLength() once into a local and reuses it for both the check and the error message. Pushed in 710bbe2.

Comment on lines +74 to +86
setDecompressLimit("1048576"); // 1 MiB, smaller than the 4 MiB block

bool rejected = false;
try {
DataFileReader<std::string> reader(path.c_str(), schema);
std::string out;
reader.read(out); // triggers block decompression
} catch (const Exception &) {
rejected = true;
}
std::filesystem::remove(path);
BOOST_CHECK_MESSAGE(rejected, std::string("codec not bounded: ") + name);
}

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. Added a RAII DecompressLimitGuard that saves and restores AVRO_MAX_DECOMPRESS_LENGTH (unsetting it if it was previously unset), applied to every test that overrides it, so the override no longer leaks into other test cases. Pushed in 710bbe2.

Comment on lines +119 to +125
setDecompressLimit("1048576");

std::string out;
{
DataFileReader<std::string> reader(path.c_str(), schema);
BOOST_CHECK(reader.read(out));
}

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.

Same RAII guard applied here so this test restores the previous AVRO_MAX_DECOMPRESS_LENGTH value on exit. Pushed in 710bbe2.

- maxDecompressLength() now skips leading whitespace before rejecting an
  explicit sign, so a value like '  -5' is caught (strtoull accepts leading
  whitespace and would otherwise wrap a negative into a huge unsigned value).
- The Snappy path evaluates maxDecompressLength() once and reuses it for both
  the check and the error message.
- The tests use a RAII guard that saves and restores AVRO_MAX_DECOMPRESS_LENGTH
  so an override does not leak into other test cases sharing the process.

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 5 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +46 to +50
// Reject before appending so the buffer never grows past the limit.
if (out.pos > maxLength - uncompressed.size()) {
throw Exception(
"Decompressed block size exceeds the maximum allowed of {} bytes", maxLength);
}

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 zstd streaming path now includes the would-be decompressed size (current size plus the pending chunk) in the exception message, matching the fixed-size zstd and Snappy paths. Pushed in 7ed107b.

The Zstandard streaming (unknown frame size) path reported only the limit when
rejecting an over-large block, unlike the fixed-size zstd and Snappy paths. It
now also reports the would-be decompressed size (current size plus the pending
chunk), matching the other messages and easing debugging and log correlation.

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 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

lang/c++/impl/ZstdDecompressWrapper.cc:33

  • ZSTD_getFrameContentSize() returns an unsigned long long, but the result is stored in a size_t. On platforms where size_t is narrower, this can truncate the declared frame size and lead to incorrect size comparisons (and potentially misleading errors). Store the frame size in an unsigned long long and only convert to size_t after enforcing the limit.
    // Get the decompressed size
    size_t decompressed_size = ZSTD_getFrameContentSize(compressed.data(), compressed.size());
    if (decompressed_size == ZSTD_CONTENTSIZE_ERROR) {
        throw Exception("ZSTD: Not a valid compressed frame");

lang/c++/impl/ZstdDecompressWrapper.cc:72

  • After changing the frame size variable to unsigned long long, the code passes it to std::string::resize() and compares it to result (both size_t). Making the narrowing conversion explicit improves clarity and avoids potential -Wconversion build warnings.
        // Batch decompress the data
        uncompressed.resize(decompressed_size);
        size_t result = ZSTD_decompress(
            uncompressed.data(), decompressed_size, compressed.data(), compressed.size());

        if (ZSTD_isError(result)) {
            throw Exception("ZSTD decompression error: {}", ZSTD_getErrorName(result));
        }
        if (result != decompressed_size) {
            throw Exception("ZSTD: Decompressed size mismatch: expected {}, got {}",
                            decompressed_size, result);
        }

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 5 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +106 to +115
bool rejected = false;
try {
DataFileReader<std::string> reader(path.c_str(), schema);
std::string out;
reader.read(out); // triggers block decompression
} catch (const Exception &) {
rejected = true;
}
std::filesystem::remove(path);
BOOST_CHECK_MESSAGE(rejected, std::string("codec not bounded: ") + name);

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 asserts the caught Exception's message contains "exceeds the maximum allowed", so an unrelated failure (I/O/corruption) can no longer produce a false positive.

… test

checkCodecRejectsOversized treated any avro::Exception as a successful
over-limit rejection, which could pass on an unrelated failure (I/O, corruption).
Assert the caught exception message contains "exceeds the maximum allowed" so the
test verifies the decompression-limit path specifically.

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 5 out of 5 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 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread lang/c++/impl/DataFile.cc Outdated
Comment on lines +696 to +700
size_t newSize = uncompressed.size() + zlibBufGrowSize;
if (newSize > maxLength) {
newSize = maxLength;
}
uncompressed.resize(newSize);

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 buffer now grows by min(maxLength - size, zlibBufGrowSize) (a remaining-capacity calc), so the addition can't overflow size_t when maxLength is near SIZE_MAX.

Comment thread lang/c++/impl/DataFile.cc Outdated
Comment on lines +692 to +694
throw Exception(
"Decompressed block size exceeds the maximum allowed of {} bytes",
maxLength);

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 message now reports the would-be size (uncompressed.size() + 1, i.e. at least maxLength + 1) alongside the limit, consistent with the snappy/zstd errors.

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 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread lang/c++/impl/DataFile.cc Outdated
Comment on lines +691 to +694
(void) inflateEnd(&zs);
throw Exception(
"Decompressed block size exceeds the maximum allowed of {} bytes",
maxLength);

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.

This was addressed in commit 432b785 — the deflate over-limit exception now reports the would-be size (uncompressed.size() + 1, i.e. at least maxLength + 1) alongside the limit: 'Decompressed block size {} exceeds the maximum allowed of {} bytes', consistent with the snappy/zstd errors.

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 5 out of 5 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 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

lang/c++/impl/ZstdDecompressWrapper.cc:32

  • ZSTD_getFrameContentSize() returns an unsigned 64-bit sentinel value (ZSTD_CONTENTSIZE_ERROR / ZSTD_CONTENTSIZE_UNKNOWN). Here it is stored in size_t and then compared to the uncast sentinel constants; on 32-bit builds this comparison can fail due to truncation + integer promotion, causing error/unknown frames to be treated as having a huge declared size. With AVRO_MAX_DECOMPRESS_LENGTH clamped to size_t max, this can lead to attempting to allocate ~4GiB before failing. Cast the sentinel constants to size_t for the comparisons (or keep the return value as unsigned long long and only cast after handling the sentinel values).
std::string ZstdDecompressWrapper::decompress(const std::vector<char> &compressed, size_t maxLength) {
    std::string uncompressed;
    // Get the decompressed size
    size_t decompressed_size = ZSTD_getFrameContentSize(compressed.data(), compressed.size());
    if (decompressed_size == ZSTD_CONTENTSIZE_ERROR) {

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 5 out of 5 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 5 out of 5 changed files in this pull request and generated 6 comments.


static void setDecompressLimit(const char *value) {
#ifdef _WIN32
_putenv_s("AVRO_MAX_DECOMPRESS_LENGTH", value);

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 env-var helpers now assert success via BOOST_REQUIRE_EQUAL(..., 0), so a failed set/clear surfaces instead of silently running with the default limit.

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 — setDecompressLimit/unsetDecompressLimit now return the putenv/setenv/unsetenv status, and every direct call asserts it with BOOST_REQUIRE_EQUAL(..., 0).

#ifdef _WIN32
_putenv_s("AVRO_MAX_DECOMPRESS_LENGTH", value);
#else
setenv("AVRO_MAX_DECOMPRESS_LENGTH", value, 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 env-var helpers now assert success via BOOST_REQUIRE_EQUAL(..., 0), so a failed set/clear surfaces instead of silently running with the default limit.

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 — setDecompressLimit/unsetDecompressLimit now return the putenv/setenv/unsetenv status, and every direct call asserts it with BOOST_REQUIRE_EQUAL(..., 0).


static void unsetDecompressLimit() {
#ifdef _WIN32
_putenv_s("AVRO_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 — the env-var helpers now assert success via BOOST_REQUIRE_EQUAL(..., 0), so a failed set/clear surfaces instead of silently running with the default limit.

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 — setDecompressLimit/unsetDecompressLimit now return the putenv/setenv/unsetenv status, and every direct call asserts it with BOOST_REQUIRE_EQUAL(..., 0).

#ifdef _WIN32
_putenv_s("AVRO_MAX_DECOMPRESS_LENGTH", "");
#else
unsetenv("AVRO_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 — the env-var helpers now assert success via BOOST_REQUIRE_EQUAL(..., 0), so a failed set/clear surfaces instead of silently running with the default limit.

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 — setDecompressLimit/unsetDecompressLimit now return the putenv/setenv/unsetenv status, and every direct call asserts it with BOOST_REQUIRE_EQUAL(..., 0).

BOOST_CHECK_MESSAGE(rejected,
std::string("unexpected exception for ") + name + ": " + e.what());
}
std::filesystem::remove(path);

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 temp-file cleanup now uses BOOST_CHECK(std::filesystem::remove(path)), matching the other C++ tests.

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 asserted — cleanup uses BOOST_CHECK(std::filesystem::remove(path)).

DataFileReader<std::string> reader(path.c_str(), schema);
BOOST_CHECK(reader.read(out));
}
std::filesystem::remove(path);

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 temp-file cleanup now uses BOOST_CHECK(std::filesystem::remove(path)), matching the other C++ tests.

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.

Same — removal is BOOST_CHECK(std::filesystem::remove(path)).

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 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread lang/c++/impl/DataFile.cc Outdated

const uint8_t *data;
size_t len;
const size_t maxLength = maxDecompressLength();

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 path now clamps the working maxLength to std::numeric_limits<uLong>::max(), so a larger AVRO_MAX_DECOMPRESS_LENGTH can't cause the total_out-based buffer arithmetic to wrap where uLong is 32-bit.

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 — maxLength is clamped to std::numeric_limits::max() before the loop so z_stream::total_out (a uLong) can't overflow; the buffer arithmetic uses total_out safely.

…he test

Check the return values of setenv/_putenv_s/unsetenv (via BOOST_REQUIRE_EQUAL)
so a failure to set/clear the limit surfaces instead of the test silently
running with the default, and assert std::filesystem::remove(path) succeeds
(BOOST_CHECK), matching the other C++ tests.

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 5 out of 5 changed files in this pull request and generated 4 comments.

Comment on lines +40 to +54
static void setDecompressLimit(const char *value) {
#ifdef _WIN32
BOOST_REQUIRE_EQUAL(_putenv_s("AVRO_MAX_DECOMPRESS_LENGTH", value), 0);
#else
BOOST_REQUIRE_EQUAL(setenv("AVRO_MAX_DECOMPRESS_LENGTH", value, 1), 0);
#endif
}

static void unsetDecompressLimit() {
#ifdef _WIN32
BOOST_REQUIRE_EQUAL(_putenv_s("AVRO_MAX_DECOMPRESS_LENGTH", ""), 0);
#else
BOOST_REQUIRE_EQUAL(unsetenv("AVRO_MAX_DECOMPRESS_LENGTH"), 0);
#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 helpers now return a status code instead of asserting internally, and the guard destructor uses BOOST_CHECK_EQUAL (non-throwing), so a failure there can't abort a noexcept destructor.

Comment on lines +67 to +73
~DecompressLimitGuard() {
if (previous) {
setDecompressLimit(previous->c_str());
} else {
unsetDecompressLimit();
}
}

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.

Done — destructor uses BOOST_CHECK_EQUAL on the returned status.

writer.close();
}

setDecompressLimit("1048576"); // 1 MiB, smaller than the 4 MiB 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.

Done — this call is now BOOST_REQUIRE_EQUAL(setDecompressLimit("1048576"), 0).

writer.close();
}

setDecompressLimit("1048576");

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.

Done — the within-limit test's setDecompressLimit is now BOOST_REQUIRE_EQUAL(..., 0).

zlib tracks decompressed output in z_stream::total_out (a uLong, 32-bit on some
platforms like Windows). If AVRO_MAX_DECOMPRESS_LENGTH is set above uLong's max,
the buffer-size arithmetic using total_out could wrap. Clamp the working
maxLength to uLong's range for the deflate path.

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 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread lang/c++/impl/DataFile.cc Outdated
Comment on lines +705 to +707
throw Exception(
"Decompressed block size {} exceeds the maximum allowed of {} bytes",
uncompressed.size() + 1, maxLength);

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 over-limit message computes the would-be size with a saturating +1, so it can't wrap to 0 at size_t's maximum.

Comment on lines +47 to +51
if (out.pos > maxLength - uncompressed.size()) {
throw Exception(
"Decompressed block size {} exceeds the maximum allowed of {} bytes",
uncompressed.size() + out.pos, maxLength);
}

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 zstd message uses a saturating add for uncompressed.size() + out.pos.

…essages

- setDecompressLimit/unsetDecompressLimit now return the underlying status code
  instead of asserting internally. DecompressLimitGuard's destructor uses a
  non-throwing BOOST_CHECK_EQUAL (a throwing BOOST_REQUIRE in a noexcept
  destructor could terminate the process); direct callers assert with
  BOOST_REQUIRE_EQUAL so a failed override surfaces immediately.
- The over-limit deflate (DataFile.cc) and zstd (ZstdDecompressWrapper.cc) error
  messages now compute the would-be size with a saturating +1/add, so the
  reported value cannot wrap to a nonsensical number when the configured limit
  is near size_t's maximum.

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 5 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +89 to +91
static std::string tempFile(const char *name) {
return (std::filesystem::temp_directory_path() / name).string();
}

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 — tempFile() now prefixes the filename with the process id (getpid on POSIX, _getpid on Windows), so concurrent runs and stale files no longer collide.

tempFile() returned a deterministic path based only on the codec name, so
concurrent runs (or a stale file from a crashed run) could contend for the same
path. Include the process id (portably: getpid/_getpid) in the filename.

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 5 out of 5 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

C++ Pull Requests for C++ binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants