iemejia commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3568112143
##########
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));
+ }
Review Comment:
Fixed — decodeUnionIndex now validates the decoded long via decodeIndex(): a
negative index is rejected before the size_t cast (so it can't wrap to a huge
value), and selectBranch's bound against leaves() still applies.
##########
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));
+ }
Review Comment:
Fixed — decodeIndex() also rejects an index beyond
std::numeric_limits<size_t>::max(), so on a 32-bit build a large on-wire union
index (e.g. 2^32) can't truncate into a small in-range value; it's rejected
before reaching selectBranch.
##########
lang/c++/impl/Stream.cc:
##########
@@ -119,6 +130,10 @@ class MemoryInputStream2 : public InputStream {
size_t byteCount() const final {
return curLen_;
}
+
+ int64_t remainingBytes() const final {
+ return static_cast<int64_t>(size_ - curLen_);
+ }
Review Comment:
Fixed — MemoryInputStream2::remainingBytes() now computes `(int64_t)size_ -
(int64_t)curLen_`, so a violated invariant can't underflow an unsigned
subtraction.
##########
lang/c++/impl/Generic.cc:
##########
@@ -25,6 +29,164 @@ 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.
+ return static_cast<int64_t>(std::min<uint64_t>(
+ node->fixedSize(),
+ static_cast<uint64_t>(std::numeric_limits<int64_t>::max())));
+ }
+ 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). 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;
+
+// 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;
+};
+
+// 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) {
+ return {static_cast<int64_t>(value), static_cast<int64_t>(value)};
+ }
+ }
+ return {kDefaultMaxCollectionItems, kDefaultMaxCollectionStructural};
Review Comment:
Already handled — collectionLimits() resets errno before strtoll and only
accepts the value when `errno == 0`, so an ERANGE overflow (saturated
LLONG_MAX) is ignored and the defaults apply (Generic.cc:122-127).
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -19,12 +19,36 @@
#include "Decoder.hh"
#include "Exception.hh"
#include "Zigzag.hh"
+#include <cstdint>
+#include <cstdlib>
+#include <limits>
#include <memory>
namespace avro {
using std::make_shared;
+// 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') {
+ char *end = nullptr;
+ long long value = std::strtoll(env, &end, 10);
+ if (*end == '\0' && value >= 0) {
+ return static_cast<int64_t>(value);
+ }
+ }
+ return kDefaultMaxCollectionStructural;
+}
Review Comment:
Already handled — maxCollectionStructural() has the same `errno == 0` guard,
so an overflowing AVRO_MAX_COLLECTION_ITEMS falls back to the default
structural cap (BinaryDecoder.cc).
--
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]
