Gerrit0 commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3566478995
##########
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:
This should use the named constant rather than duplicating an arbitrary
number
##########
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:
This would read better as a simple `return std::min(...)`
##########
lang/c++/include/avro/Stream.hh:
##########
@@ -345,6 +356,29 @@ 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. 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.
Review Comment:
This is just... not true?
> [N4950](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/n4950.pdf)
> 5 When two pointer expressions P and Q are subtracted, the type of the
result is an implementation-defined signed integral type; this type shall be
the same type that is defined as std::ptrdiff_t in the <cstddef> header
(17.2.4).
> (5.1) If P and Q both evaluate to null pointer values, the result is 0.
##########
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:
I am *extremely* suspicious of the claim that ensureCanGrow can ever fail.
Is there some platform I am not considering where max_size limits would be
exceeded before RAM is exhausted?
C++ reference implies this won't ever happen,
https://en.cppreference.com/cpp/container/vector/max_size
##########
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:
It bothers me that `bytesRemaining` and `remainingBytes` are both used for
this. They seem to be defining the exact same concept.
##########
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:
This is a rather confusing claim for C++, where that limit has never existed.
##########
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:
I really don't like this. Libraries should not grab environment variables
for configuration which developers of the library might not even know exist. If
this option is important, I should be able to set it somehow on the decoder
itself.
If this is going to stay, it at the very least needs to be properly
documented.
--
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]
