iemejia commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3566776303
##########
lang/c++/impl/Generic.cc:
##########
@@ -25,6 +26,86 @@ 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) {
+ 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;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ 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<size_t>(std::numeric_limits<int64_t>::max())
Review Comment:
Fixed in a91347f377: the fixed-size clamp now reads `return
std::min<uint64_t>(node->fixedSize(), INT64_MAX)` cast to int64_t.
##########
lang/c++/impl/Generic.cc:
##########
@@ -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);
Review Comment:
It is nearly always unreachable now that the structural cap (<= 2147483639)
is always applied, so I kept it only as a cheap guard for the edge where
`max_size()` is smaller than that cap (32-bit builds, or very large element
types), where `resize()` would otherwise throw a raw std::length_error instead
of a clean Avro Exception. Happy to drop it if you'd prefer to rely on
resize()'s own throw.
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -19,12 +19,28 @@
#include "Decoder.hh"
#include "Exception.hh"
#include "Zigzag.hh"
+#include <cstdlib>
#include <memory>
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");
Review Comment:
Documented in a91347f377 (the env var and its default). It's read
consistently across all Avro SDKs so a single knob configures them, which is
why it's an env var rather than decoder state. A programmatic per-decoder
setter would be a reasonable follow-up if you'd like one — happy to file it.
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -19,12 +19,28 @@
#include "Decoder.hh"
#include "Exception.hh"
#include "Zigzag.hh"
+#include <cstdlib>
#include <memory>
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<int64_t>(value);
+ }
+ }
+ return 2147483639;
Review Comment:
Fixed in a91347f377: replaced the duplicated literal with a named
`kDefaultMaxCollectionStructural` constant.
##########
lang/c++/impl/Generic.cc:
##########
@@ -25,6 +27,151 @@ 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) {
+ return 0;
+ }
+ switch (node->type()) {
+ case AVRO_NULL:
+ return 0;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ 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<size_t>(std::numeric_limits<int64_t>::max())
+ ? std::numeric_limits<int64_t>::max()
+ : static_cast<int64_t>(sz);
+ }
+ case AVRO_RECORD: {
+ if (depth > 64) {
+ // 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) {
+ 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<int64_t>::max() - total) {
+ return std::numeric_limits<int64_t>::max();
+ }
+ total += fieldMin;
+ }
+ return total;
+ }
+ default:
+ // boolean, int, long, bytes, string, enum, union, array, map and
+ // symbolic references all encode to at least one byte.
+ return 1;
+ }
+}
+
+// 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.
Review Comment:
Fixed in a91347f377: reworded — C++ never had that Java limit; the comment
now just says the value matches the one used by the other SDKs.
##########
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; }
Review Comment:
Acknowledged. They're two different interfaces (`Decoder::bytesRemaining()`
vs `InputStream`/`StreamReader::remainingBytes()`), which is why the names
diverged. Renaming either is a public-API change, so I left it for now; I can
align them (deprecating the old name) in a follow-up if you'd like.
##########
lang/c++/impl/Generic.cc:
##########
@@ -25,6 +27,151 @@ 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) {
+ return 0;
+ }
+ switch (node->type()) {
+ case AVRO_NULL:
+ return 0;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ 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<size_t>(std::numeric_limits<int64_t>::max())
+ ? std::numeric_limits<int64_t>::max()
+ : static_cast<int64_t>(sz);
+ }
+ case AVRO_RECORD: {
+ if (depth > 64) {
+ // 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) {
+ 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<int64_t>::max() - total) {
+ return std::numeric_limits<int64_t>::max();
+ }
+ total += fieldMin;
+ }
+ return total;
+ }
+ default:
+ // boolean, int, long, bytes, string, enum, union, array, map and
+ // symbolic references all encode to at least one byte.
+ return 1;
+ }
+}
+
+// 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<int64_t>(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 existing, size_t
count, int64_t minBytes) {
+ if (count == 0 || minBytes <= 0) {
+ return;
+ }
+ int64_t remaining = d.bytesRemaining();
+ if (remaining >= 0 &&
+ static_cast<uint64_t>(count) >
+ static_cast<uint64_t>(remaining) /
static_cast<uint64_t>(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 uint64_t limit = static_cast<uint64_t>(structural);
+ if (static_cast<uint64_t>(count) > limit ||
+ static_cast<uint64_t>(existing) > limit -
static_cast<uint64_t>(count)) {
+ throw Exception(
+ "Cannot read a collection of more than {} elements; "
+ "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate",
+ structural);
+ }
Review Comment:
Fixed in a91347f377: ensureCollectionAvailable no longer returns early for
minBytes<=0, so the structural cap now applies to every collection (including
array/map under schema resolution, where minBytes is forced to 0).
collectionLimits() also now returns a struct, so the unused
`zeroByte`/`structural` locals (the -Werror set-but-not-used) are gone.
##########
lang/c++/impl/Generic.cc:
##########
@@ -25,6 +27,151 @@ 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) {
+ return 0;
+ }
+ switch (node->type()) {
+ case AVRO_NULL:
+ return 0;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ 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<size_t>(std::numeric_limits<int64_t>::max())
+ ? std::numeric_limits<int64_t>::max()
+ : static_cast<int64_t>(sz);
+ }
+ case AVRO_RECORD: {
+ if (depth > 64) {
+ // 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) {
+ 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<int64_t>::max() - total) {
+ return std::numeric_limits<int64_t>::max();
+ }
+ total += fieldMin;
+ }
+ return total;
+ }
+ default:
+ // boolean, int, long, bytes, string, enum, union, array, map and
+ // symbolic references all encode to at least one byte.
+ return 1;
+ }
+}
+
+// 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<int64_t>(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 existing, size_t
count, int64_t minBytes) {
+ if (count == 0 || minBytes <= 0) {
+ return;
+ }
+ int64_t remaining = d.bytesRemaining();
+ if (remaining >= 0 &&
+ static_cast<uint64_t>(count) >
+ static_cast<uint64_t>(remaining) /
static_cast<uint64_t>(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 uint64_t limit = static_cast<uint64_t>(structural);
+ if (static_cast<uint64_t>(count) > limit ||
+ static_cast<uint64_t>(existing) > limit -
static_cast<uint64_t>(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<uint64_t>(zeroByte);
Review Comment:
Fixed in a91347f377: ensureZeroByteCollectionWithinLimit reads only
`collectionLimits().zeroByte` now, so there is no unused `structural` local.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
