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: [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]
