Copilot commented on code in PR #3499:
URL: https://github.com/apache/brpc/pull/3499#discussion_r3879781517
##########
test/brpc_mcpack2pb_unittest.cpp:
##########
@@ -18,6 +18,7 @@
// Unit tests for the mcpack2pb parser.
#include <gtest/gtest.h>
+#include <pthread.h>
Review Comment:
These tests now hard-depend on `pthread` APIs. If the project is built in
environments without POSIX threads (or with restricted pthread usage), this
will fail to compile/link. Consider guarding the pthread-based test with
platform macros and using `GTEST_SKIP()` when unsupported, or using an existing
brpc/bthread facility if the repo expects cross-platform test builds.
##########
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` tracks iterator nesting. Generated code constructs a top-level
+ // UnparsedValue with depth=0 and iterators are created with depth+1.
Review Comment:
The documentation here defines `UnparsedValue::depth` as a value where
iterators are created with `depth+1`. However, other comments (e.g.,
`ObjectIterator` below) describe the top-level object as starting at depth 0,
which conflicts with typical usage patterns that construct an iterator from a
top-level `UnparsedValue` (making the first iterator depth 1). Please align
these comments so they describe the same depth convention (either ‘iterator
depth’ or ‘parent depth’) to avoid confusion and future off-by-one regressions.
##########
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` tracks iterator nesting. Generated code constructs a top-level
+ // UnparsedValue with depth=0 and iterators are created with depth+1.
+ // Input nested deeper than MAX_DEPTH is rejected so parsing fails instead
+ // of recursing until the stack overflows.
+ UnparsedValue(FieldType type, InputStream* stream, size_t size, size_t
depth)
+ : _type(type), _stream(stream), _size(size), _depth(depth) {}
void set(FieldType type, InputStream* stream, size_t size) {
+ set(type, stream, size, _depth);
+ }
+ void set(FieldType type, InputStream* stream, size_t size, size_t depth) {
_type = type;
_stream = stream;
_size = size;
+ _depth = depth;
}
Review Comment:
The 3-arg `set(...)` overload implicitly preserves the previous `_depth`.
That makes it easy for future call sites to accidentally reuse an
`UnparsedValue` but forget to update depth, weakening the depth-limit
enforcement or making depth semantics depend on object reuse. Consider either
(a) making the 3-arg overload reset depth to a well-defined default (e.g., 0)
to avoid hidden state, or (b) documenting prominently that it intentionally
preserves depth and ensuring internal code always uses the 4-arg overload when
populating nested values.
##########
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);
+ }
Review Comment:
`BuildRecursivePayload(depth)` performs multiple full copies of the growing
`body` per loop iteration (`item.append(body)`, then `arr.append(item)`, then
`child.append(arr)`, then `new_body.append(child)`), which makes payload
construction roughly O(depth²) in total bytes copied. With `depth=16384` this
can significantly slow down or destabilize CI. Consider reducing the tested
depth to the minimum that still exercises the limit (e.g., `MAX_DEPTH + k`),
and/or rewriting the builder to avoid repeated copying (precompute final size
and write into a single buffer, or reserve aggressively and build in-place).
##########
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) {
Review Comment:
If `pthread_attr_setstacksize` fails, the function returns `-2` without
calling `pthread_attr_destroy(&attr)`. This leaks the pthread attribute object
on that error path. Consider using a single cleanup path (or an RAII helper) to
ensure `pthread_attr_destroy` is called after a successful `pthread_attr_init`
regardless of subsequent failures.
--
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]
