AVRO-4278: [c++] Bound collection size when decoding arrays and maps#3849
AVRO-4278: [c++] Bound collection size when decoding arrays and maps#3849iemejia wants to merge 2 commits into
Conversation
GenericReader::read() decodes arrays and maps by calling r.resize(r.size() + m) where m is the block count returned by Decoder::arrayStart()/mapStart()/arrayNext()/mapNext(). Because the block count is read from the (potentially untrusted or truncated) input, a very large or malformed count could eagerly allocate an unbounded vector before any element is read. Validate the block count per block and cumulatively against a configurable maximum (AVRO_MAX_COLLECTION_ITEMS), mirroring the Java SDK's collection item limit, and throw before resizing when exceeded. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens the C++ generic decoder against untrusted/truncated inputs by enforcing an upper bound on decoded array/map item counts before resizing the backing containers, preventing attacker-controlled allocations.
Changes:
- Add a configurable max-items limit (
AVRO_MAX_COLLECTION_ITEMS) enforced duringGenericReader::read()for arrays and maps. - Add new codec tests to validate over-limit and within-limit behavior for collections.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
lang/c++/impl/Generic.cc |
Adds max-collection-items parsing from environment and applies the bound before resizing arrays/maps during generic decoding. |
lang/c++/test/CodecTests.cc |
Adds tests for bounded array/map block counts via AVRO_MAX_COLLECTION_ITEMS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size_t maxCollectionItems() { | ||
| const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS"); | ||
| 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 DEFAULT_MAX_COLLECTION_ITEMS; | ||
| } |
There was a problem hiding this comment.
Fixed in be1e180. maxCollectionItems() now rejects any non-decimal-digit character (so a leading -/+ that strtoull would accept no longer disables the limit) and falls back to the default if the parsed value exceeds size_t (guarding truncation on 32-bit builds).
| static void setCollectionLimit(const char *value) { | ||
| #ifdef _WIN32 | ||
| _putenv_s("AVRO_MAX_COLLECTION_ITEMS", value); | ||
| #else | ||
| setenv("AVRO_MAX_COLLECTION_ITEMS", value, 1); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Fixed in be1e180. Added an RAII CollectionLimitGuard that saves and restores (or unsets) AVRO_MAX_COLLECTION_ITEMS on scope exit, so the tests no longer leak state into one another or the rest of the process.
| static void testArrayBlockCountIsBounded() { | ||
| setCollectionLimit("10"); | ||
| const char *schema = R"({"type": "array", "items": "null"})"; |
There was a problem hiding this comment.
Done in be1e180 — both testArrayBlockCountIsBounded and testMapBlockCountIsBounded now construct a CollectionLimitGuard at the top of the test.
| static void testMapBlockCountIsBounded() { | ||
| setCollectionLimit("10"); | ||
| const char *schema = R"({"type": "map", "values": "null"})"; | ||
|
|
||
| // Block count 11 (zigzag = 0x16) exceeds the limit of 10. | ||
| const uint8_t overLimit[] = {0x16, 0x00}; | ||
| BOOST_CHECK_THROW(decodeNullCollection(schema, overLimit, sizeof(overLimit)), | ||
| Exception); | ||
| } |
There was a problem hiding this comment.
Fixed in be1e180. testMapBlockCountIsBounded now covers cumulative (two blocks of 6 pairs, all keyed "a"), negative, and within-limit block counts, mirroring the array test rather than adjusting the description.
- maxCollectionItems() now accepts only a run of decimal digits (rejecting a leading sign that strtoull would otherwise allow and that could disable the limit) and falls back to the default if the value exceeds size_t. - Add an RAII guard that restores/unsets AVRO_MAX_COLLECTION_ITEMS on scope exit so the tests do not affect one another. - Expand the map test to cover cumulative (with repeated keys), negative, and within-limit block counts, mirroring the array test. Assisted-by: GitHub Copilot:claude-opus-4.8
What is the purpose of the change
GenericReader::read()decodes arrays and maps by callingr.resize(r.size() + m), wheremis the block count returned byDecoder::arrayStart()/mapStart()/arrayNext()/mapNext(). Because theblock count is read from the (potentially untrusted or truncated) input, a very
large or malformed count could eagerly allocate an unbounded vector before any
element is read (items such as
nulloccupy no bytes on the wire).This validates the block count — per block and cumulatively — against a
configurable maximum before resizing, mirroring the Java SDK's collection item
limit (
org.apache.avro.limits.collectionItems.maxLength, AVRO-3819). The limitdefaults to
2^31 - 8items and can be overridden with theAVRO_MAX_COLLECTION_ITEMSenvironment variable. Exceeding it throws anavro::Exceptionbefore allocating.This is part of the umbrella issue AVRO-4277.
Verifying this change
This change added tests and can be verified as follows:
testArrayBlockCountIsBoundedandtestMapBlockCountIsBoundedinlang/c++/test/CodecTests.cccovering: a single block count above the limit(array and map), a cumulative limit across blocks, a negative block count
bounded by its absolute value, and a within-limit collection still decoding
correctly.
ctest -R CodecTests(852 cases pass)Documentation
AVRO_MAX_COLLECTION_ITEMSenvironment variable is documented in code comments)