iemejia commented on code in PR #3849:
URL: https://github.com/apache/avro/pull/3849#discussion_r3564261249
##########
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:
Fixed in be1e1803d9. `maxCollectionItems()` now rejects any
non-decimal-digit character (so a leading `-`/`+` that `strtoull` would accept
no longer disables the limit) and falls back to the default if the parsed value
exceeds `size_t` (guarding truncation on 32-bit builds).
##########
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:
Fixed in be1e1803d9. Added an RAII `CollectionLimitGuard` that saves and
restores (or unsets) `AVRO_MAX_COLLECTION_ITEMS` on scope exit, so the tests no
longer leak state into one another or the rest of the process.
##########
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:
Done in be1e1803d9 — both `testArrayBlockCountIsBounded` and
`testMapBlockCountIsBounded` now construct a `CollectionLimitGuard` at the top
of the test.
##########
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:
Fixed in be1e1803d9. `testMapBlockCountIsBounded` now covers cumulative (two
blocks of 6 pairs, all keyed "a"), negative, and within-limit block counts,
mirroring the array test rather than adjusting the description.
--
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]
