wwbmmm commented on code in PR #3576:
URL: https://github.com/apache/brpc/pull/3576#discussion_r4151508165
##########
src/mcpack2pb/parser.cpp:
##########
@@ -588,7 +595,8 @@ void UnparsedValue::as_string(std::string* out, const char*
var) {
}
out->resize(_size - 1);
if (_stream->cutn(&(*out)[0], _size - 1) != _size - 1) {
- CHECK(false) << "Not enough data for " << var;
+ LOG(ERROR) << "Not enough data for " << var;
+ _stream->set_bad();
Review Comment:
[replied by brpc-oncall robot] Good catch — the eager
`resize(_size)`/`resize(_size - 1)` did let a wire-controlled size close to
`UINT32_MAX` trigger an uncaught `std::bad_alloc`/`std::length_error` before
the truncation check, defeating the hardening on this path.
Fixed in the latest commit: `as_string()`/`as_binary()` now share a
`cut_bytes_to_string()` helper that reads in bounded 8 KB chunks and grows the
output only for bytes that are actually present, so the output never exceeds
the real payload size; a short read marks the stream bad (visible via
`stream()->good()`, which the generated code checks) and clears the output.
Added regression tests `StringFieldHugeClaimedSizeDoesNotThrow` and
`BinaryFieldHugeClaimedSizeDoesNotThrow` (a ~4 GB claimed size with a 1-byte
payload): they fail on the previous code and pass now. The other
wire-controlled resizes were audited — `ref_cut()` in the iterators is bounded
by the 1-byte `name_size`, so it is not affected.
--
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]
