From c22ffb9c9158c048a918ac96135405103d3eb237 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sat, 11 Jul 2026 22:27:28 +0200 Subject: [PATCH 01/21] AVRO-4294: [c++] Validate available bytes before allocating for length-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 --- lang/c++/CMakeLists.txt | 1 + lang/c++/impl/BinaryDecoder.cc | 19 +++ lang/c++/impl/Generic.cc | 60 +++++++++ lang/c++/impl/Stream.cc | 13 ++ lang/c++/impl/parsing/ResolvingDecoder.cc | 4 + lang/c++/impl/parsing/ValidatingCodec.cc | 4 + lang/c++/include/avro/Decoder.hh | 8 ++ lang/c++/include/avro/Stream.hh | 31 +++++ lang/c++/test/AvailableBytesTests.cc | 149 ++++++++++++++++++++++ 9 files changed, 289 insertions(+) create mode 100644 lang/c++/test/AvailableBytesTests.cc diff --git a/lang/c++/CMakeLists.txt b/lang/c++/CMakeLists.txt index a88bc77f096..3e7c5ff1610 100644 --- a/lang/c++/CMakeLists.txt +++ b/lang/c++/CMakeLists.txt @@ -221,6 +221,7 @@ if (AVRO_BUILD_TESTS) unittest (LargeSchemaTests) unittest (CodecTests) unittest (StreamTests) + unittest (AvailableBytesTests) unittest (SpecificTests) unittest (DataFileTests) unittest (JsonTests) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index b334de7cf5d..389d13f0e97 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -53,7 +53,12 @@ class BinaryDecoder : public Decoder { int64_t doDecodeLong(); size_t doDecodeItemCount(); size_t doDecodeLength(); + void checkAvailableBytes(size_t len); void drain() final; + + int64_t bytesRemaining() const final { + return in_.remainingBytes(); + } }; DecoderPtr binaryDecoder() { @@ -109,12 +114,25 @@ size_t BinaryDecoder::doDecodeLength() { return len; } +void BinaryDecoder::checkAvailableBytes(size_t len) { + // Reject a declared length that exceeds the data actually available before + // allocating for it, to guard against an out-of-memory attack from a + // malicious or truncated input. Only enforced when the stream can report + // how many bytes remain. + int64_t remaining = in_.remainingBytes(); + if (remaining >= 0 && static_cast(len) > static_cast(remaining)) { + throw Exception( + "Length {} exceeds the {} bytes available in the stream", len, remaining); + } +} + void BinaryDecoder::drain() { in_.drain(false); } void BinaryDecoder::decodeString(std::string &value) { size_t len = doDecodeLength(); + checkAvailableBytes(len); value.resize(len); if (len > 0) { in_.readBytes(const_cast( @@ -130,6 +148,7 @@ void BinaryDecoder::skipString() { void BinaryDecoder::decodeBytes(std::vector &value) { size_t len = doDecodeLength(); + checkAvailableBytes(len); value.resize(len); if (len > 0) { in_.readBytes(value.data(), len); diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 1535c604be7..523d3be11e2 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -25,6 +25,57 @@ using std::ostringstream; using std::string; using std::vector; +// Minimum number of bytes a single value of the given schema node can occupy on +// the wire. Used to reject an array/map block count that could not be backed by +// the bytes remaining. A type that can encode to zero bytes (null) returns 0, +// which disables the collection check for it (so an array of nulls is not +// falsely rejected). A depth limit breaks self-referencing (symbolic) schemas. +static int64_t minBytesPerElement(const NodePtr &node, int depth) { + if (!node || depth > 64) { + return 0; + } + switch (node->type()) { + case AVRO_NULL: + return 0; + case AVRO_FLOAT: + return 4; + case AVRO_DOUBLE: + return 8; + case AVRO_FIXED: + return static_cast(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; + } +} + +// Reject a collection (array or map) block whose declared element count could +// not be backed by the bytes actually remaining, before resizing for it. +// Skipped when the per-element minimum is zero, or when the decoder cannot +// report how many bytes remain. The comparison divides to avoid overflow. +static void ensureCollectionAvailable(Decoder &d, size_t count, int64_t minBytes) { + if (count == 0 || minBytes <= 0) { + return; + } + int64_t remaining = d.bytesRemaining(); + if (remaining >= 0 && + static_cast(count) > + static_cast(remaining) / static_cast(minBytes)) { + throw Exception( + "Collection claims {} elements with at least {} bytes each, " + "but only {} bytes are available", + count, minBytes, remaining); + } +} + typedef vector bytes; void GenericContainer::assertType(const NodePtr &schema, Type type) { @@ -105,7 +156,13 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { const NodePtr &nn = v.schema()->leafAt(0); r.resize(0); size_t start = 0; + // Only when not resolving: the datum schema then matches the wire + // schema, so minBytesPerElement is a true lower bound. Under + // resolution the wire (writer) type may be smaller than the datum + // (reader) type, which would over-estimate and reject valid data. + int64_t minBytes = isResolving ? 0 : minBytesPerElement(nn, 0); for (size_t m = d.arrayStart(); m != 0; m = d.arrayNext()) { + ensureCollectionAvailable(d, m, minBytes); r.resize(r.size() + m); for (; start < r.size(); ++start) { r[start] = GenericDatum(nn); @@ -119,7 +176,10 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { const NodePtr &nn = v.schema()->leafAt(1); r.resize(0); size_t start = 0; + // Map keys are strings (>= 1 byte length prefix) plus the value. + int64_t minBytes = isResolving ? 0 : (1 + minBytesPerElement(nn, 0)); for (size_t m = d.mapStart(); m != 0; m = d.mapNext()) { + ensureCollectionAvailable(d, m, minBytes); r.resize(r.size() + m); for (; start < r.size(); ++start) { d.decodeString(r[start].first); diff --git a/lang/c++/impl/Stream.cc b/lang/c++/impl/Stream.cc index 1ca5c346466..245dc59c80e 100644 --- a/lang/c++/impl/Stream.cc +++ b/lang/c++/impl/Stream.cc @@ -84,6 +84,15 @@ class MemoryInputStream : public InputStream { size_t byteCount() const final { return cur_ * chunkSize_ + curLen_; } + + 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((size_ - 1) * chunkSize_ + available_); + return total - static_cast(cur_ * chunkSize_ + curLen_); + } }; class MemoryInputStream2 : public InputStream { @@ -119,6 +128,10 @@ class MemoryInputStream2 : public InputStream { size_t byteCount() const final { return curLen_; } + + int64_t remainingBytes() const final { + return static_cast(size_ - curLen_); + } }; class MemoryOutputStream final : public OutputStream { diff --git a/lang/c++/impl/parsing/ResolvingDecoder.cc b/lang/c++/impl/parsing/ResolvingDecoder.cc index 1553b8a4b62..dd5e342a2c1 100644 --- a/lang/c++/impl/parsing/ResolvingDecoder.cc +++ b/lang/c++/impl/parsing/ResolvingDecoder.cc @@ -471,6 +471,10 @@ class ResolvingDecoderImpl : public ResolvingDecoder { base_->drain(); } + int64_t bytesRemaining() const final { + return base_->bytesRemaining(); + } + public: ResolvingDecoderImpl(const ValidSchema &writer, const ValidSchema &reader, DecoderPtr base) : base_(std::move(base)), diff --git a/lang/c++/impl/parsing/ValidatingCodec.cc b/lang/c++/impl/parsing/ValidatingCodec.cc index 9ec1f040600..58bd5fb79f1 100644 --- a/lang/c++/impl/parsing/ValidatingCodec.cc +++ b/lang/c++/impl/parsing/ValidatingCodec.cc @@ -187,6 +187,10 @@ class ValidatingDecoder : public Decoder { base->drain(); } + int64_t bytesRemaining() const final { + return base->bytesRemaining(); + } + public: ValidatingDecoder(const ValidSchema &s, const shared_ptr &b) : base(b), parser(ValidatingGrammarGenerator().generate(s), NULL, handler_) {} diff --git a/lang/c++/include/avro/Decoder.hh b/lang/c++/include/avro/Decoder.hh index 77b3f9489f7..5342c1cd5e3 100644 --- a/lang/c++/include/avro/Decoder.hh +++ b/lang/c++/include/avro/Decoder.hh @@ -169,6 +169,14 @@ public: /// by the avro decoder. Similar set of problems occur if the Decoder /// consumes more than what it should. virtual void drain() = 0; + + /// Returns the number of bytes that remain to be read from the underlying + /// stream, or a negative value when that count is not known (for example a + /// streaming source, or a decoder not backed by a byte stream). The default + /// is "unknown"; byte-stream decoders override it so a length prefix or a + /// collection block count that exceeds the data actually available can be + /// rejected before allocating for it. + virtual int64_t bytesRemaining() const { return -1; } }; /** diff --git a/lang/c++/include/avro/Stream.hh b/lang/c++/include/avro/Stream.hh index de213404d3f..02f7dcd16cf 100644 --- a/lang/c++/include/avro/Stream.hh +++ b/lang/c++/include/avro/Stream.hh @@ -74,6 +74,17 @@ public: * to be used unless, returned back using backup. */ virtual size_t byteCount() const = 0; + + /** + * Returns the number of bytes still available to be read from this + * stream, or a negative value when that count is not known (for example a + * streaming source that cannot report its size). The default is + * "unknown". Memory-backed streams override this so a length prefix that + * exceeds the data actually available can be rejected before allocating + * for it, guarding against an out-of-memory attack from a malicious or + * truncated input. + */ + virtual int64_t remainingBytes() const { return -1; } }; typedef std::unique_ptr InputStreamPtr; @@ -345,6 +356,26 @@ struct StreamReader { return next_ != end_ || fill(); } + /** + * Returns the number of bytes still available to be read: those already + * buffered in this reader plus whatever the underlying stream reports as + * remaining. Returns a negative value when the underlying stream cannot + * report its remaining size. + */ + 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; + } + /** * Returns unused bytes back to the underlying stream. * If unRead is true the last byte read is also pushed back. diff --git a/lang/c++/test/AvailableBytesTests.cc b/lang/c++/test/AvailableBytesTests.cc new file mode 100644 index 00000000000..b4cf5b84622 --- /dev/null +++ b/lang/c++/test/AvailableBytesTests.cc @@ -0,0 +1,149 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// A bytes/string value is encoded as a length prefix followed by that many +// bytes of data. A malicious or truncated input can declare a huge length with +// little or no actual data, which would otherwise trigger a correspondingly +// huge allocation before the shortfall is noticed. When the input stream can +// report how many bytes remain (a memory-backed stream), the declared length +// must be rejected before allocating for it. + +#include +#include +#include + +#include + +#include "Compiler.hh" +#include "Decoder.hh" +#include "Encoder.hh" +#include "Exception.hh" +#include "Generic.hh" +#include "Stream.hh" +#include "ValidSchema.hh" + +namespace avro { + +// Reading a bytes value whose declared length far exceeds the tiny backing +// buffer must throw instead of attempting a huge allocation. +static void testDecodeBytesRejectsOversizedLength() { + // 0xFE 0x01 is the zig-zag long 127; the buffer carries no data after it. + const uint8_t data[] = {0xFE, 0x01}; + InputStreamPtr in = memoryInputStream(data, sizeof(data)); + DecoderPtr d = binaryDecoder(); + d->init(*in); + std::vector value; + BOOST_CHECK_THROW(d->decodeBytes(value), Exception); +} + +static void testDecodeStringRejectsOversizedLength() { + const uint8_t data[] = {0xFE, 0x01}; + InputStreamPtr in = memoryInputStream(data, sizeof(data)); + DecoderPtr d = binaryDecoder(); + d->init(*in); + std::string value; + BOOST_CHECK_THROW(d->decodeString(value), Exception); +} + +// A well-formed value whose declared length fits the buffer still decodes. +static void testDecodeStringWithinLimitStillReads() { + // length 3 (zig-zag 6 -> 0x06) followed by "abc". + const uint8_t data[] = {0x06, 'a', 'b', 'c'}; + InputStreamPtr in = memoryInputStream(data, sizeof(data)); + DecoderPtr d = binaryDecoder(); + d->init(*in); + std::string value; + d->decodeString(value); + BOOST_CHECK_EQUAL(value, std::string("abc")); +} + +static void testDecodeBytesWithinLimitStillReads() { + const uint8_t data[] = {0x06, 0x01, 0x02, 0x03}; + InputStreamPtr in = memoryInputStream(data, sizeof(data)); + DecoderPtr d = binaryDecoder(); + d->init(*in); + std::vector value; + d->decodeBytes(value); + BOOST_CHECK_EQUAL(value.size(), 3u); + BOOST_CHECK_EQUAL(value[0], 1); + BOOST_CHECK_EQUAL(value[2], 3); +} + +// An array/map block declares an element count; a malicious or truncated input +// can declare far more elements than the remaining bytes could hold. The count +// is validated against the bytes remaining before resizing, using the minimum +// on-wire size of the element schema (so 0-byte elements like null are not +// falsely rejected). +static GenericDatum decodeCollectionHeader(const ValidSchema &s, int64_t blockCount, + bool addEndMarker) { + // Keep the output stream alive for the whole decode: memoryInputStream + // references the output stream's buffer rather than copying it. + std::unique_ptr os = memoryOutputStream(); + EncoderPtr e = binaryEncoder(); + e->init(*os); + e->encodeLong(blockCount); + if (addEndMarker) { + e->encodeLong(0); + } + e->flush(); + + InputStreamPtr in = memoryInputStream(*os); + DecoderPtr d = binaryDecoder(); + d->init(*in); + GenericDatum datum; + GenericReader::read(*d, datum, s); + return datum; +} + +static void testReadArrayRejectsOversizedCount() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"array\",\"items\":\"long\"}"); + // 1,000,000 long elements declared, but no element data follows. + BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception); +} + +static void testReadMapRejectsOversizedCount() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"map\",\"values\":\"long\"}"); + BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception); +} + +static void testReadArrayOfNullNotFalselyRejected() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"array\",\"items\":\"null\"}"); + // 100,000 nulls (zero bytes each) is legitimate and must decode. + GenericDatum datum = decodeCollectionHeader(s, 100000, true); + BOOST_CHECK_EQUAL(datum.value().value().size(), 100000u); +} + +} // namespace avro + +boost::unit_test::test_suite * +init_unit_test_suite(int, char *[]) { + using namespace boost::unit_test; + + auto *ts = BOOST_TEST_SUITE("Avro C++ available-bytes tests"); + ts->add(BOOST_TEST_CASE(&avro::testDecodeBytesRejectsOversizedLength)); + ts->add(BOOST_TEST_CASE(&avro::testDecodeStringRejectsOversizedLength)); + ts->add(BOOST_TEST_CASE(&avro::testDecodeStringWithinLimitStillReads)); + ts->add(BOOST_TEST_CASE(&avro::testDecodeBytesWithinLimitStillReads)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayRejectsOversizedCount)); + ts->add(BOOST_TEST_CASE(&avro::testReadMapRejectsOversizedCount)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullNotFalselyRejected)); + return ts; +} From 0f223c5d12e616624a9440973ef71a4c69ab6177 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 00:18:42 +0200 Subject: [PATCH 02/21] AVRO-4294: [c++] Fix size_t overflow in remainingBytes; guard resize; 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 --- lang/c++/impl/Generic.cc | 21 ++++++++++++++++++++- lang/c++/impl/Stream.cc | 8 +++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 523d3be11e2..a6c2bb9f216 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -31,9 +31,15 @@ using std::vector; // which disables the collection check for it (so an array of nulls is not // falsely rejected). A depth limit breaks self-referencing (symbolic) schemas. static int64_t minBytesPerElement(const NodePtr &node, int depth) { - if (!node || depth > 64) { + 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; + } switch (node->type()) { case AVRO_NULL: return 0; @@ -76,6 +82,17 @@ static void ensureCollectionAvailable(Decoder &d, size_t count, int64_t minBytes } } +// Guard against size_t overflow / an over-large request when growing a +// collection container by `count` elements before calling resize(). +template +static void ensureCanGrow(const Container &c, size_t count) { + if (count > c.max_size() - c.size()) { + throw Exception( + "Collection block count {} exceeds the maximum container size", + count); + } +} + typedef vector bytes; void GenericContainer::assertType(const NodePtr &schema, Type type) { @@ -163,6 +180,7 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { int64_t minBytes = isResolving ? 0 : minBytesPerElement(nn, 0); for (size_t m = d.arrayStart(); m != 0; m = d.arrayNext()) { ensureCollectionAvailable(d, m, minBytes); + ensureCanGrow(r, m); r.resize(r.size() + m); for (; start < r.size(); ++start) { r[start] = GenericDatum(nn); @@ -180,6 +198,7 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { int64_t minBytes = isResolving ? 0 : (1 + minBytesPerElement(nn, 0)); for (size_t m = d.mapStart(); m != 0; m = d.mapNext()) { ensureCollectionAvailable(d, m, minBytes); + ensureCanGrow(r, m); r.resize(r.size() + m); for (; start < r.size(); ++start) { d.decodeString(r[start].first); diff --git a/lang/c++/impl/Stream.cc b/lang/c++/impl/Stream.cc index 245dc59c80e..f6140479f4d 100644 --- a/lang/c++/impl/Stream.cc +++ b/lang/c++/impl/Stream.cc @@ -87,11 +87,13 @@ class MemoryInputStream : public InputStream { int64_t remainingBytes() const final { // Total capacity across all chunks: full chunks plus the (partial) - // last chunk, minus what has already been consumed. + // last chunk, minus what has already been consumed. Widen to int64_t + // before multiplying so the arithmetic cannot overflow size_t. int64_t total = (size_ == 0) ? 0 - : static_cast((size_ - 1) * chunkSize_ + available_); - return total - static_cast(cur_ * chunkSize_ + curLen_); + : static_cast(size_ - 1) * static_cast(chunkSize_) + static_cast(available_); + int64_t consumed = static_cast(cur_) * static_cast(chunkSize_) + static_cast(curLen_); + return total - consumed; } }; From 711e27b4567c385de04e875e0e5483b2103a1960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 00:59:13 +0200 Subject: [PATCH 03/21] AVRO-4294: [c++] Saturate min-bytes arithmetic to INT64_MAX 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::max(). Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/Generic.cc | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index a6c2bb9f216..64fa379e6d4 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -17,6 +17,7 @@ */ #include "Generic.hh" +#include #include namespace avro { @@ -47,12 +48,24 @@ static int64_t minBytesPerElement(const NodePtr &node, int depth) { return 4; case AVRO_DOUBLE: return 8; - case AVRO_FIXED: - return static_cast(node->fixedSize()); + case AVRO_FIXED: { + // fixedSize() is a size_t; clamp to int64_t so a huge fixed size + // cannot wrap negative. + size_t sz = node->fixedSize(); + return sz > static_cast(std::numeric_limits::max()) + ? std::numeric_limits::max() + : static_cast(sz); + } case AVRO_RECORD: { int64_t total = 0; for (size_t i = 0; i < node->leaves(); ++i) { - total += minBytesPerElement(node->leafAt(i), depth + 1); + int64_t fieldMin = minBytesPerElement(node->leafAt(i), depth + 1); + // Saturate rather than overflow: a wrapped (negative) total + // would disable the collection check. + if (fieldMin > std::numeric_limits::max() - total) { + return std::numeric_limits::max(); + } + total += fieldMin; } return total; } @@ -195,7 +208,13 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { r.resize(0); size_t start = 0; // Map keys are strings (>= 1 byte length prefix) plus the value. - int64_t minBytes = isResolving ? 0 : (1 + minBytesPerElement(nn, 0)); + // Saturate the +1 so a maxed-out value minimum cannot wrap. + int64_t valuesMin = minBytesPerElement(nn, 0); + int64_t minBytes = isResolving + ? 0 + : (valuesMin < std::numeric_limits::max() + ? valuesMin + 1 + : valuesMin); for (size_t m = d.mapStart(); m != 0; m = d.mapNext()) { ensureCollectionAvailable(d, m, minBytes); ensureCanGrow(r, m); From 4d983f8f2952394f94a7e204f438e9c8bffcf93e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 01:21:38 +0200 Subject: [PATCH 04/21] AVRO-4294: [c++] Include in AvailableBytesTests Review feedback: the test uses std::unique_ptr but relied on an indirect include (e.g. via Boost headers), which is non-portable. Include directly. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/test/AvailableBytesTests.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/lang/c++/test/AvailableBytesTests.cc b/lang/c++/test/AvailableBytesTests.cc index b4cf5b84622..80e79124d44 100644 --- a/lang/c++/test/AvailableBytesTests.cc +++ b/lang/c++/test/AvailableBytesTests.cc @@ -24,6 +24,7 @@ // must be rejected before allocating for it. #include +#include #include #include From 3f6569da284c5838566a01ea5c54170342b83a2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 07:13:26 +0200 Subject: [PATCH 05/21] AVRO-4294: [c++] Apply depth guard only to records so deep null returns 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 --- lang/c++/impl/Generic.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 64fa379e6d4..1c0d421e23a 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -35,12 +35,6 @@ static int64_t minBytesPerElement(const NodePtr &node, int depth) { 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; - } switch (node->type()) { case AVRO_NULL: return 0; @@ -57,6 +51,14 @@ static int64_t minBytesPerElement(const NodePtr &node, int depth) { : static_cast(sz); } case AVRO_RECORD: { + 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 + // such as null still return 0 regardless of nesting depth.) + return 1; + } int64_t total = 0; for (size_t i = 0; i < node->leaves(); ++i) { int64_t fieldMin = minBytesPerElement(node->leafAt(i), depth + 1); From bf25d413cf7684e78ebcc5eec8c59d3778ec8281 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 08:00:59 +0200 Subject: [PATCH 06/21] AVRO-4294: [c++] Return a conservative 0 minimum at the recursion depth 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 --- lang/c++/impl/Generic.cc | 17 +++++++++++------ lang/c++/test/AvailableBytesTests.cc | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 1c0d421e23a..2b18235f6a4 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -52,12 +52,17 @@ static int64_t minBytesPerElement(const NodePtr &node, int depth) { } case AVRO_RECORD: { 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 - // such as null still return 0 regardless of nesting depth.) - return 1; + // Purely a recursion (stack-overflow) safety net for a + // pathologically deep schema. A truly cyclic schema never + // reaches this: a self-reference is an AVRO_SYMBOLIC node, + // handled by the default case below (returning 1 without + // following the link), so recursion always terminates. This + // guard therefore only trips on a genuinely deep *acyclic* + // record, whose true minimum can still be 0 (e.g. a long chain + // of records whose only leaves are null). Return 0 rather than + // over-estimating, so a valid array/map of such elements is not + // falsely rejected; 0 is always a valid lower bound. + return 0; } int64_t total = 0; for (size_t i = 0; i < node->leaves(); ++i) { diff --git a/lang/c++/test/AvailableBytesTests.cc b/lang/c++/test/AvailableBytesTests.cc index 80e79124d44..288edc0f982 100644 --- a/lang/c++/test/AvailableBytesTests.cc +++ b/lang/c++/test/AvailableBytesTests.cc @@ -132,6 +132,26 @@ static void testReadArrayOfNullNotFalselyRejected() { BOOST_CHECK_EQUAL(datum.value().value().size(), 100000u); } +// A deeply but acyclically nested record whose only leaf is null encodes to +// zero bytes, so the per-element minimum must be 0 and a large array of such +// records must not be falsely rejected. This exercises the recursion depth +// guard in minBytesPerElement(), which must yield a conservative lower bound +// (0) rather than over-estimating for a legitimately deep schema. +static void testReadArrayOfDeeplyNestedNullNotFalselyRejected() { + // Build ~70 nested records: R0 { null f; }, R1 { R0 f; }, ... The nesting + // exceeds the depth guard, yet every leaf is null so the true minimum is 0. + std::string schema = "\"null\""; + for (int i = 0; i < 70; ++i) { + schema = "{\"type\":\"record\",\"name\":\"R" + std::to_string(i) + + "\",\"fields\":[{\"name\":\"f\",\"type\":" + schema + "}]}"; + } + ValidSchema s = compileJsonSchemaFromString( + ("{\"type\":\"array\",\"items\":" + schema + "}").c_str()); + // 100,000 zero-byte elements is legitimate and must decode. + GenericDatum datum = decodeCollectionHeader(s, 100000, true); + BOOST_CHECK_EQUAL(datum.value().value().size(), 100000u); +} + } // namespace avro boost::unit_test::test_suite * @@ -146,5 +166,6 @@ init_unit_test_suite(int, char *[]) { ts->add(BOOST_TEST_CASE(&avro::testReadArrayRejectsOversizedCount)); ts->add(BOOST_TEST_CASE(&avro::testReadMapRejectsOversizedCount)); ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullNotFalselyRejected)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfDeeplyNestedNullNotFalselyRejected)); return ts; } From 83b3b7137add84d482d06de11d589d149c2beb10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 08:59:21 +0200 Subject: [PATCH 07/21] AVRO-4294: [c++] Avoid null pointer subtraction in StreamReader::remainingBytes 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 --- lang/c++/include/avro/Stream.hh | 9 ++++++--- lang/c++/test/AvailableBytesTests.cc | 12 ++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lang/c++/include/avro/Stream.hh b/lang/c++/include/avro/Stream.hh index 02f7dcd16cf..32cb2569f6b 100644 --- a/lang/c++/include/avro/Stream.hh +++ b/lang/c++/include/avro/Stream.hh @@ -371,9 +371,12 @@ struct StreamReader { 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; + // to what the underlying stream still has. Guard the pointer + // subtraction: after init()/reset() and before any data is buffered, + // next_ and end_ are both null, and subtracting null pointers is + // undefined behavior. + int64_t buffered = (next_ != nullptr) ? (end_ - next_) : 0; + return buffered + streamRemaining; } /** diff --git a/lang/c++/test/AvailableBytesTests.cc b/lang/c++/test/AvailableBytesTests.cc index 288edc0f982..f6b1e62868e 100644 --- a/lang/c++/test/AvailableBytesTests.cc +++ b/lang/c++/test/AvailableBytesTests.cc @@ -152,6 +152,17 @@ static void testReadArrayOfDeeplyNestedNullNotFalselyRejected() { BOOST_CHECK_EQUAL(datum.value().value().size(), 100000u); } +// Calling bytesRemaining() immediately after init(), before any data has been +// buffered, must be well-defined (the underlying StreamReader must not subtract +// null buffer pointers) and report the whole stream as available. +static void testBytesRemainingRightAfterInit() { + const uint8_t data[] = {0x06, 'a', 'b', 'c'}; + InputStreamPtr in = memoryInputStream(data, sizeof(data)); + DecoderPtr d = binaryDecoder(); + d->init(*in); + BOOST_CHECK_EQUAL(d->bytesRemaining(), static_cast(sizeof(data))); +} + } // namespace avro boost::unit_test::test_suite * @@ -167,5 +178,6 @@ init_unit_test_suite(int, char *[]) { ts->add(BOOST_TEST_CASE(&avro::testReadMapRejectsOversizedCount)); ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullNotFalselyRejected)); ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfDeeplyNestedNullNotFalselyRejected)); + ts->add(BOOST_TEST_CASE(&avro::testBytesRemainingRightAfterInit)); return ts; } From 17b7a7b19fccd513293f39b6ae7f2582608c0393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 14:04:39 +0200 Subject: [PATCH 08/21] AVRO-4294: [c++] Cap zero-byte collection element allocation Completes the available-bytes protection for collections and supersedes the separate collection-limit change. Elements whose schema encodes to zero bytes (null, a zero-length fixed, or a record with only zero-byte fields) consume no input, so ensureCollectionAvailable cannot bound their count. A tiny payload declaring a huge array block count of such elements (e.g. {"type":"array","items":"null"} with a count of 200,000,000) therefore drove a single huge vector resize and exhausted memory. ensureCollectionAvailable now enforces the bytes-remaining check plus a structural cap on all collections (kDefaultMaxCollectionStructural = Integer.MAX_VALUE - 8), and ensureZeroByteCollectionWithinLimit caps zero-byte elements at kDefaultMaxCollectionItems = 10,000,000; the true element minimum is computed independently of the resolving flag (which forces minBytes to 0). AVRO_MAX_COLLECTION_ITEMS, when set, caps both. BinaryDecoder::skipArray is also bounded by the structural cap so skipping a huge block cannot loop unboundedly. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 28 ++++++++ lang/c++/impl/Generic.cc | 76 ++++++++++++++++++++-- lang/c++/test/AvailableBytesTests.cc | 97 ++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+), 4 deletions(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index 389d13f0e97..d6f83035811 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -19,12 +19,28 @@ #include "Decoder.hh" #include "Exception.hh" #include "Zigzag.hh" +#include #include namespace avro { using std::make_shared; +// Structural cap on the number of elements to skip in an array or map. Mirrors +// the read-path limit in Generic.cc; AVRO_MAX_COLLECTION_ITEMS, when a +// non-negative integer, overrides it. +static int64_t maxCollectionStructural() { + const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS"); + if (env != nullptr && *env != '\0') { + char *end = nullptr; + long long value = std::strtoll(env, &end, 10); + if (*end == '\0' && value >= 0) { + return static_cast(value); + } + } + return 2147483639; +} + class BinaryDecoder : public Decoder { StreamReader in_; @@ -199,6 +215,18 @@ size_t BinaryDecoder::skipArray() { auto n = static_cast(doDecodeLong()); in_.skipBytes(n); } else { + // Bound the block count: skipping a huge block of zero-byte elements + // would otherwise loop unboundedly (a CPU exhaustion) even though it + // reads/allocates nothing. The decoder has no element schema here, so + // apply the structural cap (AVRO_MAX_COLLECTION_ITEMS, default + // Integer.MAX_VALUE - 8). + static const int64_t structural = maxCollectionStructural(); + if (r > structural) { + throw Exception( + "Cannot skip a collection of more than {} elements; " + "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate", + structural); + } return static_cast(r); } } diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 2b18235f6a4..8d089d7970b 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -17,6 +17,7 @@ */ #include "Generic.hh" +#include #include #include @@ -83,11 +84,40 @@ static int64_t minBytesPerElement(const NodePtr &node, int depth) { } } +// Default maximum number of zero-byte-encoded collection elements (e.g. an +// array of nulls) to allocate from a single decode. Such elements consume no +// input, so ensureCollectionAvailable cannot bound their count; without a cap a +// tiny payload can declare a huge block count and exhaust memory. Overridable +// via the AVRO_MAX_COLLECTION_ITEMS environment variable. +static constexpr int64_t kDefaultMaxCollectionItems = 10000000; + +// Structural cap on the number of elements in any array or map (an overflow / +// defense-in-depth guard), matching the historical Integer.MAX_VALUE - 8 limit. +// Non-zero-byte elements are also bounded by the bytes remaining. +static constexpr int64_t kDefaultMaxCollectionStructural = 2147483639; + +// AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both limits; +// otherwise zero-byte elements use the tighter default and all collections use +// the structural default. +static void collectionLimits(int64_t &zeroByte, int64_t &structural) { + const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS"); + if (env != nullptr && *env != '\0') { + char *end = nullptr; + long long value = std::strtoll(env, &end, 10); + if (*end == '\0' && value >= 0) { + zeroByte = structural = static_cast(value); + return; + } + } + zeroByte = kDefaultMaxCollectionItems; + structural = kDefaultMaxCollectionStructural; +} + // Reject a collection (array or map) block whose declared element count could // not be backed by the bytes actually remaining, before resizing for it. // Skipped when the per-element minimum is zero, or when the decoder cannot // report how many bytes remain. The comparison divides to avoid overflow. -static void ensureCollectionAvailable(Decoder &d, size_t count, int64_t minBytes) { +static void ensureCollectionAvailable(Decoder &d, size_t existing, size_t count, int64_t minBytes) { if (count == 0 || minBytes <= 0) { return; } @@ -100,6 +130,35 @@ static void ensureCollectionAvailable(Decoder &d, size_t count, int64_t minBytes "but only {} bytes are available", count, minBytes, remaining); } + // Structural / overflow guard, also covering decoders that cannot report the + // bytes remaining. Compare without adding so existing + count cannot overflow. + int64_t zeroByte, structural; + collectionLimits(zeroByte, structural); + const uint64_t limit = static_cast(structural); + if (static_cast(count) > limit || + static_cast(existing) > limit - static_cast(count)) { + throw Exception( + "Cannot read a collection of more than {} elements; " + "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate", + structural); + } +} + +// Reject a collection of zero-byte elements (e.g. null) whose cumulative count +// exceeds the configured limit. These elements consume no input, so they cannot +// be bounded by the bytes remaining; the count is the only signal. +static void ensureZeroByteCollectionWithinLimit(size_t existing, size_t count) { + int64_t zeroByte, structural; + collectionLimits(zeroByte, structural); + const uint64_t limit = static_cast(zeroByte); + // Compare without adding, so existing + count cannot overflow. + if (static_cast(count) > limit || + static_cast(existing) > limit - static_cast(count)) { + throw Exception( + "Cannot read a collection of more than {} zero-byte elements; " + "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate", + zeroByte); + } } // Guard against size_t overflow / an over-large request when growing a @@ -197,9 +256,16 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { // schema, so minBytesPerElement is a true lower bound. Under // resolution the wire (writer) type may be smaller than the datum // (reader) type, which would over-estimate and reject valid data. - int64_t minBytes = isResolving ? 0 : minBytesPerElement(nn, 0); + int64_t trueMin = minBytesPerElement(nn, 0); + bool zeroByte = trueMin == 0; + int64_t minBytes = isResolving ? 0 : trueMin; for (size_t m = d.arrayStart(); m != 0; m = d.arrayNext()) { - ensureCollectionAvailable(d, m, minBytes); + ensureCollectionAvailable(d, r.size(), m, minBytes); + // Zero-byte elements are not bounded by the bytes check, so cap + // their cumulative count (r.size() is the count so far). + if (zeroByte) { + ensureZeroByteCollectionWithinLimit(r.size(), m); + } ensureCanGrow(r, m); r.resize(r.size() + m); for (; start < r.size(); ++start) { @@ -217,13 +283,15 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { // Map keys are strings (>= 1 byte length prefix) plus the value. // Saturate the +1 so a maxed-out value minimum cannot wrap. int64_t valuesMin = minBytesPerElement(nn, 0); + // A map entry always includes a >= 1 byte key, so it is never a + // zero-byte element; the bytes check alone bounds it. int64_t minBytes = isResolving ? 0 : (valuesMin < std::numeric_limits::max() ? valuesMin + 1 : valuesMin); for (size_t m = d.mapStart(); m != 0; m = d.mapNext()) { - ensureCollectionAvailable(d, m, minBytes); + ensureCollectionAvailable(d, r.size(), m, minBytes); ensureCanGrow(r, m); r.resize(r.size() + m); for (; start < r.size(); ++start) { diff --git a/lang/c++/test/AvailableBytesTests.cc b/lang/c++/test/AvailableBytesTests.cc index f6b1e62868e..12a09c6cdb1 100644 --- a/lang/c++/test/AvailableBytesTests.cc +++ b/lang/c++/test/AvailableBytesTests.cc @@ -24,6 +24,7 @@ // must be rejected before allocating for it. #include +#include #include #include #include @@ -163,6 +164,95 @@ static void testBytesRemainingRightAfterInit() { BOOST_CHECK_EQUAL(d->bytesRemaining(), static_cast(sizeof(data))); } +// Zero-byte elements (null, a record with only zero-byte fields) consume no +// input, so ensureCollectionAvailable cannot bound their count. A huge declared +// block count of such elements is capped against a configurable limit. +static GenericDatum decodeLongs(const ValidSchema &s, const std::vector &longs) { + std::unique_ptr os = memoryOutputStream(); + EncoderPtr e = binaryEncoder(); + e->init(*os); + for (int64_t v : longs) { + e->encodeLong(v); + } + e->flush(); + InputStreamPtr in = memoryInputStream(*os); + DecoderPtr d = binaryDecoder(); + d->init(*in); + GenericDatum datum; + GenericReader::read(*d, datum, s); + return datum; +} + +static void testReadArrayOfNullRejectsCountAboveDefaultLimit() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"array\",\"items\":\"null\"}"); + // The reported exploit: 200,000,000 nulls rejected by the default limit. + BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception); +} + +static void testReadArrayOfAllNullRecordRejectsHugeCount() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"array\",\"items\":" + "{\"type\":\"record\",\"name\":\"R\",\"fields\":[{\"name\":\"n\",\"type\":\"null\"}]}}"); + BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception); +} + +static void testReadArrayOfNullRejectsNegativeCount() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"array\",\"items\":\"null\"}"); + // Negative count (-200M), block byte-size 0, end marker 0: normalized to + // 200M and still bounded. + BOOST_CHECK_THROW(decodeLongs(s, {-200000000, 0, 0}), Exception); +} + +static void testReadMapOfNullRejectedByAvailableBytes() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"map\",\"values\":\"null\"}"); + // Each map entry carries a >= 1 byte key, so a huge map is bounded by + // the bytes-remaining check. + BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, false), Exception); +} + +static void testReadArrayOfNullRespectsConfiguredLimit() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"array\",\"items\":\"null\"}"); + setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1); + // Within the limit decodes; over the limit and cumulative are rejected. + GenericDatum ok = decodeCollectionHeader(s, 1000, true); + BOOST_CHECK_EQUAL(ok.value().value().size(), 1000u); + BOOST_CHECK_THROW(decodeCollectionHeader(s, 1001, true), Exception); + BOOST_CHECK_THROW(decodeLongs(s, {600, 600, 0}), Exception); + unsetenv("AVRO_MAX_COLLECTION_ITEMS"); +} + +// A backed non-zero-byte array that passes the bytes check is still bounded by +// the structural cap (exercised with a lowered limit). +static void testReadArrayOfLongRejectedByStructuralCap() { + ValidSchema s = compileJsonSchemaFromString( + "{\"type\":\"array\",\"items\":\"long\"}"); + setenv("AVRO_MAX_COLLECTION_ITEMS", "5", 1); + // 10 real longs: block count 10, ten 1-byte longs, end marker. + std::vector longs = {10, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0}; + BOOST_CHECK_THROW(decodeLongs(s, longs), Exception); + unsetenv("AVRO_MAX_COLLECTION_ITEMS"); +} + +// Skipping a huge array (via the decoder's skipArray) is bounded by the +// structural cap. +static void testSkipArrayRejectsHugeCount() { + setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1); + std::unique_ptr os = memoryOutputStream(); + EncoderPtr e = binaryEncoder(); + e->init(*os); + e->encodeLong(2000); // block count 2000, no data needed (skip throws first) + e->flush(); + InputStreamPtr in = memoryInputStream(*os); + DecoderPtr d = binaryDecoder(); + d->init(*in); + BOOST_CHECK_THROW(d->skipArray(), Exception); + unsetenv("AVRO_MAX_COLLECTION_ITEMS"); +} + } // namespace avro boost::unit_test::test_suite * @@ -179,5 +269,12 @@ init_unit_test_suite(int, char *[]) { ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullNotFalselyRejected)); ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfDeeplyNestedNullNotFalselyRejected)); ts->add(BOOST_TEST_CASE(&avro::testBytesRemainingRightAfterInit)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullRejectsCountAboveDefaultLimit)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfAllNullRecordRejectsHugeCount)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullRejectsNegativeCount)); + ts->add(BOOST_TEST_CASE(&avro::testReadMapOfNullRejectedByAvailableBytes)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullRespectsConfiguredLimit)); + ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfLongRejectedByStructuralCap)); + ts->add(BOOST_TEST_CASE(&avro::testSkipArrayRejectsHugeCount)); return ts; } From 6fad00505d5d45171ff7dc7a7db6c1074b4d29b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 18:10:32 +0200 Subject: [PATCH 09/21] AVRO-4294: [c++] Reject INT64_MIN array/map block count Folds in AVRO-4276. BinaryDecoder::doDecodeItemCount() negated a negative block count with the -(result + 1) + 1 idiom, which avoids undefined behavior but turns INT64_MIN into 2^63; callers (e.g. GenericReader) then attempt an impossible allocation. The zig-zag encoding of INT64_MIN is a valid 10-byte varint, so this is reachable from malformed input. Reject INT64_MIN explicitly before negating, and simplify the negation to -result (safe once INT64_MIN is excluded), matching the C binding. Adds tests for a INT64_MIN block count on both arrayStart() and mapStart(). Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 9 ++++++++- lang/c++/test/CodecTests.cc | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index d6f83035811..36befb35283 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -19,6 +19,7 @@ #include "Decoder.hh" #include "Exception.hh" #include "Zigzag.hh" +#include #include #include @@ -198,8 +199,14 @@ size_t BinaryDecoder::arrayStart() { size_t BinaryDecoder::doDecodeItemCount() { auto result = doDecodeLong(); if (result < 0) { + // INT64_MIN cannot be negated in int64_t (it would overflow); reject it + // rather than propagating 2^63 as an item count that inevitably fails a + // huge allocation downstream. + if (result == INT64_MIN) { + throw Exception("Invalid negative block count: {}", result); + } doDecodeLong(); - return static_cast(-(result + 1)) + 1; + return static_cast(-result); } return static_cast(result); } diff --git a/lang/c++/test/CodecTests.cc b/lang/c++/test/CodecTests.cc index 59aa023dbd1..11231151de7 100644 --- a/lang/c++/test/CodecTests.cc +++ b/lang/c++/test/CodecTests.cc @@ -2145,6 +2145,32 @@ static void testByteCount() { BOOST_CHECK_EQUAL(os1->byteCount(), 3); } +static void testArrayInt64MinBlockCount() { + // INT64_MIN encoded as a zigzag varint is the 10-byte sequence + // FF FF FF FF FF FF FF FF FF 01. As an array block count its negation + // overflows int64_t, so the decoder must reject it rather than return 2^63. + const uint8_t data[] = {0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0x01}; + + InputStreamPtr is = memoryInputStream(data, sizeof(data)); + DecoderPtr d = binaryDecoder(); + d->init(*is); + + BOOST_CHECK_THROW(d->arrayStart(), Exception); +} + +static void testMapInt64MinBlockCount() { + // Same INT64_MIN varint, but as a map block count. + const uint8_t data[] = {0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0x01}; + + InputStreamPtr is = memoryInputStream(data, sizeof(data)); + DecoderPtr d = binaryDecoder(); + d->init(*is); + + BOOST_CHECK_THROW(d->mapStart(), Exception); +} + } // namespace avro boost::unit_test::test_suite * @@ -2161,6 +2187,8 @@ init_unit_test_suite(int, char *[]) { ts->add(BOOST_TEST_CASE(avro::testJsonCodecReinit)); ts->add(BOOST_TEST_CASE(avro::testArrayNegativeBlockCount)); ts->add(BOOST_TEST_CASE(avro::testByteCount)); + ts->add(BOOST_TEST_CASE(avro::testArrayInt64MinBlockCount)); + ts->add(BOOST_TEST_CASE(avro::testMapInt64MinBlockCount)); return ts; } From a91347f37750105fcbc92ba34370665010d8afdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 19:26:24 +0200 Subject: [PATCH 10/21] AVRO-4294: [c++] Always apply structural cap; drop unused vars; cleanups Addresses review feedback: - ensureCollectionAvailable no longer returns early for minBytes <= 0, so the structural/overflow cap now applies to every collection, including zero-byte elements and (crucially) array/map decoding under schema resolution, where minBytes is forced to 0 and the cap was previously skipped entirely. - collectionLimits now returns a CollectionLimits struct, so ensureCollection- Available and ensureZeroByteCollectionWithinLimit no longer declare an unused zeroByte/structural local (which broke -Werror builds as set-but-not-used). - minBytesPerElement's fixed-size clamp uses std::min. - BinaryDecoder.cc: replace the duplicated 2147483639 literal with a named kDefaultMaxCollectionStructural constant and document the AVRO_MAX_COLLECTION_ITEMS override. - Reword the structural-cap comment: C++ never had the Java "historical" limit; the value simply matches the other SDKs. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 14 +++++-- lang/c++/impl/Generic.cc | 72 ++++++++++++++++++---------------- 2 files changed, 49 insertions(+), 37 deletions(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index 36befb35283..928e2de5032 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -27,9 +27,15 @@ namespace avro { using std::make_shared; -// Structural cap on the number of elements to skip in an array or map. Mirrors -// the read-path limit in Generic.cc; AVRO_MAX_COLLECTION_ITEMS, when a -// non-negative integer, overrides it. +// Structural cap on the number of elements to skip in an array or map (an +// overflow / defense-in-depth guard). Mirrors the read-path limit in Generic.cc +// and matches the value used by the other Avro SDKs (Integer.MAX_VALUE - 8). +static constexpr int64_t kDefaultMaxCollectionStructural = 2147483639; + +// Returns the structural collection cap. It can be overridden by the +// AVRO_MAX_COLLECTION_ITEMS environment variable (a non-negative integer), +// matching the read path and the other SDKs so a single knob configures all of +// them; an invalid or unset value uses the default. static int64_t maxCollectionStructural() { const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS"); if (env != nullptr && *env != '\0') { @@ -39,7 +45,7 @@ static int64_t maxCollectionStructural() { return static_cast(value); } } - return 2147483639; + return kDefaultMaxCollectionStructural; } class BinaryDecoder : public Decoder { diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 8d089d7970b..90e7c209972 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -17,6 +17,8 @@ */ #include "Generic.hh" +#include +#include #include #include #include @@ -46,10 +48,9 @@ static int64_t minBytesPerElement(const NodePtr &node, int depth) { case AVRO_FIXED: { // fixedSize() is a size_t; clamp to int64_t so a huge fixed size // cannot wrap negative. - size_t sz = node->fixedSize(); - return sz > static_cast(std::numeric_limits::max()) - ? std::numeric_limits::max() - : static_cast(sz); + return static_cast(std::min( + node->fixedSize(), + static_cast(std::numeric_limits::max()))); } case AVRO_RECORD: { if (depth > 64) { @@ -92,48 +93,54 @@ static int64_t minBytesPerElement(const NodePtr &node, int depth) { static constexpr int64_t kDefaultMaxCollectionItems = 10000000; // Structural cap on the number of elements in any array or map (an overflow / -// defense-in-depth guard), matching the historical Integer.MAX_VALUE - 8 limit. -// Non-zero-byte elements are also bounded by the bytes remaining. +// defense-in-depth guard). It matches the value used by the other Avro SDKs +// (Integer.MAX_VALUE - 8); non-zero-byte elements are also bounded by the bytes +// remaining. static constexpr int64_t kDefaultMaxCollectionStructural = 2147483639; -// AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both limits; -// otherwise zero-byte elements use the tighter default and all collections use -// the structural default. -static void collectionLimits(int64_t &zeroByte, int64_t &structural) { +struct CollectionLimits { + int64_t zeroByte; + int64_t structural; +}; + +// AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, overrides both limits +// to that value; otherwise zero-byte elements use the tighter default and all +// collections use the structural default. +static CollectionLimits collectionLimits() { const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS"); if (env != nullptr && *env != '\0') { char *end = nullptr; long long value = std::strtoll(env, &end, 10); if (*end == '\0' && value >= 0) { - zeroByte = structural = static_cast(value); - return; + return {static_cast(value), static_cast(value)}; } } - zeroByte = kDefaultMaxCollectionItems; - structural = kDefaultMaxCollectionStructural; + return {kDefaultMaxCollectionItems, kDefaultMaxCollectionStructural}; } -// Reject a collection (array or map) block whose declared element count could -// not be backed by the bytes actually remaining, before resizing for it. -// Skipped when the per-element minimum is zero, or when the decoder cannot -// report how many bytes remain. The comparison divides to avoid overflow. +// Reject a collection (array or map) block that could drive an unbounded +// allocation, before resizing for it. A block whose declared element count +// could not be backed by the bytes actually remaining is rejected (only when +// the per-element minimum is positive and the decoder can report the bytes +// remaining); and every collection is bounded by the structural cap, which also +// covers zero-byte elements and decoders that cannot report the bytes remaining. +// The comparisons divide/subtract to avoid overflow. static void ensureCollectionAvailable(Decoder &d, size_t existing, size_t count, int64_t minBytes) { - if (count == 0 || minBytes <= 0) { + if (count == 0) { return; } - int64_t remaining = d.bytesRemaining(); - if (remaining >= 0 && - static_cast(count) > - static_cast(remaining) / static_cast(minBytes)) { - throw Exception( - "Collection claims {} elements with at least {} bytes each, " - "but only {} bytes are available", - count, minBytes, remaining); + if (minBytes > 0) { + int64_t remaining = d.bytesRemaining(); + if (remaining >= 0 && + static_cast(count) > + static_cast(remaining) / static_cast(minBytes)) { + throw Exception( + "Collection claims {} elements with at least {} bytes each, " + "but only {} bytes are available", + count, minBytes, remaining); + } } - // Structural / overflow guard, also covering decoders that cannot report the - // bytes remaining. Compare without adding so existing + count cannot overflow. - int64_t zeroByte, structural; - collectionLimits(zeroByte, structural); + const int64_t structural = collectionLimits().structural; const uint64_t limit = static_cast(structural); if (static_cast(count) > limit || static_cast(existing) > limit - static_cast(count)) { @@ -148,8 +155,7 @@ static void ensureCollectionAvailable(Decoder &d, size_t existing, size_t count, // exceeds the configured limit. These elements consume no input, so they cannot // be bounded by the bytes remaining; the count is the only signal. static void ensureZeroByteCollectionWithinLimit(size_t existing, size_t count) { - int64_t zeroByte, structural; - collectionLimits(zeroByte, structural); + const int64_t zeroByte = collectionLimits().zeroByte; const uint64_t limit = static_cast(zeroByte); // Compare without adding, so existing + count cannot overflow. if (static_cast(count) > limit || From e6a35fa80282dbbbd8d061eb6f491bfc6822e1d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 20:43:04 +0200 Subject: [PATCH 11/21] AVRO-4294: [c++] Reject negative skip block size; tighten resolving caps Addresses review feedback: - skipArray(): reject a negative block byte-size (it would convert to a huge size_t and drive an unbounded skip), and read the structural limit on each call instead of caching it in a function-local static, so a runtime change to AVRO_MAX_COLLECTION_ITEMS is honoured (matching Generic.cc). Adds a test. - Generic.cc: under schema resolution the on-wire element can be zero bytes even when the reader element is not (reader-only fields from defaults). Apply the zero-byte cap conservatively for arrays when resolving, and bound maps by the >= 1 byte key (minBytes = 1) even when resolving, so the up-front resize cannot be driven past the caps. - Stream.hh: subtracting two null pointers is well-defined (yields 0), so drop the unnecessary guard and its incorrect UB comment. - AvailableBytesTests: use an RAII ScopedCollectionLimit that is MSVC-portable (setenv/unsetenv or _putenv_s) and restores any pre-existing env value. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 14 ++++-- lang/c++/impl/Generic.cc | 14 ++++-- lang/c++/include/avro/Stream.hh | 11 ++--- lang/c++/test/AvailableBytesTests.cc | 73 +++++++++++++++++++++++++--- 4 files changed, 93 insertions(+), 19 deletions(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index 928e2de5032..ee92a14d892 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -225,15 +225,21 @@ size_t BinaryDecoder::skipArray() { for (;;) { auto r = doDecodeLong(); if (r < 0) { - auto n = static_cast(doDecodeLong()); - in_.skipBytes(n); + auto byteSize = doDecodeLong(); + if (byteSize < 0) { + // A negative block byte-size would convert to a huge size_t and + // drive an unbounded skip; reject it. + throw Exception("Invalid negative block size: {}", byteSize); + } + in_.skipBytes(static_cast(byteSize)); } else { // Bound the block count: skipping a huge block of zero-byte elements // would otherwise loop unboundedly (a CPU exhaustion) even though it // reads/allocates nothing. The decoder has no element schema here, so // apply the structural cap (AVRO_MAX_COLLECTION_ITEMS, default - // Integer.MAX_VALUE - 8). - static const int64_t structural = maxCollectionStructural(); + // Integer.MAX_VALUE - 8). Read the limit each call so a runtime + // change to the environment variable is honoured, matching Generic.cc. + const int64_t structural = maxCollectionStructural(); if (r > structural) { throw Exception( "Cannot skip a collection of more than {} elements; " diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 90e7c209972..2f7dad1762a 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -263,7 +263,13 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { // resolution the wire (writer) type may be smaller than the datum // (reader) type, which would over-estimate and reject valid data. int64_t trueMin = minBytesPerElement(nn, 0); - bool zeroByte = trueMin == 0; + // Under resolution the on-wire (writer) element can be zero bytes + // even when the reader element is not (e.g. reader-only fields filled + // from defaults), so the bytes check is disabled and we cannot tell + // whether an element is zero-byte on the wire. Apply the tighter + // zero-byte cap conservatively in that case, so the up-front resize + // cannot be driven past it. + bool zeroByte = isResolving || trueMin == 0; int64_t minBytes = isResolving ? 0 : trueMin; for (size_t m = d.arrayStart(); m != 0; m = d.arrayNext()) { ensureCollectionAvailable(d, r.size(), m, minBytes); @@ -290,9 +296,11 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { // Saturate the +1 so a maxed-out value minimum cannot wrap. int64_t valuesMin = minBytesPerElement(nn, 0); // A map entry always includes a >= 1 byte key, so it is never a - // zero-byte element; the bytes check alone bounds it. + // zero-byte element and even under resolution the count is bounded by + // the bytes remaining (each entry consumes at least the key). Use 1 + // when resolving (the value type may differ), else the key + value. int64_t minBytes = isResolving - ? 0 + ? 1 : (valuesMin < std::numeric_limits::max() ? valuesMin + 1 : valuesMin); diff --git a/lang/c++/include/avro/Stream.hh b/lang/c++/include/avro/Stream.hh index 32cb2569f6b..d9fb42bc032 100644 --- a/lang/c++/include/avro/Stream.hh +++ b/lang/c++/include/avro/Stream.hh @@ -370,12 +370,11 @@ struct StreamReader { 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. Guard the pointer - // subtraction: after init()/reset() and before any data is buffered, - // next_ and end_ are both null, and subtracting null pointers is - // undefined behavior. - int64_t buffered = (next_ != nullptr) ? (end_ - next_) : 0; + // Bytes already buffered in this reader, added to what the underlying + // stream still has. When next_ and end_ are both null (right after + // init()/reset(), before any data is buffered), the subtraction is + // well-defined and yields 0. + int64_t buffered = end_ - next_; return buffered + streamRemaining; } diff --git a/lang/c++/test/AvailableBytesTests.cc b/lang/c++/test/AvailableBytesTests.cc index 12a09c6cdb1..566f92370fb 100644 --- a/lang/c++/test/AvailableBytesTests.cc +++ b/lang/c++/test/AvailableBytesTests.cc @@ -41,6 +41,53 @@ namespace avro { +// Portable set/unset of the collection-limit environment variable that restores +// any previous value: setenv/unsetenv are POSIX-only (MSVC uses _putenv_s, where +// an empty value unsets), and unconditionally clearing the variable would leak +// state if it was already set before the test ran. +namespace { +const char *const kCollectionItemsEnv = "AVRO_MAX_COLLECTION_ITEMS"; + +class ScopedCollectionLimit { + std::string previous_; + bool hadPrevious_; + + static void setEnv(const char *value) { +#ifdef _WIN32 + _putenv_s(kCollectionItemsEnv, value); +#else + setenv(kCollectionItemsEnv, value, 1); +#endif + } + + static void unsetEnv() { +#ifdef _WIN32 + _putenv_s(kCollectionItemsEnv, ""); +#else + unsetenv(kCollectionItemsEnv); +#endif + } + +public: + explicit ScopedCollectionLimit(const char *value) { + const char *prev = std::getenv(kCollectionItemsEnv); + hadPrevious_ = prev != nullptr; + if (hadPrevious_) { + previous_ = prev; + } + setEnv(value); + } + + ~ScopedCollectionLimit() { + if (hadPrevious_) { + setEnv(previous_.c_str()); + } else { + unsetEnv(); + } + } +}; +} // namespace + // Reading a bytes value whose declared length far exceeds the tiny backing // buffer must throw instead of attempting a huge allocation. static void testDecodeBytesRejectsOversizedLength() { @@ -216,13 +263,12 @@ static void testReadMapOfNullRejectedByAvailableBytes() { static void testReadArrayOfNullRespectsConfiguredLimit() { ValidSchema s = compileJsonSchemaFromString( "{\"type\":\"array\",\"items\":\"null\"}"); - setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1); + ScopedCollectionLimit limitGuard("1000"); // Within the limit decodes; over the limit and cumulative are rejected. GenericDatum ok = decodeCollectionHeader(s, 1000, true); BOOST_CHECK_EQUAL(ok.value().value().size(), 1000u); BOOST_CHECK_THROW(decodeCollectionHeader(s, 1001, true), Exception); BOOST_CHECK_THROW(decodeLongs(s, {600, 600, 0}), Exception); - unsetenv("AVRO_MAX_COLLECTION_ITEMS"); } // A backed non-zero-byte array that passes the bytes check is still bounded by @@ -230,17 +276,16 @@ static void testReadArrayOfNullRespectsConfiguredLimit() { static void testReadArrayOfLongRejectedByStructuralCap() { ValidSchema s = compileJsonSchemaFromString( "{\"type\":\"array\",\"items\":\"long\"}"); - setenv("AVRO_MAX_COLLECTION_ITEMS", "5", 1); + ScopedCollectionLimit limitGuard("5"); // 10 real longs: block count 10, ten 1-byte longs, end marker. std::vector longs = {10, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0}; BOOST_CHECK_THROW(decodeLongs(s, longs), Exception); - unsetenv("AVRO_MAX_COLLECTION_ITEMS"); } // Skipping a huge array (via the decoder's skipArray) is bounded by the // structural cap. static void testSkipArrayRejectsHugeCount() { - setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1); + ScopedCollectionLimit limitGuard("1000"); std::unique_ptr os = memoryOutputStream(); EncoderPtr e = binaryEncoder(); e->init(*os); @@ -250,7 +295,22 @@ static void testSkipArrayRejectsHugeCount() { DecoderPtr d = binaryDecoder(); d->init(*in); BOOST_CHECK_THROW(d->skipArray(), Exception); - unsetenv("AVRO_MAX_COLLECTION_ITEMS"); +} + +static void testSkipArrayRejectsNegativeBlockSize() { + // A negative block count is followed by a block byte-size; a negative size + // would convert to a huge size_t and drive an unbounded skip, so it must be + // rejected. + std::unique_ptr os = memoryOutputStream(); + EncoderPtr e = binaryEncoder(); + e->init(*os); + e->encodeLong(-3); // negative block count: a byte-size follows + e->encodeLong(-1); // negative block byte-size + e->flush(); + InputStreamPtr in = memoryInputStream(*os); + DecoderPtr d = binaryDecoder(); + d->init(*in); + BOOST_CHECK_THROW(d->skipArray(), Exception); } } // namespace avro @@ -276,5 +336,6 @@ init_unit_test_suite(int, char *[]) { ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfNullRespectsConfiguredLimit)); ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfLongRejectedByStructuralCap)); ts->add(BOOST_TEST_CASE(&avro::testSkipArrayRejectsHugeCount)); + ts->add(BOOST_TEST_CASE(&avro::testSkipArrayRejectsNegativeBlockSize)); return ts; } From 0884d6226348ef35ba7f23ec60564daa00fbf69f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 21:10:29 +0200 Subject: [PATCH 12/21] AVRO-4294: [c++] Reject block count that would truncate on size_t cast Addresses review feedback: doDecodeItemCount() cast the int64 block count directly to size_t. On builds where size_t is narrower than int64_t (e.g. 32-bit), a large count truncates/wraps, which could turn a huge block into a small one and bypass the downstream structural caps. Reject a count that does not fit into size_t before returning (guarded with if constexpr so the check only compiles where size_t is narrower, avoiding a useless-cast/tautological comparison on 64-bit). Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index ee92a14d892..0ab3e5b1e5a 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -21,6 +21,7 @@ #include "Zigzag.hh" #include #include +#include #include namespace avro { @@ -212,7 +213,15 @@ size_t BinaryDecoder::doDecodeItemCount() { throw Exception("Invalid negative block count: {}", result); } doDecodeLong(); - return static_cast(-result); + result = -result; + } + // On builds where size_t is narrower than int64_t (e.g. 32-bit), reject a + // count that would truncate on the cast -- otherwise a huge block could wrap + // to a small one and bypass the downstream structural caps. + if constexpr (sizeof(size_t) < sizeof(int64_t)) { + if (static_cast(result) > std::numeric_limits::max()) { + throw Exception("Block count {} exceeds the maximum supported size", result); + } } return static_cast(result); } From 579f38468266fa611f6f5c07538ffdda6d6b7b23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 22:38:39 +0200 Subject: [PATCH 13/21] AVRO-4294: [c++] Grow collections on demand instead of preallocating readArray/readMap resized the container to the full declared block count up front (r.resize(r.size() + m)) before reading any element. On a stream source the bytes-remaining check cannot bound the count, so a large declared count could allocate a huge vector before the truncated input is detected. Grow the container in bounded steps (kMaxCollectionPrealloc) inside the block loop, reading the elements of each step before growing again. The container still ends up holding every element actually read, but a hostile count now fails within read() after a bounded allocation rather than a giant up-front resize. Applies to both the array and map paths. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/Generic.cc | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 2f7dad1762a..4959ba373cb 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -98,6 +98,15 @@ static constexpr int64_t kDefaultMaxCollectionItems = 10000000; // remaining. static constexpr int64_t kDefaultMaxCollectionStructural = 2147483639; +// Upper bound on how many elements a collection container is grown by in a +// single step while decoding. The container still grows to hold every element +// that is actually read; this only prevents resizing to the full (possibly +// attacker-declared) block count up front, before any element is decoded. That +// matters most for stream sources, where the bytes-remaining check cannot bound +// the declared count against the input, so a single up-front resize could +// allocate a huge container before the truncated stream is detected. +static constexpr size_t kMaxCollectionPrealloc = 1024; + struct CollectionLimits { int64_t zeroByte; int64_t structural; @@ -279,10 +288,17 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { ensureZeroByteCollectionWithinLimit(r.size(), m); } ensureCanGrow(r, m); - r.resize(r.size() + m); - for (; start < r.size(); ++start) { - r[start] = GenericDatum(nn); - read(r[start], d, isResolving); + // Grow on demand in bounded steps rather than resizing to the + // full block count up front, so a huge declared count on a stream + // cannot allocate a giant container before any element is read. + for (size_t remaining = m; remaining != 0;) { + size_t step = std::min(remaining, kMaxCollectionPrealloc); + r.resize(r.size() + step); + for (; start < r.size(); ++start) { + r[start] = GenericDatum(nn); + read(r[start], d, isResolving); + } + remaining -= step; } } } break; @@ -307,11 +323,17 @@ void GenericReader::read(GenericDatum &datum, Decoder &d, bool isResolving) { for (size_t m = d.mapStart(); m != 0; m = d.mapNext()) { ensureCollectionAvailable(d, r.size(), m, minBytes); ensureCanGrow(r, m); - r.resize(r.size() + m); - for (; start < r.size(); ++start) { - d.decodeString(r[start].first); - r[start].second = GenericDatum(nn); - read(r[start].second, d, isResolving); + // Grow on demand in bounded steps rather than resizing to the + // full block count up front (see the array case above). + for (size_t remaining = m; remaining != 0;) { + size_t step = std::min(remaining, kMaxCollectionPrealloc); + r.resize(r.size() + step); + for (; start < r.size(); ++start) { + d.decodeString(r[start].first); + r[start].second = GenericDatum(nn); + read(r[start].second, d, isResolving); + } + remaining -= step; } } } break; From 195707b6bfc6958d129e149b94a7c2444fa60287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 23:35:32 +0200 Subject: [PATCH 14/21] AVRO-4294: [c++] Bounds-check union branch index in selectBranch GenericUnion::selectBranch passed the branch index straight to schema()->leafAt(), which throws std::out_of_range for an out-of-range index (and a negative wire index decodes to a huge size_t). Validate the index against the union's branch count first and throw an Avro Exception, so a malformed union index on read fails with a clear, Avro-specific error. Adds a test for a too-large and a negative union branch index. (Enum is already guarded by GenericEnum::set.) Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/include/avro/GenericDatum.hh | 3 +++ lang/c++/test/AvailableBytesTests.cc | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/lang/c++/include/avro/GenericDatum.hh b/lang/c++/include/avro/GenericDatum.hh index 1b1c3d9af87..fc958f1b56c 100644 --- a/lang/c++/include/avro/GenericDatum.hh +++ b/lang/c++/include/avro/GenericDatum.hh @@ -244,6 +244,9 @@ public: * \param branch The index for the selected branch. */ void selectBranch(size_t branch) { + if (branch >= schema()->leaves()) { + throw Exception("Union branch index out of range: must be less than " + std::to_string(schema()->leaves()) + ", but is " + std::to_string(branch)); + } if (curBranch_ != branch) { datum_ = GenericDatum(schema()->leafAt(branch)); curBranch_ = branch; diff --git a/lang/c++/test/AvailableBytesTests.cc b/lang/c++/test/AvailableBytesTests.cc index 566f92370fb..76e68da1ef8 100644 --- a/lang/c++/test/AvailableBytesTests.cc +++ b/lang/c++/test/AvailableBytesTests.cc @@ -313,6 +313,25 @@ static void testSkipArrayRejectsNegativeBlockSize() { BOOST_CHECK_THROW(d->skipArray(), Exception); } +// A union branch index outside [0, branch count) is malformed and must be +// rejected with an Avro Exception rather than letting leafAt() throw +// std::out_of_range. +static void testReadUnionRejectsOutOfRangeIndex() { + ValidSchema s = compileJsonSchemaFromString("[\"null\",\"long\"]"); + for (int64_t index : {int64_t(5), int64_t(-1)}) { + std::unique_ptr os = memoryOutputStream(); + EncoderPtr e = binaryEncoder(); + e->init(*os); + e->encodeLong(index); // raw branch index; only 2 branches exist + e->flush(); + InputStreamPtr in = memoryInputStream(*os); + DecoderPtr d = binaryDecoder(); + d->init(*in); + GenericDatum datum; + BOOST_CHECK_THROW(GenericReader::read(*d, datum, s), Exception); + } +} + } // namespace avro boost::unit_test::test_suite * @@ -337,5 +356,6 @@ init_unit_test_suite(int, char *[]) { ts->add(BOOST_TEST_CASE(&avro::testReadArrayOfLongRejectedByStructuralCap)); ts->add(BOOST_TEST_CASE(&avro::testSkipArrayRejectsHugeCount)); ts->add(BOOST_TEST_CASE(&avro::testSkipArrayRejectsNegativeBlockSize)); + ts->add(BOOST_TEST_CASE(&avro::testReadUnionRejectsOutOfRangeIndex)); return ts; } From 4f4f8f4de64c69aa1ff2df3e3beec5ee539aa799 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 02:06:05 +0200 Subject: [PATCH 15/21] AVRO-4294: [c++] Validate negative block byte-size; bound skipString/skipBytes Address review feedback: - doDecodeItemCount discarded the block byte-size that follows a negative block count without validation; reject a negative byte-size (as skipArray does) so arrayStart()/mapStart() fail fast on corrupt input. - skipString and skipBytes skipped the declared length without checking it against the bytes remaining. Memory-backed InputStream::skip clamps at EOF rather than throwing, so a truncated/oversized length was silently accepted. Call checkAvailableBytes(len) in both, matching decodeString/decodeBytes. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index 0ab3e5b1e5a..3eae5226e21 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -167,6 +167,7 @@ void BinaryDecoder::decodeString(std::string &value) { void BinaryDecoder::skipString() { size_t len = doDecodeLength(); + checkAvailableBytes(len); in_.skipBytes(len); } @@ -181,6 +182,7 @@ void BinaryDecoder::decodeBytes(std::vector &value) { void BinaryDecoder::skipBytes() { size_t len = doDecodeLength(); + checkAvailableBytes(len); in_.skipBytes(len); } @@ -212,7 +214,13 @@ size_t BinaryDecoder::doDecodeItemCount() { if (result == INT64_MIN) { throw Exception("Invalid negative block count: {}", result); } - doDecodeLong(); + int64_t blockSize = doDecodeLong(); + if (blockSize < 0) { + // The byte-size that follows a negative block count is a byte count + // and must be non-negative; reject malformed input here too (as + // skipArray already does) so arrayStart()/mapStart() fail fast. + throw Exception("Invalid negative block byte-size: {}", blockSize); + } result = -result; } // On builds where size_t is narrower than int64_t (e.g. 32-bit), reject a From 3b300c144ff3a6f5d627db12ca2de8ddb31f254d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 02:37:09 +0200 Subject: [PATCH 16/21] AVRO-4294: [c++] Guard skipArray block count against size_t truncation skipArray cast the decoded block count directly to size_t. On builds where size_t is narrower than int64_t (e.g. 32-bit), a count above SIZE_MAX (possible with a large AVRO_MAX_COLLECTION_ITEMS override) could truncate and desynchronize decoding. Apply the same size_t-fit guard doDecodeItemCount uses before casting. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index 3eae5226e21..05c7b3aac66 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -263,6 +263,14 @@ size_t BinaryDecoder::skipArray() { "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate", structural); } + // On builds where size_t is narrower than int64_t (e.g. 32-bit), + // reject a count that would truncate on the cast, matching + // doDecodeItemCount so a huge block can't wrap to a small one. + if constexpr (sizeof(size_t) < sizeof(int64_t)) { + if (static_cast(r) > std::numeric_limits::max()) { + throw Exception("Block count {} exceeds the maximum supported size", r); + } + } return static_cast(r); } } From 8f81bd46bad711877e989fafd9ac6532d098e4ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 02:53:52 +0200 Subject: [PATCH 17/21] AVRO-4294: [c++] Guard skipArray byte-size against truncation and EOF The negative-block path in skipArray cast the block byte-size to size_t without a fit check (32-bit truncation) and skipped it without validating against the bytes remaining, so a memory-backed skip() would clamp at EOF and silently accept a truncated block. Add the size_t-fit guard and checkAvailableBytes() before skipping. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index 05c7b3aac66..af7dcb34734 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -248,6 +248,15 @@ size_t BinaryDecoder::skipArray() { // drive an unbounded skip; reject it. throw Exception("Invalid negative block size: {}", byteSize); } + // Reject a byte-size that would truncate on the cast (32-bit builds) + // and one that exceeds the bytes remaining, so a truncated block is + // not silently skipped past EOF by the memory-backed skip(). + if constexpr (sizeof(size_t) < sizeof(int64_t)) { + if (static_cast(byteSize) > std::numeric_limits::max()) { + throw Exception("Block size {} exceeds the maximum supported size", byteSize); + } + } + checkAvailableBytes(static_cast(byteSize)); in_.skipBytes(static_cast(byteSize)); } else { // Bound the block count: skipping a huge block of zero-byte elements From 6b3e5b1b28f6aba8d7d7cc94d0e9125a55d431e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 03:17:03 +0200 Subject: [PATCH 18/21] AVRO-4294: [c++] Reject overflowing AVRO_MAX_COLLECTION_ITEMS values collectionLimits() and maxCollectionStructural() parsed the env var with std::strtoll without checking errno, so an overflowing value (e.g. many digits) saturated to LLONG_MAX and was accepted, effectively disabling the structural/ zero-byte caps. Check errno == ERANGE (via ) and fall back to the defaults on overflow. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 6 +++++- lang/c++/impl/Generic.cc | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index af7dcb34734..ff8ee535488 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -21,6 +21,7 @@ #include "Zigzag.hh" #include #include +#include #include #include @@ -41,8 +42,11 @@ static int64_t maxCollectionStructural() { const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS"); if (env != nullptr && *env != '\0') { char *end = nullptr; + errno = 0; long long value = std::strtoll(env, &end, 10); - if (*end == '\0' && value >= 0) { + // Ignore an overflowing value (errno == ERANGE) rather than accepting a + // saturated LLONG_MAX, which would effectively remove the structural cap. + if (errno == 0 && *end == '\0' && value >= 0) { return static_cast(value); } } diff --git a/lang/c++/impl/Generic.cc b/lang/c++/impl/Generic.cc index 4959ba373cb..9fe9f29b6ba 100644 --- a/lang/c++/impl/Generic.cc +++ b/lang/c++/impl/Generic.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -119,8 +120,11 @@ static CollectionLimits collectionLimits() { const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS"); if (env != nullptr && *env != '\0') { char *end = nullptr; + errno = 0; long long value = std::strtoll(env, &end, 10); - if (*end == '\0' && value >= 0) { + // Ignore an overflowing value (errno == ERANGE) rather than accepting a + // saturated LLONG_MAX, which would effectively disable the caps. + if (errno == 0 && *end == '\0' && value >= 0) { return {static_cast(value), static_cast(value)}; } } From 11c42af555c16342c6f4a19660ca666977da71fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 06:51:14 +0200 Subject: [PATCH 19/21] AVRO-4294: [c++] Validate enum/union index at decode; signed remaining subtraction - decodeUnionIndex/decodeEnum now go through a shared decodeIndex() helper that rejects a negative decoded long (which would wrap to a huge size_t) and a value beyond size_t (which could truncate into a spuriously in-range branch on builds where size_t is narrower than int64_t) before casting. The concrete bound against the enum/union size is still enforced by the caller. - MemoryInputStream2::remainingBytes() subtracts in int64_t so a violated invariant (curLen_ > size_) cannot underflow an unsigned subtraction into a huge positive remaining count. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/impl/BinaryDecoder.cc | 22 ++++++++++++++++++++-- lang/c++/impl/Stream.cc | 5 ++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lang/c++/impl/BinaryDecoder.cc b/lang/c++/impl/BinaryDecoder.cc index ff8ee535488..564853fb62e 100644 --- a/lang/c++/impl/BinaryDecoder.cc +++ b/lang/c++/impl/BinaryDecoder.cc @@ -79,6 +79,7 @@ class BinaryDecoder : public Decoder { size_t decodeUnionIndex() final; int64_t doDecodeLong(); + size_t decodeIndex(); size_t doDecodeItemCount(); size_t doDecodeLength(); void checkAvailableBytes(size_t len); @@ -202,7 +203,7 @@ void BinaryDecoder::skipFixed(size_t n) { } size_t BinaryDecoder::decodeEnum() { - return static_cast(doDecodeLong()); + return decodeIndex(); } size_t BinaryDecoder::arrayStart() { @@ -302,7 +303,7 @@ size_t BinaryDecoder::skipMap() { } size_t BinaryDecoder::decodeUnionIndex() { - return static_cast(doDecodeLong()); + return decodeIndex(); } int64_t BinaryDecoder::doDecodeLong() { @@ -321,4 +322,21 @@ int64_t BinaryDecoder::doDecodeLong() { return decodeZigzag64(encoded); } +// Decode an enum ordinal or union branch index. Both are wire longs that the +// callers cast to size_t; validate the decoded value here so a negative index +// cannot wrap to a huge size_t and a value beyond size_t (possible where size_t +// is narrower than int64_t, e.g. 32-bit builds) cannot truncate into a +// spuriously in-range branch. The concrete bound (against the enum/union size) +// is still enforced by the caller. +size_t BinaryDecoder::decodeIndex() { + int64_t index = doDecodeLong(); + if (index < 0) { + throw Exception("Invalid negative index: {}", index); + } + if (static_cast(index) > std::numeric_limits::max()) { + throw Exception("Index {} is out of range", index); + } + return static_cast(index); +} + } // namespace avro diff --git a/lang/c++/impl/Stream.cc b/lang/c++/impl/Stream.cc index f6140479f4d..a58d5a37894 100644 --- a/lang/c++/impl/Stream.cc +++ b/lang/c++/impl/Stream.cc @@ -132,7 +132,10 @@ class MemoryInputStream2 : public InputStream { } int64_t remainingBytes() const final { - return static_cast(size_ - curLen_); + // Subtract in int64_t: if an invariant were ever violated (curLen_ > + // size_), doing the subtraction in unsigned size_t would underflow to a + // huge value and weaken the available-bytes guard. + return static_cast(size_) - static_cast(curLen_); } }; From 1f7ac839e7cc4ccbd58ebd1564cdaf7401e21d79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 07:23:57 +0200 Subject: [PATCH 20/21] AVRO-4294: [c++] Avoid null-pointer subtraction in StreamReader::remainingBytes end_ and next_ can both be null right after init()/reset(); subtracting two null pointers is undefined behavior (the previous comment wrongly claimed it was well-defined). Guard the equal-pointer case (which covers both-null) and only subtract real buffer pointers. Pointer equality, unlike subtraction, is well-defined for null pointers. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/include/avro/Stream.hh | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lang/c++/include/avro/Stream.hh b/lang/c++/include/avro/Stream.hh index d9fb42bc032..d014fc621e7 100644 --- a/lang/c++/include/avro/Stream.hh +++ b/lang/c++/include/avro/Stream.hh @@ -371,10 +371,12 @@ struct StreamReader { return -1; } // Bytes already buffered in this reader, added to what the underlying - // stream still has. When next_ and end_ are both null (right after - // init()/reset(), before any data is buffered), the subtraction is - // well-defined and yields 0. - int64_t buffered = end_ - next_; + // stream still has. next_ and end_ can both be null right after + // init()/reset() (before any data is buffered); subtracting two null + // pointers is undefined behavior, so treat equal pointers (including the + // both-null case) as zero buffered and only subtract real buffer + // pointers. Pointer equality, unlike subtraction, is well-defined here. + int64_t buffered = (next_ == end_) ? 0 : (end_ - next_); return buffered + streamRemaining; } From a91982fa24aff8df00c983c7e92658a065622553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 07:32:09 +0200 Subject: [PATCH 21/21] AVRO-4294: [c++] Treat any null buffer pointer as zero buffered Harden StreamReader::remainingBytes() further: if either next_ or end_ is null (post-reset or a partial fill), evaluating end_ - next_ is still UB. Guard on either pointer being null and only subtract when both point into a real buffer. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/c++/include/avro/Stream.hh | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lang/c++/include/avro/Stream.hh b/lang/c++/include/avro/Stream.hh index d014fc621e7..0dd63ee3da0 100644 --- a/lang/c++/include/avro/Stream.hh +++ b/lang/c++/include/avro/Stream.hh @@ -371,12 +371,11 @@ struct StreamReader { return -1; } // Bytes already buffered in this reader, added to what the underlying - // stream still has. next_ and end_ can both be null right after - // init()/reset() (before any data is buffered); subtracting two null - // pointers is undefined behavior, so treat equal pointers (including the - // both-null case) as zero buffered and only subtract real buffer - // pointers. Pointer equality, unlike subtraction, is well-defined here. - int64_t buffered = (next_ == end_) ? 0 : (end_ - next_); + // stream still has. Either pointer can be null right after init()/reset() + // (or a partial fill); subtracting when either operand is null is + // undefined behavior, so treat any null pointer as zero buffered and only + // subtract when both point into a real buffer. + int64_t buffered = (next_ == nullptr || end_ == nullptr) ? 0 : (end_ - next_); return buffered + streamRemaining; }