Copilot commented on code in PR #3499:
URL: https://github.com/apache/brpc/pull/3499#discussion_r3880119357
##########
test/brpc_mcpack2pb_unittest.cpp:
##########
@@ -156,4 +157,142 @@ 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'
+ // Reserve enough space for the final payload so that appending at each
+ // nesting level does not reallocate (the size of each wrapping level is
+ // 1 + 1 + 4 + body + 4 + item + 1 + 1 + 4 + name + arr).
+ body.reserve((size_t)depth * 30);
+ 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;
+ int rc = pthread_attr_init(&attr);
+ if (rc == 0) {
+ rc = pthread_attr_setstacksize(&attr, 1024 * 1024);
+ }
+ int stack_rc = rc;
+ pthread_t tid;
+ if (stack_rc == 0) {
+ stack_rc = pthread_create(&tid, &attr, ParseOnSmallStack, &args);
+ }
+ if (stack_rc == 0) {
+ rc = pthread_join(tid, nullptr);
+ }
+ pthread_attr_destroy(&attr);
Review Comment:
`pthread_attr_destroy(&attr)` is called unconditionally, but `attr` is only
valid if `pthread_attr_init` succeeded. If `pthread_attr_init` fails,
destroying an uninitialized `pthread_attr_t` is undefined behavior.
##########
src/mcpack2pb/parser.h:
##########
@@ -152,9 +168,15 @@ class ObjectIterator {
};
// Parse `n' bytes from `stream' as fields of an object.
- ObjectIterator(InputStream* stream, size_t n) { init(stream, n); }
+ // `depth' is the nesting level of the container being iterated
+ // (1 for the top-level object, since the provided value is already
+ // nested in one container). Input nested deeper than MAX_DEPTH is
+ // rejected to avoid stack overflow on unbounded recursion (CWE-674),
+ // mirroring the serializer's limit.
+ ObjectIterator(InputStream* stream, size_t n, size_t depth = 0)
+ { init(stream, n, depth); }
Review Comment:
`ObjectIterator(InputStream*, size_t, size_t depth = 0)` defaults `depth` to
0, which prevents depth from increasing when iterating nested containers
(nested `UnparsedValue` instances inherit `_depth`), effectively bypassing the
MAX_DEPTH guard for callers using this overload. The comment above also states
the top-level object should be depth 1, so the default should match that
behavior.
This issue also appears on line 214 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]
