Fix bug when parsing zero-length string field in mcpack2pb - #3450
Open
wwbmmm wants to merge 3 commits into
Open
Conversation
UnparsedValue::as_string() resizes the output string to without checking , where is the value_size of a string field read from the input. When value_size is 0, underflows to SIZE_MAX and resize() throws std::length_error, which is not caught on the request path and therefore crashes the server. Reject such malformed fields by marking the stream bad so the caller can fail the request gracefully instead.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the mcpack2pb request-parsing path when encountering malformed string fields with value_size == 0, improving robustness of servers using the nshead + ubrpc mcpack2 adaptor.
Changes:
- Add validation in
mcpack2pb::UnparsedValue::as_string()to reject zero-length string fields (prevents_size - 1underflow and uncaughtstd::length_error). - Add unit tests that cover the malformed zero-size string case and a normal string parse.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mcpack2pb/parser.cpp | Adds guard for _size == 0 in UnparsedValue::as_string() to avoid underflow/exception. |
| test/brpc_mcpack2pb_unittest.cpp | Introduces parser unit tests for malformed zero-size string field and a valid string field. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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?
Issue Number: null
Problem Summary:
A malformed mcpack2/compack request that contains a string field with a
zero value_size crashes the server. In
UnparsedValue::as_string(),_size(the string field's value_size, taken directly from the input)is used in
out->resize(_size - 1)without being validated first. Whenvalue_size is 0,
_size - 1underflows to SIZE_MAX,resize()throwsstd::length_error, and since the exception is not caught anywhere onthe request parsing path the whole process aborts. This is reachable
from
UbrpcAdaptor::ParseNsheadMetaandUbrpcAdaptor::ParseRequestFromIOBuffor servers wired with thenshead + ubrpc mcpack2 adaptor.
What is changed and the side effects?
Changed:
src/mcpack2pb/parser.cpp: reject string fields whose value_size is 0in
UnparsedValue::as_string()by marking the input stream bad andreturning, instead of underflowing and throwing.
test/brpc_mcpack2pb_unittest.cpp: add unit tests covering thezero-size string field case and a normal string field parse.
Side effects:
exactly as before; only previously-crashing malformed input now fails
gracefully.
Check List: