You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "sahvx655-wq (via GitHub)" <gi...@apache.org> on 2026/06/02 06:23:26 UTC
[PR] reject zero-length padded DATA frame in H2Context::OnData (brpc)
sahvx655-wq opened a new pull request, #3325:
URL: https://github.com/apache/brpc/pull/3325
A DATA frame with the PADDED flag but Length 0 underflows frag_size here. `--frag_size` wraps the uint32_t to 0xFFFFFFFF, the `frag_size < pad_length` check then passes, and the bogus size reaches append_and_forward, which swallows the rest of the connection buffer as one DATA payload and feeds ~4GiB into the flow-control counter. OnHeaders already rejects this case; OnData was missing the same guard. Fix returns FRAME_SIZE_ERROR per RFC 7540 6.1 when a padded frame has no room for the pad-length byte.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject zero-length padded DATA frame in H2Context::OnData (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3325:
URL: https://github.com/apache/brpc/pull/3325#discussion_r3359958859
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -698,9 +698,14 @@ H2ParseResult H2StreamContext::OnContinuation(
H2ParseResult H2Context::OnData(
butil::IOBufBytesIterator& it, const H2FrameHead& frame_head) {
+ const bool has_padding = (frame_head.flags & H2_FLAGS_PADDED);
+ if (frame_head.payload_size < (size_t)has_padding) {
Review Comment:
The logic is not clear to converter a bool to a size_t here
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -698,9 +698,14 @@ H2ParseResult H2StreamContext::OnContinuation(
H2ParseResult H2Context::OnData(
butil::IOBufBytesIterator& it, const H2FrameHead& frame_head) {
+ const bool has_padding = (frame_head.flags & H2_FLAGS_PADDED);
+ if (frame_head.payload_size < (size_t)has_padding) {
+ LOG(ERROR) << "Invalid payload_size=" << frame_head.payload_size;
+ return MakeH2Error(H2_FRAME_SIZE_ERROR);
+ }
uint32_t frag_size = frame_head.payload_size;
uint8_t pad_length = 0;
- if (frame_head.flags & H2_FLAGS_PADDED) {
+ if (has_padding) {
Review Comment:
I think a better way is to add the check code here:
```
if (frag_size <= 0) {
LOG(ERROR) << ...
return ...;
}
```
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject zero-length padded DATA frame in H2Context::OnData (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3325:
URL: https://github.com/apache/brpc/pull/3325
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject zero-length padded DATA frame in H2Context::OnData (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3325:
URL: https://github.com/apache/brpc/pull/3325#issuecomment-4700651525
LGTM
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject zero-length padded DATA frame in H2Context::OnData (brpc)
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on code in PR #3325:
URL: https://github.com/apache/brpc/pull/3325#discussion_r3403330156
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -698,9 +698,14 @@ H2ParseResult H2StreamContext::OnContinuation(
H2ParseResult H2Context::OnData(
butil::IOBufBytesIterator& it, const H2FrameHead& frame_head) {
+ const bool has_padding = (frame_head.flags & H2_FLAGS_PADDED);
+ if (frame_head.payload_size < (size_t)has_padding) {
Review Comment:
Agreed, the bool to size_t cast was too cute and obscured what it was actually testing. I've removed it in favour of the explicit zero check inside the PADDED branch below.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -698,9 +698,14 @@ H2ParseResult H2StreamContext::OnContinuation(
H2ParseResult H2Context::OnData(
butil::IOBufBytesIterator& it, const H2FrameHead& frame_head) {
+ const bool has_padding = (frame_head.flags & H2_FLAGS_PADDED);
+ if (frame_head.payload_size < (size_t)has_padding) {
+ LOG(ERROR) << "Invalid payload_size=" << frame_head.payload_size;
+ return MakeH2Error(H2_FRAME_SIZE_ERROR);
+ }
uint32_t frag_size = frame_head.payload_size;
uint8_t pad_length = 0;
- if (frame_head.flags & H2_FLAGS_PADDED) {
+ if (has_padding) {
Review Comment:
Done, moved the check inside the PADDED branch as you suggested. One small tweak: I used `frag_size == 0` rather than `<= 0`, since frag_size is uint32_t and `-W` flags the `<= 0` form as always-false. Same effect, just keeps the build warning-free.
It reads cleaner there anyway, the zero-length case only matters when PADDED is set and we're about to decrement for the pad-length byte. The check sits before `--frag_size`, so a payload_size of 0 with PADDED now returns FRAME_SIZE_ERROR rather than wrapping frag_size to 0xFFFFFFFF and slipping past the `frag_size < pad_length` check. An empty unpadded DATA frame still passes through untouched.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org