AVRO-4294: [c++] Validate available bytes before allocating for length-prefixed values by iemejia · Pull Request #3859 · apache/avro

iemejia · 2026-07-11T21:20:16Z

What is the purpose of the change

A bytes or string value is encoded as a length prefix followed by that many bytes of data, and an array or map block is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.

This applies the equivalent of the Java SDK fix AVRO-4241 to the C++ SDK: when the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as null) is never falsely rejected. Sources that cannot report their remaining size are unaffected.

Adds remainingBytes() to InputStream (default -1) overridden by the memory input streams and exposed via StreamReader, and bytesRemaining() to the Decoder interface, overridden by BinaryDecoder and forwarded by the validating and resolving decoders. decodeString/decodeBytes and GenericReader's array/map decoding consult it. The collection check is skipped under schema resolution, where the datum (reader) type may be larger than the wire (writer) type.

This is a sub-task of AVRO-4292 and resolves AVRO-4294.

Verifying this change

This change added tests and can be verified as follows:

Added lang/c++/test/AvailableBytesTests.cc covering over-limit string/bytes/array/map rejection, within-limit values, and an array of nulls that must not be falsely rejected.
Run: cd lang/c++ && mkdir build && cd build && cmake .. && make && ctest

Documentation

Does this pull request introduce a new feature? (no — hardening / robustness)
If yes, how is the feature documented? (not applicable)

…h-prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data, causing a correspondingly huge allocation before the shortfall is noticed. - Add remainingBytes() to InputStream (default -1, 'unknown'), overridden by the memory-backed input streams, and exposed via StreamReader. Add bytesRemaining() to the Decoder interface (default -1), overridden by BinaryDecoder and forwarded by the validating and resolving decorators. - decodeString/decodeBytes reject a declared length that exceeds the bytes available before resizing. - GenericReader rejects an array/map block whose element count could not be backed by the bytes remaining, using minBytesPerElement() from the element schema so a zero-byte element (e.g. null) is not falsely rejected. The check is applied only when not resolving, since under resolution the datum (reader) type may be larger than the wire (writer) type and would over-estimate. Mirrors the Java SDK's checks (AVRO-4241). Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Hardens the C++ Avro binary decoding path against malicious/truncated inputs that declare large length-prefixed values or collection block counts, by validating declared sizes against the underlying stream’s known remaining bytes before performing large allocations.

Changes:

Add remainingBytes() to InputStream/StreamReader and bytesRemaining() to the Decoder interface, with forwarding through validating/resolving decoders.
Enforce available-bytes checks in BinaryDecoder::decodeString / decodeBytes and in GenericReader when resizing arrays/maps (when remaining size is known and schema is not resolving).
Add AvailableBytesTests and wire it into the C++ test CMake target.

Reviewed changes

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

Show a summary per file

File	Description
lang/c++/test/AvailableBytesTests.cc	New unit tests covering oversized/within-limit lengths and collection counts, including null-element edge case.
lang/c++/include/avro/Stream.hh	Introduces `InputStream::remainingBytes()` and `StreamReader::remainingBytes()` aggregation.
lang/c++/include/avro/Decoder.hh	Introduces `Decoder::bytesRemaining()` defaulting to unknown (-1).
lang/c++/impl/Stream.cc	Implements `remainingBytes()` for memory-backed input streams.
lang/c++/impl/parsing/ValidatingCodec.cc	Forwards `bytesRemaining()` through validating decoder wrapper.
lang/c++/impl/parsing/ResolvingDecoder.cc	Forwards `bytesRemaining()` through resolving decoder wrapper.
lang/c++/impl/Generic.cc	Adds collection block count vs remaining-bytes validation before resizing containers.
lang/c++/impl/BinaryDecoder.cc	Adds remaining-bytes enforcement for string/bytes length-prefixed allocations.
lang/c++/CMakeLists.txt	Registers the new `AvailableBytesTests` unit test.

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

iemejia · 2026-07-11T22:28:27Z

+    int64_t remainingBytes() const final {
+        // Total capacity across all chunks: full chunks plus the (partial)
+        // last chunk, minus what has already been consumed.
+        int64_t total = (size_ == 0)
+                            ? 0
+                            : static_cast<int64_t>((size_ - 1) * chunkSize_ + available_);
+        return total - static_cast<int64_t>(cur_ * chunkSize_ + curLen_);
+    }


Fixed in 0f223c5: remainingBytes() now widens each operand to int64_t before multiplying, so (size_-1)*chunkSize_ can no longer overflow size_t before the cast.

