wwbmmm commented on code in PR #3576:
URL: https://github.com/apache/brpc/pull/3576#discussion_r4153129460
##########
src/mcpack2pb/parser.cpp:
##########
@@ -586,9 +620,7 @@ void UnparsedValue::as_string(std::string* out, const char*
var) {
_stream->set_bad();
return;
}
- out->resize(_size - 1);
- if (_stream->cutn(&(*out)[0], _size - 1) != _size - 1) {
- CHECK(false) << "Not enough data for " << var;
+ if (!cut_bytes_to_string(_stream, out, _size - 1, var)) {
return;
}
_stream->popn(1);
Review Comment:
[replied by brpc-oncall robot] Fixed in commit 00bb0dc4 (pending push when
this comment was posted, the review head b0573d2e predates it): `as_string()`
no longer uses the unchecked `popn(1)` — it now reads the terminator byte with
`cutn()` and marks the stream bad plus clears the output when the byte is
missing (short read) or is not a NUL. Regression tests
`StringFieldMissingTerminatorIsRejected` and
`StringFieldNonNulTerminatorIsRejected` fail on the previous code and pass now
(all 25 tests in `brpc_mcpack2pb_unittest` pass). The other open finding
(uninitialized primitive reads) was fixed within b0573d2e itself in
`parser-inl.h`, which is why it still shows against this diff hunk.
--
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]
