This is an automated email from the ASF dual-hosted git repository.
wwbmmm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 04b39291 check chunk header bounds in ubring recv paths (#3497)
04b39291 is described below
commit 04b39291382daeb3e269ec2dac32197646a0682e
Author: UB <[email protected]>
AuthorDate: Sun Aug 30 19:26:09 2026 +0530
check chunk header bounds in ubring recv paths (#3497)
---
src/brpc/ubshm/ub_ring.cpp | 12 ++++++++++++
src/brpc/ubshm/ub_ring.h | 15 +++++++++++++++
test/brpc_ubring_unittest.cpp | 42 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 69 insertions(+)
diff --git a/src/brpc/ubshm/ub_ring.cpp b/src/brpc/ubshm/ub_ring.cpp
index cdc346ed..ea342673 100644
--- a/src/brpc/ubshm/ub_ring.cpp
+++ b/src/brpc/ubshm/ub_ring.cpp
@@ -424,6 +424,12 @@ int UBRing::UbrTrxRecvBlockMode(uint8_t *dest, uint32_t
buf_len)
}
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 recv failed, invalid chunk header msg_len="
+ << (uint32_t)chunk_msg_len << " cur_index=" <<
(uint32_t)cur_index;
+ errno = EBADMSG;
+ return UBRING_ERR;
+ }
uint8_t available_data = chunk_msg_len - cur_index;
int32_t copy_len = (remaining_len < available_data) ? remaining_len :
available_data;
@@ -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;
+ }
uint8_t recv_len =
remain_buf_len > (size_t)(chunk_msg_len - cur_index) ?
(chunk_msg_len - cur_index) : (uint8_t)remain_buf_len;
while (iov_index < iovcnt && recv_len > 0) {
diff --git a/src/brpc/ubshm/ub_ring.h b/src/brpc/ubshm/ub_ring.h
index 412c7598..0a02dd2f 100644
--- a/src/brpc/ubshm/ub_ring.h
+++ b/src/brpc/ubshm/ub_ring.h
@@ -160,6 +160,21 @@ public:
return UBRING_OK;
}
+ // The message length and current offset of a received chunk are read out
+ // of the receive ring, which the remote peer writes. The peer is trusted
+ // to keep msg_len within the payload capacity and cur_index within msg_len
+ // (the send path clamps msg_len to UBR_MSG_PAYLOAD_LEN), but nothing
+ // enforces that on the receive side. When msg_len > UBR_MSG_PAYLOAD_LEN or
+ // cur_index > msg_len, reading `payload.inner + cur_index' for
+ // `msg_len - cur_index' bytes runs past the 60-byte payload (the uint8_t
+ // subtraction also wraps when cur_index > msg_len), so validate the header
+ // before using it.
+ static inline bool IsRecvChunkHeaderValid(uint8_t chunk_msg_len,
+ uint8_t cur_index)
+ {
+ return chunk_msg_len <= UBR_MSG_PAYLOAD_LEN && cur_index <=
chunk_msg_len;
+ }
+
static inline void UpdateDataQTail(UbrTrx *trx)
{
((UbrDataStatusQMsg *)trx->ubr_rx.remote_data_status_q.addr)->tail =
trx->ubr_rx.read_pos;
diff --git a/test/brpc_ubring_unittest.cpp b/test/brpc_ubring_unittest.cpp
index 53bb0a9b..d3d0a7ce 100644
--- a/test/brpc_ubring_unittest.cpp
+++ b/test/brpc_ubring_unittest.cpp
@@ -28,6 +28,8 @@
#include "brpc/ubshm/shm/shm_def.h"
#include "brpc/ubshm/shm/shm_mgr.h"
#include "brpc/ubshm/ub_ring_manager.h"
+#include "brpc/ubshm/ub_ring.h"
+#include "brpc/ubshm/ubr_msg.h"
namespace brpc {
namespace ubring {
@@ -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));
+}
+
+// A crafted chunk laid out exactly like one in the ring: the guard rejects it
+// so the recv loop never reaches the over-reading memcpy.
+TEST(UBRingRecvChunkHeaderTest, crafted_chunk_is_rejected) {
+ brpc::ubring::UbrMsgFormat chunk;
+ memset(&chunk, 0xAB, sizeof(chunk));
+ chunk.header[UBR_MSG_LEN_INDEX] = 255; // peer claims 255 bytes in a
60-byte payload
+ chunk.header[UBR_MSG_CUR_INDEX] = 0;
+ EXPECT_FALSE(brpc::ubring::UBRing::IsRecvChunkHeaderValid(
+ chunk.header[UBR_MSG_LEN_INDEX], chunk.header[UBR_MSG_CUR_INDEX]));
+
+ chunk.header[UBR_MSG_LEN_INDEX] = UBR_MSG_PAYLOAD_LEN;
+ chunk.header[UBR_MSG_CUR_INDEX] = 0;
+ EXPECT_TRUE(brpc::ubring::UBRing::IsRecvChunkHeaderValid(
+ chunk.header[UBR_MSG_LEN_INDEX], chunk.header[UBR_MSG_CUR_INDEX]));
+}
+
#else
TEST(UbringDisabledTest, skip) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
