Limit recursion depth of mcpack2pb parser - #3499
Open
wwbmmm wants to merge 3 commits into
Open
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
The mcpack2pb parser recurses once per nesting level of the incoming object/array. A message with an excessively deep recursive structure makes the recursion grow unbounded and the stack overflow, crashing the process. The serializer already enforces MAX_DEPTH=128, but the parse path never checked it. Thread the nesting depth through UnparsedValue and the iterators, and fail the parse once the depth exceeds MAX_DEPTH, mirroring the serializer. Deeply nested but legitimate messages keep working.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a recursion/nesting depth limit to the mcpack2pb parser to prevent unbounded recursion on deeply nested mcpack objects/arrays (stack overflow), aligning parser behavior with the existing serializer depth cap.
Changes:
- Thread a nesting-depth counter through mcpack2pb::UnparsedValue and iterator constructors.
- Reject parsing when iterator nesting exceeds MAX_DEPTH to bound recursion.
- Add unit tests for deep (rejected) vs moderate (accepted) nesting on a small-stack thread.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/mcpack2pb/parser.h | Adds depth plumbing to UnparsedValue, ObjectIterator, and ArrayIterator declarations and constructors. |
| src/mcpack2pb/parser-inl.h | Enforces MAX_DEPTH in iterator init() and threads depth through as_object() / as_array(). |
| src/mcpack2pb/parser.cpp | Propagates iterator depth into yielded UnparsedValue items during iteration. |
| test/brpc_mcpack2pb_unittest.cpp | Adds regression tests ensuring deep nesting is rejected without stack overflow (small-stack thread). |
src/mcpack2pb/parser.h:170
- The depth comment here says the top-level object starts at 0, but ObjectIterator(UnparsedValue&) initializes with value.depth() + 1 (and generated code builds the top-level UnparsedValue with depth=0). Clarifying that iterator depth is effectively 1-based in the common/auto-generated path will make the MAX_DEPTH enforcement easier to maintain.
// Parse `n' bytes from `stream' as fields of an object.
// `depth' is the nesting level of the object; the top-level object
// starts at 0 and each nested iterator adds 1. Input nested deeper
// than MAX_DEPTH is rejected to avoid stack overflow on unbounded
// recursion (CWE-674), mirroring the serializer's limit.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Comment thread
src/mcpack2pb/parser.h
Outdated
Comment thread
test/brpc_mcpack2pb_unittest.cpp
Outdated
Comment thread
test/brpc_mcpack2pb_unittest.cpp
Outdated
Comment thread
test/brpc_mcpack2pb_unittest.cpp
Comment thread
test/brpc_mcpack2pb_unittest.cpp
Comment thread
src/mcpack2pb/parser.h
Outdated
Comment thread
src/mcpack2pb/parser.h
Make the depth semantics in parser.h consistent with the implementation (UnparsedValue::depth counts the containers the value is nested in, iterators add one), document that the 3-arg set() intentionally keeps the depth, and trim the unit test: parse a payload a few levels beyond MAX_DEPTH instead of 16384 levels, which is equally effective, much faster and avoids O(depth^2) payload construction. Also destroy the pthread attribute on every path.
wwbmmm
force-pushed
the
fix-mcpack2pb-parser-depth-limit
branch
from
67f8a92 to
10d3847
Compare
August 28, 2026 11:07
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (1)src/mcpack2pb/parser.h:221
- ArrayIterator(InputStream*, size_t, size_t depth = 0) has the same issue as ObjectIterator: with the default depth of 0, nested UnparsedValue items will keep depth 0, so recursion depth never increases and MAX_DEPTH is never enforced for callers using this overload.
class ArrayIterator {
public:
typedef UnparsedValue Field;
ArrayIterator(InputStream* stream, size_t size, size_t depth = 0)
{ init(stream, size, depth); }
explicit ArrayIterator(UnparsedValue& value)
{ init(value.stream(), value.size(), value.depth() + 1); }
Sorry, something went wrong.
Default the stream-based ObjectIterator/ArrayIterator depth to 1 so the top-level container always has depth 1 whether built from a value (depth+1) or directly from a stream, matching the documented semantics and keeping the MAX_DEPTH guard effective for all constructor paths. In the unit test, destroy the pthread attribute on a single shared path right after create (when it is still valid and no longer needed), instead of unconditionally calling pthread_attr_destroy which would be undefined behavior if pthread_attr_init failed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
The mcpack2pb parser (used by nshead_mcpack / ubrpc / public_pbrpc
requests) has no limit on the nesting depth of the input object/array.
For a protobuf message that references itself (e.g. a repeated
message field), the generated parser recurses once per nesting level,
so an input nested deep enough makes the recursion grow unbounded and
overflow the stack, crashing the process. The serializer has enforced
MAX_DEPTH (src/mcpack2pb/field_type.h) since the beginning, the
parser just never did.
What is changed and the side effects?
Changed:
and the ObjectIterator / ArrayIterator constructors.
exceeds MAX_DEPTH (128), exactly like the serializer already does,
so that the recursion stops at a bounded level instead of exhausting
the stack.
without crashing) and a moderately-nested one (parsed normally).
The depth limit is enforced on the iterator level, so existing code
generated by protoc-gen-mcpack is protected without regeneration:
no generated-code API is changed.
Side effects:
Parsing now fails for mcpack input nested deeper than 128 levels
(the same limit the serializer already applies). Such input is
abnormal for any real workload, roughly matching
json2pb_max_recursion_depth (100) used by the JSON parser.
Performance effects: none (one integer compare per iterator).
Breaking backward compatibility: no.
Check List: