Copilot commented on code in PR #3499:
URL: https://github.com/apache/brpc/pull/3499#discussion_r3879465275
##########
test/brpc_mcpack2pb_unittest.cpp:
##########
@@ -156,4 +157,134 @@ TEST(Mcpack2pbParserTest,
ArrayItemCountIsZeroWhenPayloadSmallerThanHeader) {
EXPECT_EQ(0u, it.item_count());
}
+// Builds the wire bytes of a recursive message `Node { repeated Node
+// children = 1; }' nesting `depth' levels of { children: [ ... ] }, the way
+// protoc-gen-mcpack serializes such a message.
+static void AppendU32(std::string* out, uint32_t value) {
+ char buf[4];
+ buf[0] = (char)(value & 0xff);
+ buf[1] = (char)((value >> 8) & 0xff);
+ buf[2] = (char)((value >> 16) & 0xff);
+ buf[3] = (char)((value >> 24) & 0xff);
+ out->append(buf, 4);
+}
+
+static std::string BuildRecursivePayload(int depth) {
+ // The innermost level is an empty object: an ItemsHead with no items.
+ std::string body;
+ AppendU32(&body, 0);
+ const std::string name = std::string("children\0", 9); // trailing '\0'
+ for (int i = 1; i < depth; ++i) {
+ std::string item; // a FIELD_OBJECT item wrapping the inner payload
+ item.push_back(0x10); // FIELD_OBJECT
+ item.push_back(0x00); // name_size = 0
+ AppendU32(&item, (uint32_t)body.size()); // value_size
+ item.append(body);
+
+ std::string arr; // an array holding a single item
+ AppendU32(&arr, 1);
+ arr.append(item);
+
+ std::string child; // FIELD_ARRAY "children"
+ child.push_back(0x20); // FIELD_ARRAY
+ child.push_back((char)name.size()); // name_size
+ AppendU32(&child, (uint32_t)arr.size()); // value_size
+ child.append(name);
+ child.append(arr);
+
+ std::string new_body; // an object holding a single field
+ AppendU32(&new_body, 1);
+ new_body.append(child);
+ body.swap(new_body);
+ }
+ return body;
+}
+
+// Simulates the recursion pattern of the functions generated by
+// protoc-gen-mcpack for a message with a repeated message field
+// (e.g. Node.children): parse_<msg>_body_internal creates an ObjectIterator,
+// set_<msg>_<field> creates an ArrayIterator and calls
+// parse_<msg>_body_internal for each item.
+static bool ParseNodeInternal(mcpack2pb::UnparsedValue& value) {
+ mcpack2pb::ObjectIterator it(value);
+ for (; it != nullptr; ++it) {
+ if (it->name == "children") {
+ if (it->value.type() != mcpack2pb::FIELD_ARRAY) {
+ return false;
+ }
+ mcpack2pb::ArrayIterator it2(it->value);
+ for (; it2 != nullptr; ++it2) {
+ if (it2->type() != mcpack2pb::FIELD_OBJECT ||
+ !ParseNodeInternal(*it2)) {
+ return false;
+ }
+ }
+ }
+ }
+ return value.stream()->good();
+}
+
+struct ParseArgs {
+ const std::string* payload;
+ bool parse_ok;
+};
+
+static void* ParseOnSmallStack(void* arg) {
+ ParseArgs* args = static_cast<ParseArgs*>(arg);
+ butil::IOBuf buf;
+ buf.append(args->payload->data(), args->payload->size());
+ butil::IOBufAsZeroCopyInputStream zc_stream(buf);
+ mcpack2pb::InputStream stream(&zc_stream);
+ mcpack2pb::UnparsedValue value(mcpack2pb::FIELD_OBJECT, &stream,
+ buf.size());
+ args->parse_ok = ParseNodeInternal(value);
+ return nullptr;
+}
+
+// Parses `payload' on a 1 MB stack thread, the size of a NORMAL bthread
+// stack in brpc, so that the test behaves like a request served by brpc.
+static int ParseRecursivePayloadWith1MBStack(const std::string& payload,
+ bool* ok) {
+ ParseArgs args = { &payload, false };
+ pthread_attr_t attr;
+ if (pthread_attr_init(&attr) != 0) {
+ return -1;
+ }
+ if (pthread_attr_setstacksize(&attr, 1024 * 1024) != 0) {
+ return -2;
+ }
+ pthread_t tid;
+ const int rc = pthread_create(&tid, &attr, ParseOnSmallStack, &args);
+ pthread_attr_destroy(&attr);
+ if (rc != 0) {
+ return -3;
+ }
+ if (pthread_join(tid, nullptr) != 0) {
+ return -4;
+ }
+ *ok = args.parse_ok;
+ return 0;
+}
+
+TEST(Mcpack2pbParserTest, DeeplyNestedPayloadIsRejectedWithoutStackOverflow) {
+ // 16384 levels are far beyond both MAX_DEPTH (128) and what a 1 MB stack
+ // can hold for the recursive parse functions. The payload is only a few
+ // hundred KB, far below the server's max_body_size, so an attacker can
+ // easily send it. Without a recursion limit the parse overflows the
+ // stack and crashes the process.
+ const std::string payload = BuildRecursivePayload(16384);
Review Comment:
`BuildRecursivePayload(16384)` is likely much deeper than needed to validate
the MAX_DEPTH guard, and the current payload builder repeatedly copies the
growing `body` at each nesting level, making construction roughly O(depth²).
This can noticeably slow the unit test suite (especially under ASAN). Using a
depth just above MAX_DEPTH keeps the test meaningful while avoiding excessive
work.
##########
src/mcpack2pb/parser.h:
##########
@@ -91,19 +91,30 @@ class ISOArrayIterator;
// Represent a piece of unparsed(and unread) data of InputStream.
struct UnparsedValue {
UnparsedValue()
- : _type(FIELD_UNKNOWN), _stream(NULL), _size(0) {}
+ : _type(FIELD_UNKNOWN), _stream(NULL), _size(0), _depth(0) {}
UnparsedValue(FieldType type, InputStream* stream, size_t size)
- : _type(type), _stream(stream), _size(size) {}
+ : _type(type), _stream(stream), _size(size), _depth(0) {}
+ // `depth' is the nesting level of this value (0 for the top-level
+ // object). Iterators derive it from the value they iterate over and
+ // reject input deeper than MAX_DEPTH, so that parsing a deeply nested
+ // object fails instead of recursing until the stack overflows.
Review Comment:
`depth` documentation is inconsistent with how iterators are actually
constructed. Generated code creates a top-level `UnparsedValue` with `depth=0`
(see `generator.cpp`), but
`ObjectIterator(UnparsedValue&)`/`UnparsedValue::as_object()` always do
`value.depth() + 1`, so the top-level iterator depth is effectively 1, not 0 as
documented here. This makes the intended depth semantics hard to reason about
and risks off-by-one mistakes in future changes.
This issue also appears on line 166 of the same file.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