iemejia · 2026-07-11T22:28:28Z

            for (size_t m = d.arrayStart(); m != 0; m = d.arrayNext()) {
+                ensureCollectionAvailable(d, m, minBytes);
                r.resize(r.size() + m);


Fixed in 0f223c5: array decoding now calls ensureCanGrow(r, m), a max_size()-based guard, before r.resize(r.size() + m), so a huge block count cannot overflow size_t or request an impossible allocation (this also covers null-element arrays where the byte check is skipped).

iemejia · 2026-07-11T22:28:29Z

            for (size_t m = d.mapStart(); m != 0; m = d.mapNext()) {
+                ensureCollectionAvailable(d, m, minBytes);
                r.resize(r.size() + m);


Fixed in 0f223c5: the same max_size()-based ensureCanGrow(r, m) guard is applied before resizing the map container.

… keep check on deep schemas Review feedback: - MemoryInputStream::remainingBytes() computed (size_-1)*chunkSize_ in size_t before the int64_t cast, which could overflow on large buffers and yield a wrong (possibly negative) remaining count. Widen each operand to int64_t before multiplying. - GenericReader's array/map decoding now guards r.resize(r.size() + m) with a max_size()-based check, so a very large block count cannot overflow size_t or request an impossible allocation (this also covers null-element collections, where the per-element minimum is zero and the byte check is skipped). - minBytesPerElement() returns 1 (not 0) when the depth guard trips, so the collection check stays enabled for a crafted deep/recursive schema. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

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

iemejia · 2026-07-11T23:06:29Z

+    switch (node->type()) {
+        case AVRO_NULL:
+            return 0;
+        case AVRO_FLOAT:
+            return 4;
+        case AVRO_DOUBLE:
+            return 8;
+        case AVRO_FIXED:
+            return static_cast<int64_t>(node->fixedSize());
+        case AVRO_RECORD: {
+            int64_t total = 0;
+            for (size_t i = 0; i < node->leaves(); ++i) {
+                total += minBytesPerElement(node->leafAt(i), depth + 1);
+            }
+            return total;
+        }
+        default:
+            // boolean, int, long, bytes, string, enum, union, array, map and
+            // symbolic references all encode to at least one byte.
+            return 1;
+    }


Fixed in 711e27b: minBytesPerElement() clamps the fixed size to INT64_MAX and saturates the record field-minima sum, so an overflow can no longer wrap negative and weaken the guard.

iemejia · 2026-07-11T23:06:31Z

+            // Map keys are strings (>= 1 byte length prefix) plus the value.
+            int64_t minBytes = isResolving ? 0 : (1 + minBytesPerElement(nn, 0));


Fixed in 711e27b: the map's (1 + valueMin) is now saturated (only adds 1 when valueMin < INT64_MAX), so it cannot overflow to a negative minBytes.

Review feedback: minBytesPerElement() could overflow int64_t for a very large fixed size or when summing record field minima, and the map's (1 + valueMin) could wrap. A wrapped negative minimum would disable ensureCollectionAvailable (minBytes <= 0) and bypass the guard. Clamp the fixed size, saturate the record sum, and saturate the map key +1, all to std::numeric_limits<int64_t>::max(). Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

lang/c++/impl/BinaryDecoder.cc:141

decodeString writes into std::string storage by const_cast'ing value.c_str(), but the standard specifies the buffer returned by c_str() must not be modified. Since this C++ code is built with C++20, use value.data() (mutable) or &value[0] after resize() to avoid undefined behavior.

    if (len > 0) {
        in_.readBytes(const_cast<uint8_t *>(
                          reinterpret_cast<const uint8_t *>(value.c_str())),
                      len);
    }

iemejia · 2026-07-11T23:30:38Z

+#include <cstdint>
+#include <string>
+#include <vector>
+


Fixed in 4d983f8: AvailableBytesTests.cc now includes directly instead of relying on an indirect include.

Review feedback: the test uses std::unique_ptr but relied on an indirect include (e.g. via Boost headers), which is non-portable. Include <memory> directly. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

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

iemejia · 2026-07-12T05:26:12Z

+    if (!node) {
+        return 0;
+    }
+    if (depth > 64) {
+        // A cyclic or pathologically deep schema. Return 1 (not 0) so the
+        // collection check stays enabled rather than being silently bypassed;
+        // a valid recursive value always encodes to at least 1 byte.
+        return 1;
+    }


Good catch. Moved the depth guard inside the AVRO_RECORD case so a zero-byte leaf type (null) nested under deeply recursive records still returns 0, while a cyclic/pathologically deep record returns 1 (keeping the collection check enabled). Fixed in 3f6569d.

…ns 0 Review feedback: the depth cutoff was applied before checking the node type, so a zero-byte leaf type (e.g. null) nested under deeply chained records returned 1 instead of 0, enabling the collection check and potentially rejecting valid data. Move the depth guard into the AVRO_RECORD case; leaf types now return their true minimum (null -> 0) regardless of nesting depth. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

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

iemejia · 2026-07-12T06:01:07Z

+            if (depth > 64) {
+                // A cyclic or pathologically deep record. Return 1 (not 0) so the
+                // collection check stays enabled rather than being silently
+                // bypassed; a valid recursive value always encodes to >= 1 byte.
+                // (The depth guard is applied only here, so zero-byte leaf types


Good point. Fixed in bf25d41: the depth guard now returns 0 (a valid conservative lower bound) instead of 1. A truly cyclic schema never reaches the guard because a self-reference is an AVRO_SYMBOLIC node handled by the default case (returning 1 without following the link), so recursion always terminates; the guard is purely a stack-overflow safety net for a genuinely deep acyclic record, where returning 1 would over-estimate and could falsely reject a valid array/map of null-leaf records. Added testReadArrayOfDeeplyNestedNullNotFalselyRejected with a ~70-level nested null-leaf record.

…th guard Review feedback: minBytesPerElement() returned 1 when the depth guard tripped, which over-estimates the true lower bound for a genuinely deep but acyclic schema whose leaves are all null (true minimum 0), and could falsely reject a valid array/map of such elements. A truly cyclic schema never reaches the guard: a self-reference is an AVRO_SYMBOLIC node handled by the default case (returning 1 without following the link), so recursion always terminates. The guard is therefore only a stack-overflow safety net for a pathologically deep acyclic record, where returning 0 (always a valid lower bound) avoids the false rejection. Added a test with a ~70-level nested null-leaf record. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

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

iemejia · 2026-07-12T06:59:28Z

+    int64_t remainingBytes() const {
+        if (in_ == nullptr) {
+            return -1;
+        }
+        int64_t streamRemaining = in_->remainingBytes();
+        if (streamRemaining < 0) {
+            return -1;
+        }
+        // end_ - next_ is the data already buffered in this reader; it is added
+        // to what the underlying stream still has. The addition promotes the
+        // pointer difference to int64_t.
+        return (end_ - next_) + streamRemaining;
+    }


Good catch. Fixed in 83b3b71: the buffered-byte count is now computed as (next_ != nullptr) ? (end_ - next_) : 0, so the pointer subtraction is skipped when next_/end_ are both null (right after init()/reset(), before any fill). Added testBytesRemainingRightAfterInit that calls bytesRemaining() immediately after init() on a BinaryDecoder.

…iningBytes Review feedback: after init()/reset() and before any data is buffered, next_ and end_ are both null, and computing end_ - next_ is undefined behavior. A caller triggers this by calling Decoder::bytesRemaining() immediately after init() on a BinaryDecoder. Guard the buffered-byte count so the subtraction is only performed when next_ is non-null. Added a test that calls bytesRemaining() right after init(). Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

lang/c++/impl/BinaryDecoder.cc:141

decodeString() writes into std::string via const_cast on value.c_str(). Modifying the buffer returned by c_str() is undefined behavior; use a writable pointer (e.g., &value[0] after resize) instead.

    if (len > 0) {
        in_.readBytes(const_cast<uint8_t *>(
                          reinterpret_cast<const uint8_t *>(value.c_str())),
                      len);
    }

github-actions Bot added the C++ Pull Requests for C++ binding label Jul 11, 2026

iemejia requested a review from Copilot July 11, 2026 21:26

Copilot started reviewing on behalf of iemejia July 11, 2026 21:26 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 22:34

Copilot started reviewing on behalf of iemejia July 11, 2026 22:35 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 23:07

Copilot started reviewing on behalf of iemejia July 11, 2026 23:08 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

AVRO-4294: [c++] Include <memory> in AvailableBytesTests

4d983f8

Review feedback: the test uses std::unique_ptr but relied on an indirect include (e.g. via Boost headers), which is non-portable. Include <memory> directly. Assisted-by: GitHub Copilot:claude-opus-4.8

iemejia requested a review from Copilot July 12, 2026 05:03

Copilot started reviewing on behalf of iemejia July 12, 2026 05:04 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 05:36

Copilot started reviewing on behalf of iemejia July 12, 2026 05:36 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 06:45

Copilot started reviewing on behalf of iemejia July 12, 2026 06:46 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 07:21

Copilot started reviewing on behalf of iemejia July 12, 2026 07:21 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

		// Map keys are strings (>= 1 byte length prefix) plus the value.
		int64_t minBytes = isResolving ? 0 : (1 + minBytesPerElement(nn, 0));

Uh oh!

Conversation

iemejia commented Jul 11, 2026

What is the purpose of the change

Verifying this change

Documentation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants