Copilot commented on code in PR #3476:
URL: https://github.com/apache/brpc/pull/3476#discussion_r3836080105
##########
src/brpc/policy/gzip_compress.cpp:
##########
@@ -154,6 +236,13 @@ inline bool GzipDecompressBase(
}
const int size_cp = std::min(size_in, size_out);
memcpy(data_out, data_in, size_cp);
+ total_out += size_cp;
+ if (total_out > limit) {
+ LOG(WARNING) << "Decompressed size exceeds"
+ " -max_decompressed_body_size=" << limit
+ << ", format=" << Format2CStr(format);
+ return false;
Review Comment:
On this newly added cap-exceeded return, `out.Next()` has already appended
the whole output block to `msg` and only `size_out` bytes were actually
written. Returning without `out.BackUp(size_out)` leaves the unwritten tail
(including uninitialized bytes) in the caller's IOBuf; trim the current output
block before returning.
##########
src/brpc/policy/gzip_compress.cpp:
##########
@@ -26,6 +28,66 @@
namespace brpc {
namespace policy {
+namespace {
+
+// A ZeroCopyInputStream wrapper that stops reading from the underlying stream
+// once a limit of bytes has been handed out. Different protobuf releases
+// disagree on the availability/location of the stock LimitingInputStream (it
+// does not exist before ~3.19), so implement the same behaviour locally to
+// stay portable across the protobuf versions CI builds against.
+class DelegatingLimitingInputStream : public
google::protobuf::io::ZeroCopyInputStream {
+public:
+ DelegatingLimitingInputStream(google::protobuf::io::ZeroCopyInputStream*
input,
+ int64_t limit)
+ : _input(input), _limit(limit), _bytes_read(0) {}
+
+ bool Next(const void** data, int* size) override {
+ if (_bytes_read >= _limit) {
+ return false;
+ }
+ if (!_input->Next(data, size)) {
+ return false;
+ }
+ const int64_t total = _bytes_read + *size;
+ if (total > _limit) {
+ const int excess = (int)(total - _limit);
+ _input->BackUp(excess);
+ *size -= excess;
Review Comment:
When this `Next()` call clips a decompressed block, it first calls
`_input->BackUp(excess)` and then exposes the remaining prefix as if it came
from a fresh `Next()`. If the consumer backs up unused bytes from that prefix
(protobuf parsers are allowed to do so),
`DelegatingLimitingInputStream::BackUp()` forwards a second `BackUp()` without
an intervening `Next()`, which violates the underlying stream contract and can
trigger its `CHECK` or rewind the wrong range. Keep the clipped tail/state in
this wrapper and handle subsequent backup without double-backing up the wrapped
stream.
##########
src/mcpack2pb/parser-inl.h:
##########
@@ -144,11 +144,25 @@ inline void ObjectIterator::init(InputStream* stream,
size_t size) {
_stream = stream;
_expected_popped_bytes = _stream->popped_bytes() + sizeof(ItemsHead);
_expected_popped_end = _stream->popped_bytes() + size;
+ // Every field head takes at least 2 bytes (FieldFixedHead), so a valid
+ // item_count never exceeds half of the remaining value size. The count
+ // is copied verbatim from the wire, reject inconsistent values instead
+ // of trusting them. Guard the size before reading ItemsHead so payloads
+ // shorter than the header are not read past their declared boundary.
+ if (size < sizeof(ItemsHead)) {
+ CHECK(false) << "buffer(size=" << size << ") is not enough";
Review Comment:
This path is reachable from a wire-declared object size smaller than
`ItemsHead`. `CHECK(false)` is a fatal assertion in this project, so a
malformed mcpack request can terminate the process instead of being rejected by
`set_bad()`; use non-fatal logging for untrusted input validation.
This issue also appears on line 162 of the same file.
--
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]
