Copilot commented on code in PR #3497:
URL: https://github.com/apache/brpc/pull/3497#discussion_r3885884698
##########
src/brpc/ubshm/ub_ring.cpp:
##########
@@ -1081,6 +1087,12 @@ ssize_t UBRing::StartReadv(UbrTrx *trx, const struct
iovec *iov, int iovcnt, siz
}
uint8_t chunk_msg_len = current_chunk->header[UBR_MSG_LEN_INDEX];
uint8_t cur_index = current_chunk->header[UBR_MSG_CUR_INDEX];
+ if (UNLIKELY(!IsRecvChunkHeaderValid(chunk_msg_len, cur_index))) {
+ LOG(ERROR) << "Trx readv failed, invalid chunk header msg_len="
+ << (uint32_t)chunk_msg_len << " cur_index=" <<
(uint32_t)cur_index;
+ errno = EBADMSG;
+ return UBRING_ERR;
+ }
Review Comment:
In this error path you set errno=EBADMSG and return -1, but callers going
through UbrTrxReadv() will never observe EBADMSG: UbrTrxReadv() treats any -1
from UbrTrxReadvBlockMode/StartReadv as “connection has been closed”, logs that
message, and unconditionally overwrites errno to EPIPE. That makes the new
EBADMSG contract ineffective for the readv receive path and produces a
misleading log message.
##########
test/brpc_ubring_unittest.cpp:
##########
@@ -245,6 +247,46 @@ TEST_F(UBShmEndpointTest, reset_is_idempotent) {
_ep->Reset();
}
+// The receive paths (UbrTrxRecvBlockMode / StartReadv) read `msg_len' and
+// `cur_index' out of a chunk header the remote peer writes into the ring, then
+// copy `msg_len - cur_index' bytes from the 60-byte `payload.inner'. A peer
+// that writes msg_len > 60, or cur_index > msg_len (which underflows the
+// uint8_t subtraction), makes that copy over-read the payload into adjacent
+// shared memory. IsRecvChunkHeaderValid is the guard both paths now apply.
+TEST(UBRingRecvChunkHeaderTest, reject_out_of_range_len_and_index) {
+ using brpc::ubring::UBRing;
+ // Legitimate values a well-formed peer produces: full payload, partial
+ // consume, and the fully-consumed boundary.
+ EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(UBR_MSG_PAYLOAD_LEN, 0));
+ EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(10, 5));
+ EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(0, 0));
+ EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(UBR_MSG_PAYLOAD_LEN,
+ UBR_MSG_PAYLOAD_LEN));
+ // msg_len past the payload capacity -> over-read source.
+ EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(UBR_MSG_PAYLOAD_LEN + 1, 0));
+ EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(255, 0));
+ // cur_index past msg_len -> `msg_len - cur_index' underflows to a large
+ // uint8_t.
+ EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(0, 1));
+ EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(10, 20));
+}
Review Comment:
These new tests only validate the helper IsRecvChunkHeaderValid(), but they
don’t exercise the actual receive APIs (UbrTrxRecvBlockMode /
UbrTrxReadvBlockMode) to prove the guard is applied and that malformed headers
are rejected with the intended errno (especially via UbrTrxReadv(), which
currently overwrites errno on -1). A regression test that drives a real read/
readv call against a crafted invalid chunk header would catch integration
issues.
--
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]
