Copilot commented on code in PR #3402:
URL: https://github.com/apache/brpc/pull/3402#discussion_r3629018954
##########
src/brpc/couchbase.cpp:
##########
@@ -1506,10 +1506,14 @@ bool
CouchbaseOperations::CouchbaseResponse::popCollectionId(
if (header.status != 0) {
// handle error case
- _buf.pop_front(sizeof(header) + header.extras_length + header.key_length);
// Possibly read error message from value if present
- size_t value_size =
- header.total_body_length - header.extras_length - header.key_length;
+ const int value_size = (int)header.total_body_length -
+ (int)header.extras_length - (int)header.key_length;
+ if (value_size < 0) {
+ butil::string_printf(&_err, "value_size=%d is negative", value_size);
+ return false;
+ }
Review Comment:
Casting the (likely) 32-bit unsigned `total_body_length` to `int` can
overflow for responses larger than `INT_MAX` (e.g., >2GiB), incorrectly turning
a valid size into a negative number and causing false failures. Prefer
computing with a wider signed type (e.g., `int64_t`) and only converting to
`size_t` after validating the range; also prefer `static_cast<...>` over
C-style casts.
##########
src/brpc/couchbase.cpp:
##########
@@ -1506,10 +1506,14 @@ bool
CouchbaseOperations::CouchbaseResponse::popCollectionId(
if (header.status != 0) {
// handle error case
- _buf.pop_front(sizeof(header) + header.extras_length + header.key_length);
// Possibly read error message from value if present
- size_t value_size =
- header.total_body_length - header.extras_length - header.key_length;
+ const int value_size = (int)header.total_body_length -
+ (int)header.extras_length - (int)header.key_length;
+ if (value_size < 0) {
+ butil::string_printf(&_err, "value_size=%d is negative", value_size);
Review Comment:
The new error message is low-context for diagnosing malformed responses.
Consider including the relevant header fields (e.g., `total_body_length`,
`extras_length`, `key_length`, and possibly `command/status`) so logs clearly
identify what was inconsistent.
##########
test/brpc_couchbase_unittest.cpp:
##########
@@ -98,6 +98,36 @@ TEST_F(CouchbaseUnitTest, ResultStruct) {
EXPECT_EQ(0x00, result.status_code);
}
+// The header fields are attacker-controlled, extras_length + key_length may
+// exceed total_body_length. popManifest/popCollectionId must not treat the
+// underflowed remainder as a value size and drain the pipelined buffer.
+TEST_F(CouchbaseUnitTest, CollectionResponseWithInconsistentBodyLength) {
+ const std::string next_response(64, 'A');
+
+ brpc::policy::CouchbaseResponseHeader header = {};
+ header.magic = brpc::policy::CB_MAGIC_RESPONSE;
+ header.command = brpc::policy::CB_GET_COLLECTIONS_MANIFEST;
+ header.extras_length = 1;
+ header.total_body_length = 0;
+
+ brpc::CouchbaseOperations::CouchbaseResponse res;
+ res.rawBuffer().append(&header, sizeof(header));
+ res.rawBuffer().append(next_response);
+ std::string manifest = "untouched";
+ EXPECT_FALSE(res.popManifest(&manifest));
+ EXPECT_EQ("untouched", manifest);
+ EXPECT_EQ(sizeof(header) + next_response.size(), res.rawBuffer().size());
+
+ header.command = brpc::policy::CB_COLLECTIONS_GET_CID;
+ header.status = 1;
+ brpc::CouchbaseOperations::CouchbaseResponse res2;
+ res2.rawBuffer().append(&header, sizeof(header));
+ res2.rawBuffer().append(next_response);
+ uint8_t collection_id = 0;
+ EXPECT_FALSE(res2.popCollectionId(&collection_id));
+ EXPECT_EQ(sizeof(header) + next_response.size(), res2.rawBuffer().size());
Review Comment:
This regression test validates the buffer is untouched on failure, but it
doesn’t assert that the output parameter is also untouched. Add an
`EXPECT_EQ(0, collection_id)` (or initialize to a non-zero sentinel) to ensure
`popCollectionId()` doesn’t partially mutate outputs on malformed responses.
##########
test/brpc_couchbase_unittest.cpp:
##########
@@ -98,6 +98,36 @@ TEST_F(CouchbaseUnitTest, ResultStruct) {
EXPECT_EQ(0x00, result.status_code);
}
+// The header fields are attacker-controlled, extras_length + key_length may
+// exceed total_body_length. popManifest/popCollectionId must not treat the
+// underflowed remainder as a value size and drain the pipelined buffer.
+TEST_F(CouchbaseUnitTest, CollectionResponseWithInconsistentBodyLength) {
+ const std::string next_response(64, 'A');
+
+ brpc::policy::CouchbaseResponseHeader header = {};
+ header.magic = brpc::policy::CB_MAGIC_RESPONSE;
+ header.command = brpc::policy::CB_GET_COLLECTIONS_MANIFEST;
+ header.extras_length = 1;
+ header.total_body_length = 0;
Review Comment:
Couchbase/Memcached binary protocol headers typically encode multi-byte
fields in network byte order. In tests, prefer writing these fields using
`htons/htonl` (or a helper that serializes the header in wire format) to avoid
host-endianness dependence and ensure the test is validating the parser against
realistic on-the-wire bytes.
--
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]
