Copilot commented on code in PR #3849:
URL: https://github.com/apache/avro/pull/3849#discussion_r3564160930
##########
lang/c++/impl/Generic.cc:
##########
@@ -27,6 +29,41 @@ using std::vector;
typedef vector<uint8_t> bytes;
+namespace {
+
+// The block count of an array or map is read from the (potentially untrusted
or
+// truncated) input and drives allocation of the resulting collection. To guard
+// against unbounded memory allocation from a very large or malformed block
+// count, the number of items in a single decoded array or map is capped. This
+// mirrors the Java SDK's collection item limit. The default can be overridden
+// with the AVRO_MAX_COLLECTION_ITEMS environment variable.
+constexpr size_t DEFAULT_MAX_COLLECTION_ITEMS = (size_t(1) << 31) - 8; // 2^31
- 8
+
+size_t maxCollectionItems() {
+ const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS");
+ if (env != nullptr && *env != '\0') {
+ errno = 0;
+ char *end = nullptr;
+ unsigned long long value = std::strtoull(env, &end, 10);
+ if (errno == 0 && end != nullptr && *end == '\0' && value > 0) {
+ return static_cast<size_t>(value);
+ }
+ }
+ return DEFAULT_MAX_COLLECTION_ITEMS;
+}
Review Comment:
`maxCollectionItems()` currently accepts non-digit inputs like "-1" because
`strtoull` allows an optional sign; that would effectively disable the limit.
It also `static_cast`s `unsigned long long` to `size_t` without an overflow
check (e.g., truncation on 32-bit builds). Consider rejecting any
non-decimal-digit characters and falling back to the default when the parsed
value exceeds `size_t`'s max.
##########
lang/c++/test/CodecTests.cc:
##########
@@ -2145,6 +2146,73 @@ static void testByteCount() {
BOOST_CHECK_EQUAL(os1->byteCount(), 3);
}
+// AVRO-4278: the block count of an array or map is read from the input and
+// drives allocation of the resulting collection in GenericReader::read(). A
+// very large or malformed block count must be rejected rather than eagerly
+// resizing the collection. These tests cap the limit via the
+// AVRO_MAX_COLLECTION_ITEMS environment variable.
+
+static GenericDatum decodeNullCollection(const char *schemaJson,
+ const uint8_t *data, size_t len) {
+ ValidSchema vs = parsing::makeValidSchema(schemaJson);
+ InputStreamPtr is = memoryInputStream(data, len);
+ DecoderPtr d = binaryDecoder();
+ d->init(*is);
+ GenericDatum datum(vs);
+ GenericReader::read(*d, datum);
+ return datum;
+}
+
+static void setCollectionLimit(const char *value) {
+#ifdef _WIN32
+ _putenv_s("AVRO_MAX_COLLECTION_ITEMS", value);
+#else
+ setenv("AVRO_MAX_COLLECTION_ITEMS", value, 1);
+#endif
+}
+
+static void testArrayBlockCountIsBounded() {
+ setCollectionLimit("10");
+ const char *schema = R"({"type": "array", "items": "null"})";
Review Comment:
After introducing a scoped environment guard, use it here so the limit is
restored even if the test exits early.
##########
lang/c++/test/CodecTests.cc:
##########
@@ -2145,6 +2146,73 @@ static void testByteCount() {
BOOST_CHECK_EQUAL(os1->byteCount(), 3);
}
+// AVRO-4278: the block count of an array or map is read from the input and
+// drives allocation of the resulting collection in GenericReader::read(). A
+// very large or malformed block count must be rejected rather than eagerly
+// resizing the collection. These tests cap the limit via the
+// AVRO_MAX_COLLECTION_ITEMS environment variable.
+
+static GenericDatum decodeNullCollection(const char *schemaJson,
+ const uint8_t *data, size_t len) {
+ ValidSchema vs = parsing::makeValidSchema(schemaJson);
+ InputStreamPtr is = memoryInputStream(data, len);
+ DecoderPtr d = binaryDecoder();
+ d->init(*is);
+ GenericDatum datum(vs);
+ GenericReader::read(*d, datum);
+ return datum;
+}
+
+static void setCollectionLimit(const char *value) {
+#ifdef _WIN32
+ _putenv_s("AVRO_MAX_COLLECTION_ITEMS", value);
+#else
+ setenv("AVRO_MAX_COLLECTION_ITEMS", value, 1);
+#endif
+}
Review Comment:
These tests set `AVRO_MAX_COLLECTION_ITEMS` but never restore the prior
value. That can make the suite order-dependent or interfere with other tests in
the same process. Consider using an RAII guard that restores/unsets the
environment variable on scope exit.
##########
lang/c++/test/CodecTests.cc:
##########
@@ -2145,6 +2146,73 @@ static void testByteCount() {
BOOST_CHECK_EQUAL(os1->byteCount(), 3);
}
+// AVRO-4278: the block count of an array or map is read from the input and
+// drives allocation of the resulting collection in GenericReader::read(). A
+// very large or malformed block count must be rejected rather than eagerly
+// resizing the collection. These tests cap the limit via the
+// AVRO_MAX_COLLECTION_ITEMS environment variable.
+
+static GenericDatum decodeNullCollection(const char *schemaJson,
+ const uint8_t *data, size_t len) {
+ ValidSchema vs = parsing::makeValidSchema(schemaJson);
+ InputStreamPtr is = memoryInputStream(data, len);
+ DecoderPtr d = binaryDecoder();
+ d->init(*is);
+ GenericDatum datum(vs);
+ GenericReader::read(*d, datum);
+ return datum;
+}
+
+static void setCollectionLimit(const char *value) {
+#ifdef _WIN32
+ _putenv_s("AVRO_MAX_COLLECTION_ITEMS", value);
+#else
+ setenv("AVRO_MAX_COLLECTION_ITEMS", value, 1);
+#endif
+}
+
+static void testArrayBlockCountIsBounded() {
+ setCollectionLimit("10");
+ const char *schema = R"({"type": "array", "items": "null"})";
+
+ // A single block declaring 11 items (zigzag(11) = 0x16) exceeds the limit
+ // of 10 and must be rejected before the vector is resized. The null items
+ // occupy no bytes, so a tiny input could otherwise drive a huge
allocation.
+ const uint8_t overLimit[] = {0x16, 0x00};
+ BOOST_CHECK_THROW(decodeNullCollection(schema, overLimit,
sizeof(overLimit)),
+ Exception);
+
+ // Two blocks of 6 items (zigzag(6) = 0x0c) exceed the limit cumulatively
+ // even though neither block does on its own.
+ const uint8_t cumulative[] = {0x0c, 0x0c};
+ BOOST_CHECK_THROW(decodeNullCollection(schema, cumulative,
sizeof(cumulative)),
+ Exception);
+
+ // A negative count (zigzag(-11) = 0x15) uses its absolute value (11) as
the
+ // item count and is followed by a block byte-size (0x00). It must still be
+ // bounded.
+ const uint8_t negative[] = {0x15, 0x00};
+ BOOST_CHECK_THROW(decodeNullCollection(schema, negative, sizeof(negative)),
+ Exception);
+
+ // Three items (zigzag(3) = 0x06) then the end-of-array marker (0x00) is
+ // within the limit and decodes successfully.
+ const uint8_t withinLimit[] = {0x06, 0x00};
+ GenericDatum datum =
+ decodeNullCollection(schema, withinLimit, sizeof(withinLimit));
+ BOOST_CHECK_EQUAL(datum.value<GenericArray>().value().size(), 3u);
+}
+
+static void testMapBlockCountIsBounded() {
+ setCollectionLimit("10");
+ const char *schema = R"({"type": "map", "values": "null"})";
+
+ // Block count 11 (zigzag = 0x16) exceeds the limit of 10.
+ const uint8_t overLimit[] = {0x16, 0x00};
+ BOOST_CHECK_THROW(decodeNullCollection(schema, overLimit,
sizeof(overLimit)),
+ Exception);
+}
Review Comment:
PR description says `testMapBlockCountIsBounded` covers cumulative and
negative block counts and a within-limit decode, but the current test only
checks a single over-limit block. Either expand this test to cover the
described cases (mirroring the array test) or adjust the PR description
accordingly.
--
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]
