Copilot commented on code in PR #3476:
URL: https://github.com/apache/brpc/pull/3476#discussion_r3835965447
##########
src/mcpack2pb/parser-inl.h:
##########
@@ -149,6 +149,16 @@ inline void ObjectIterator::init(InputStream* stream,
size_t size) {
CHECK(false) << "buffer(size=" << size << ") is not enough";
return set_bad();
}
+ // `size' covers ItemsHead plus all fields and every field head takes at
+ // least 2 bytes (FieldFixedHead), thus 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.
+ if (size < sizeof(ItemsHead) ||
+ items_head.item_count > (size - sizeof(ItemsHead)) / 2) {
Review Comment:
The `size < sizeof(ItemsHead)` guard is evaluated only after
`cut_packed_pod` has already consumed the header. For an object value shorter
than four bytes, this reads past the value's declared boundary (potentially
into the next field) before rejecting it. Check `size` before reading
`ItemsHead`, as `ArrayIterator::init` does.
This issue also appears on line 158 of the same file.
##########
src/brpc/compress.h:
##########
@@ -27,6 +27,15 @@
namespace brpc {
+DECLARE_uint64(max_decompressed_body_size);
Review Comment:
This public header uses `DECLARE_uint64` without including
`gflags/gflags_declare.h`; its current includes do not define that macro. As a
result, including `brpc/compress.h` directly (as the new compression test does)
fails to compile. Add the gflags declaration header before this declaration.
##########
test/brpc_snappy_compress_unittest.cpp:
##########
@@ -253,3 +254,35 @@ TEST_F(test_compress_method, mass_snappy_iobuf) {
ASSERT_TRUE(strcmp(check_str.c_str(), text) == 0);
delete [] text;
}
+
+TEST_F(test_compress_method, decompressed_size_capped) {
+ // Regression test: decompressors used to enforce no output limit, so a
+ // small compressed body (checked against -max_body_size in compressed
+ // form only) could decompress to tens of GiB (decompression bomb).
+ const uint64_t saved = brpc::FLAGS_max_decompressed_body_size;
+ brpc::FLAGS_max_decompressed_body_size = 1024;
Review Comment:
The test mutates the process-global decompression flag, but any `ASSERT_*`
below can return before line 287 restores it, making later tests
order-dependent. Guard the mutation with `GFLAGS_NAMESPACE::FlagSaver`, as the
nearby max-body tests do, instead of relying only on the final assignment.
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -659,6 +659,84 @@ TEST(RtmpTest, amf_rejects_oversized_ecma_array_count) {
EXPECT_FALSE(brpc::ReadAMFObject(&obj, &istream));
}
+TEST(RtmpTest, amf_truncated_long_string_does_not_allocate_declared_size) {
+ // Regression: a tiny message declaring a huge (but under-the-cap)
+ // string length must not cause the declared size to be allocated
+ // before the bytes are actually available in the stream.
+ const uint32_t declared_len = 8 * 1024 * 1024;
+ std::string req_buf;
+ AppendAMFLongStringHeader(&req_buf, declared_len);
+ req_buf.append("only-a-few-bytes", 16);
+
+ google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
+ brpc::AMFInputStream istream(&zc_stream);
+ std::string result;
+ EXPECT_FALSE(brpc::ReadAMFString(&result, &istream));
+ EXPECT_TRUE(result.empty());
+ // Reading is chunked, so a truncated stream leaves at most one chunk
+ // of capacity behind instead of the full declared length.
+ EXPECT_LT(result.capacity(), (size_t)declared_len);
+}
+
+TEST(RtmpTest, amf_reads_long_string_larger_than_one_chunk) {
+ const std::string big(200 * 1024, 'x');
+ std::string req_buf;
+ {
+ google::protobuf::io::StringOutputStream zc_stream(&req_buf);
+ brpc::AMFOutputStream ostream(&zc_stream);
+ brpc::WriteAMFString(big, &ostream);
+ ASSERT_TRUE(ostream.good());
+ }
+ google::protobuf::io::ArrayInputStream zc_stream(req_buf.data(),
req_buf.size());
+ brpc::AMFInputStream istream(&zc_stream);
+ std::string result;
+ ASSERT_TRUE(brpc::ReadAMFString(&result, &istream));
+ ASSERT_EQ(big, result);
+}
+
+TEST(RtmpTest, chunk_stream_rejects_message_length_over_max_body_size) {
+ int pipe_fds[2];
+ ASSERT_EQ(0, pipe(pipe_fds));
+ butil::fd_guard guard0(pipe_fds[0]); // read end, closed by this guard
+ butil::fd_guard guard1(pipe_fds[1]); // write end, handed over to Socket
+
+ brpc::SocketId id;
+ brpc::SocketOptions options;
+ options.fd = guard1.release(); // Socket takes ownership of the fd
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
+ brpc::SocketUniquePtr sock;
+ ASSERT_EQ(0, brpc::Socket::Address(id, &sock));
+
+ brpc::policy::RtmpContext ctx(nullptr, nullptr);
+ ctx.SetState(sock->remote_side(),
+ brpc::policy::RtmpContext::STATE_RECEIVED_C2);
+
+ // The message length declared by a chunk header is remote-controlled and
+ // was never bounded: with repeated mid-message headers a connection's
+ // reassembly buffer could grow without limit. A type-0 header declaring
+ // a length above -max_body_size must be rejected up front.
+ const uint64_t saved_max_body_size = brpc::FLAGS_max_body_size;
+ brpc::FLAGS_max_body_size = 1024;
Review Comment:
The test mutates the process-global `max_body_size`, but an assertion can
return before line 736 restores it, leaving subsequent tests with a 1024-byte
limit. Use `GFLAGS_NAMESPACE::FlagSaver` for this scoped mutation, matching the
existing flag-handling pattern in the test suite.
--
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]
