Copilot commented on code in PR #3524:
URL: https://github.com/apache/brpc/pull/3524#discussion_r3958359812
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1396,24 +1422,34 @@ int
H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
char* endptr = nullptr;
const int sc = strtol(pair.value.c_str(), &endptr, 10);
if (*endptr != '\0') {
- LOG(ERROR) << "Invalid status=" << pair.value;
- return -1;
+ LOG(ERROR) << "Invalid status=" << pair.value
+ << ", stream_id=" << _stream_id;
+ _rejected_error = H2_PROTOCOL_ERROR;
+ } else {
+ h.set_status_code(sc);
}
- h.set_status_code(sc);
}
break;
default:
break;
}
if (!matched) {
- LOG(ERROR) << "Unknown name=`" << name << '\'';
- return -1;
+ LOG(ERROR) << "Unknown pseudo-header=`" << name
+ << "', stream_id=" << _stream_id;
+ _rejected_error = H2_PROTOCOL_ERROR;
}
} else if (name[0] == 'c' &&
strcmp(name + 1, /*c*/"ontent-type") == 0) {
h.set_content_type(pair.value);
} else {
h.AppendHeader(pair.name, pair.value);
+ if (FLAGS_http_max_header_count > 0 &&
+ h.HeaderCount() > FLAGS_http_max_header_count) {
+ LOG(ERROR) << "Too many headers, max="
+ << FLAGS_http_max_header_count
+ << ", stream_id=" << _stream_id;
+ _rejected_error = H2_ENHANCE_YOUR_CALM;
Review Comment:
The header-count limit is checked after `AppendHeader`, which means the
(max+1)th header is still stored. This conflicts with the intent described
elsewhere in the code/comments about stopping memory growth once a stream is
rejected. Consider checking the limit *before* appending (e.g., `HeaderCount()
>= max`) or removing the just-appended header when the limit is exceeded so the
stored header set is strictly bounded.
##########
src/brpc/uri.h:
##########
@@ -99,8 +99,9 @@ class URI {
void set_port(int port) { _port = port; }
void SetHostAndPort(const std::string& host_and_optional_port);
// Set path/query/fragment with the input in form of "path?query#fragment"
- void SetH2Path(const char* h2_path);
- void SetH2Path(const std::string& path) { SetH2Path(path.c_str()); }
+ // Returns 0 on success, -1 otherwise and status() is set.
+ int SetH2Path(const char* h2_path);
+ int SetH2Path(const std::string& path) { return SetH2Path(path.c_str()); }
Review Comment:
Changing `URI::SetH2Path` from `void` to `int` is a public API break (and
can also be an ABI break for downstreams). If backward compatibility is
required, consider keeping the existing `void SetH2Path(...)` as a wrapper
(possibly deprecated) and introducing a new status-returning method (e.g.,
`TrySetH2Path`), or otherwise clearly documenting this breaking change in
release notes.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -1940,6 +1942,231 @@ TEST_F(HttpTest,
h2_header_list_budget_resets_per_block) {
delete sctx;
}
+// Literal header field with a new name, with both lengths in a single 7-bit
+// prefix octet. `first_octet` selects the representation: 0x00 is "without
+// indexing" (RFC 7541 6.2.2), 0x40 is "with incremental indexing" (6.2.1)
+// which also adds the field to the dynamic table.
+// 0x80 of a length octet is the Huffman flag and a length of 128 or more needs
+// the multi-octet form, so refuse what does not fit instead of emitting a
+// corrupt header block.
+void AppendLiteralHeader(butil::IOBuf* out, const std::string& name,
+ const std::string& value, uint8_t first_octet = 0x00)
{
+ ASSERT_LT(name.size(), 0x80u);
+ ASSERT_LT(value.size(), 0x80u);
+ uint8_t prefix[] = { first_octet, (uint8_t)name.size() };
+ out->append(prefix, sizeof(prefix));
+ out->append(name);
+ uint8_t value_len = (uint8_t)value.size();
+ out->append(&value_len, 1);
+ out->append(value);
+}
+
+// Feed `payload` to `sctx` as one complete HEADERS block, the way
+// H2Context::Consume() would. A non-zero stream_id in the result means the
+// frame handler asked for a RST_STREAM, a zero one means a GOAWAY that closes
+// the whole connection.
+brpc::policy::H2ParseResult ConsumeHeadersBlock(
+ brpc::policy::H2StreamContext* sctx, const butil::IOBuf& payload,
+ int stream_id) {
+ brpc::policy::H2FrameHead head;
+ head.payload_size = payload.size();
+ head.type = brpc::policy::H2_FRAME_HEADERS;
+ head.flags = 0x4; // H2_FLAGS_END_HEADERS
+ head.stream_id = stream_id;
+ butil::IOBufBytesIterator it(payload);
+ return sctx->OnHeaders(it, head, payload.size(), 0);
+}
+
+TEST_F(HttpTest, h2_too_many_headers) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_header_count = 8;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
Review Comment:
Using `CHECK_EQ` in unit tests aborts the entire test process on failure,
which can hide subsequent failures and reduce diagnosability. Prefer
`ASSERT_EQ` (or `ASSERT_*`) here so failures are reported by gtest without
terminating the whole suite.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -1940,6 +1942,231 @@ TEST_F(HttpTest,
h2_header_list_budget_resets_per_block) {
delete sctx;
}
+// Literal header field with a new name, with both lengths in a single 7-bit
+// prefix octet. `first_octet` selects the representation: 0x00 is "without
+// indexing" (RFC 7541 6.2.2), 0x40 is "with incremental indexing" (6.2.1)
+// which also adds the field to the dynamic table.
+// 0x80 of a length octet is the Huffman flag and a length of 128 or more needs
+// the multi-octet form, so refuse what does not fit instead of emitting a
+// corrupt header block.
+void AppendLiteralHeader(butil::IOBuf* out, const std::string& name,
+ const std::string& value, uint8_t first_octet = 0x00)
{
+ ASSERT_LT(name.size(), 0x80u);
+ ASSERT_LT(value.size(), 0x80u);
+ uint8_t prefix[] = { first_octet, (uint8_t)name.size() };
+ out->append(prefix, sizeof(prefix));
+ out->append(name);
+ uint8_t value_len = (uint8_t)value.size();
+ out->append(&value_len, 1);
+ out->append(value);
+}
+
+// Feed `payload` to `sctx` as one complete HEADERS block, the way
+// H2Context::Consume() would. A non-zero stream_id in the result means the
+// frame handler asked for a RST_STREAM, a zero one means a GOAWAY that closes
+// the whole connection.
+brpc::policy::H2ParseResult ConsumeHeadersBlock(
+ brpc::policy::H2StreamContext* sctx, const butil::IOBuf& payload,
+ int stream_id) {
+ brpc::policy::H2FrameHead head;
+ head.payload_size = payload.size();
+ head.type = brpc::policy::H2_FRAME_HEADERS;
+ head.flags = 0x4; // H2_FLAGS_END_HEADERS
+ head.stream_id = stream_id;
+ butil::IOBufBytesIterator it(payload);
+ return sctx->OnHeaders(it, head, payload.size(), 0);
+}
+
+TEST_F(HttpTest, h2_too_many_headers) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_header_count = 8;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
+ _socket->initialize_parsing_context(&ctx);
+
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 1);
+ butil::IOBuf payload;
+ for (int i = 0; i < 8; ++i) {
+ AppendLiteralHeader(&payload, "h" + std::to_string(i), "v");
+ }
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 1);
+ ASSERT_TRUE(res.is_ok()) << brpc::H2ErrorToString(res.error());
+ ASSERT_EQ(8u, sctx->header().HeaderCount());
+ }
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 3);
+ butil::IOBuf payload;
+ for (int i = 0; i < 9; ++i) {
+ AppendLiteralHeader(&payload, "h" + std::to_string(i), "v");
+ }
+ // Refusing the request must not cost the connection its other
+ // streams, so the frame handler asks for a RST_STREAM (non-zero
+ // stream_id) rather than a GOAWAY.
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 3);
+ ASSERT_FALSE(res.is_ok());
+ ASSERT_EQ(brpc::H2_ENHANCE_YOUR_CALM, res.error());
+ ASSERT_EQ(3, res.stream_id());
+ }
+}
+
+TEST_F(HttpTest, h2_too_many_queries_in_path) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_query_count = 4;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
Review Comment:
Using `CHECK_EQ` in unit tests aborts the entire test process on failure,
which can hide subsequent failures and reduce diagnosability. Prefer
`ASSERT_EQ` (or `ASSERT_*`) here so failures are reported by gtest without
terminating the whole suite.
##########
src/brpc/uri.cpp:
##########
@@ -17,25 +17,25 @@
#include <ctype.h> // isalnum
-
#include <unordered_set>
-
+#include <gflags/gflags.h>
#include "brpc/log.h"
#include "brpc/details/http_parser.h" // http_parser_parse_url
#include "brpc/uri.h" // URI
namespace brpc {
+DEFINE_uint32(http_max_query_count, 1000,
+ "Reject an URL carrying more than so many query parameters. "
Review Comment:
Grammatical fix: use 'a URL' instead of 'an URL' in the flag description
string.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -1940,6 +1942,231 @@ TEST_F(HttpTest,
h2_header_list_budget_resets_per_block) {
delete sctx;
}
+// Literal header field with a new name, with both lengths in a single 7-bit
+// prefix octet. `first_octet` selects the representation: 0x00 is "without
+// indexing" (RFC 7541 6.2.2), 0x40 is "with incremental indexing" (6.2.1)
+// which also adds the field to the dynamic table.
+// 0x80 of a length octet is the Huffman flag and a length of 128 or more needs
+// the multi-octet form, so refuse what does not fit instead of emitting a
+// corrupt header block.
+void AppendLiteralHeader(butil::IOBuf* out, const std::string& name,
+ const std::string& value, uint8_t first_octet = 0x00)
{
+ ASSERT_LT(name.size(), 0x80u);
+ ASSERT_LT(value.size(), 0x80u);
+ uint8_t prefix[] = { first_octet, (uint8_t)name.size() };
+ out->append(prefix, sizeof(prefix));
+ out->append(name);
+ uint8_t value_len = (uint8_t)value.size();
+ out->append(&value_len, 1);
+ out->append(value);
+}
+
+// Feed `payload` to `sctx` as one complete HEADERS block, the way
+// H2Context::Consume() would. A non-zero stream_id in the result means the
+// frame handler asked for a RST_STREAM, a zero one means a GOAWAY that closes
+// the whole connection.
+brpc::policy::H2ParseResult ConsumeHeadersBlock(
+ brpc::policy::H2StreamContext* sctx, const butil::IOBuf& payload,
+ int stream_id) {
+ brpc::policy::H2FrameHead head;
+ head.payload_size = payload.size();
+ head.type = brpc::policy::H2_FRAME_HEADERS;
+ head.flags = 0x4; // H2_FLAGS_END_HEADERS
+ head.stream_id = stream_id;
+ butil::IOBufBytesIterator it(payload);
+ return sctx->OnHeaders(it, head, payload.size(), 0);
+}
+
+TEST_F(HttpTest, h2_too_many_headers) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_header_count = 8;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
+ _socket->initialize_parsing_context(&ctx);
+
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 1);
+ butil::IOBuf payload;
+ for (int i = 0; i < 8; ++i) {
+ AppendLiteralHeader(&payload, "h" + std::to_string(i), "v");
+ }
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 1);
+ ASSERT_TRUE(res.is_ok()) << brpc::H2ErrorToString(res.error());
+ ASSERT_EQ(8u, sctx->header().HeaderCount());
+ }
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 3);
+ butil::IOBuf payload;
+ for (int i = 0; i < 9; ++i) {
+ AppendLiteralHeader(&payload, "h" + std::to_string(i), "v");
+ }
+ // Refusing the request must not cost the connection its other
+ // streams, so the frame handler asks for a RST_STREAM (non-zero
+ // stream_id) rather than a GOAWAY.
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 3);
+ ASSERT_FALSE(res.is_ok());
+ ASSERT_EQ(brpc::H2_ENHANCE_YOUR_CALM, res.error());
+ ASSERT_EQ(3, res.stream_id());
+ }
+}
+
+TEST_F(HttpTest, h2_too_many_queries_in_path) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_query_count = 4;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
+ _socket->initialize_parsing_context(&ctx);
+
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 1);
+ butil::IOBuf payload;
+ AppendLiteralHeader(&payload, ":path", "/s?a=1&b=2&c=3&d=4");
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 1);
+ ASSERT_TRUE(res.is_ok()) << brpc::H2ErrorToString(res.error());
+ }
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 3);
+ butil::IOBuf payload;
+ AppendLiteralHeader(&payload, ":path", "/s?a=1&b=2&c=3&d=4&e=5");
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 3);
+ ASSERT_FALSE(res.is_ok());
+ ASSERT_EQ(brpc::H2_ENHANCE_YOUR_CALM, res.error());
+ ASSERT_EQ(3, res.stream_id());
+ }
+}
+
+// A refused header block still has to be fed to the HPACK decoder in full.
+// The dynamic table belongs to the connection, so dropping the tail of a block
+// would leave it out of step with the encoding table of the peer and turn
+// every later block into garbage, which is why RFC 9113 section 10.5.1 says
+// the field block MUST be processed unless the connection is closed.
+TEST_F(HttpTest, h2_refused_header_block_keeps_hpack_in_sync) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_header_count = 2;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
+ _socket->initialize_parsing_context(&ctx);
+
+ // Four headers with incremental indexing, two of them past the limit.
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 1);
+ butil::IOBuf payload;
+ AppendLiteralHeader(&payload, "a", "1", 0x40);
+ AppendLiteralHeader(&payload, "b", "2", 0x40);
+ AppendLiteralHeader(&payload, "c", "3", 0x40);
+ AppendLiteralHeader(&payload, "d", "4", 0x40);
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 1);
+ ASSERT_FALSE(res.is_ok());
+ ASSERT_EQ(brpc::H2_ENHANCE_YOUR_CALM, res.error());
+ ASSERT_EQ(1, res.stream_id());
+ // Everything after the offending field is decoded but thrown away.
+ ASSERT_EQ(3u, sctx->header().HeaderCount());
+
+ // The static table ends at index 61, so 62 names the newest dynamic entry.
+ // That is "d" only because decoding ran to the end of the block; had it
+ // stopped at the limit, 62 would still be "c".
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx2(
+ new brpc::policy::H2StreamContext(false));
+ sctx2->Init(ctx, 3);
+ butil::IOBuf indexed;
+ const uint8_t indexed_field[] = { 0x80 | 62 }; // Indexed Header Field
+ indexed.append(indexed_field, sizeof(indexed_field));
+ brpc::policy::H2ParseResult res2 =
+ ConsumeHeadersBlock(sctx2.get(), indexed, 3);
+ ASSERT_TRUE(res2.is_ok()) << brpc::H2ErrorToString(res2.error());
+ const std::string* value = sctx2->header().GetHeader("d");
+ ASSERT_TRUE(value != nullptr);
+ ASSERT_EQ("4", *value);
+}
+
+// RFC 9113 section 8.1.1: "Malformed requests or responses that are detected
+// MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR."
+// A bad pseudo-header says nothing about the health of the connection, so it
+// must not cost the other streams theirs. :path has its own case table in
+// HttpTest.http2_reject_path_not_starting_with_slash.
+TEST_F(HttpTest, h2_malformed_pseudo_header_resets_stream_only) {
+ struct MalformedField {
+ const char* name;
+ const char* value;
+ };
+ MalformedField malformed[] = {
+ { ":method", "NOSUCH" },
+ { ":status", "20x" },
+ { ":nosuchheader", "1" }, // 8.3: undefined pseudo-header
+ };
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
Review Comment:
Using `CHECK_EQ` in unit tests aborts the entire test process on failure,
which can hide subsequent failures and reduce diagnosability. Prefer
`ASSERT_EQ` (or `ASSERT_*`) here so failures are reported by gtest without
terminating the whole suite.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -1940,6 +1942,231 @@ TEST_F(HttpTest,
h2_header_list_budget_resets_per_block) {
delete sctx;
}
+// Literal header field with a new name, with both lengths in a single 7-bit
+// prefix octet. `first_octet` selects the representation: 0x00 is "without
+// indexing" (RFC 7541 6.2.2), 0x40 is "with incremental indexing" (6.2.1)
+// which also adds the field to the dynamic table.
+// 0x80 of a length octet is the Huffman flag and a length of 128 or more needs
+// the multi-octet form, so refuse what does not fit instead of emitting a
+// corrupt header block.
+void AppendLiteralHeader(butil::IOBuf* out, const std::string& name,
+ const std::string& value, uint8_t first_octet = 0x00)
{
+ ASSERT_LT(name.size(), 0x80u);
+ ASSERT_LT(value.size(), 0x80u);
+ uint8_t prefix[] = { first_octet, (uint8_t)name.size() };
+ out->append(prefix, sizeof(prefix));
+ out->append(name);
+ uint8_t value_len = (uint8_t)value.size();
+ out->append(&value_len, 1);
+ out->append(value);
+}
+
+// Feed `payload` to `sctx` as one complete HEADERS block, the way
+// H2Context::Consume() would. A non-zero stream_id in the result means the
+// frame handler asked for a RST_STREAM, a zero one means a GOAWAY that closes
+// the whole connection.
+brpc::policy::H2ParseResult ConsumeHeadersBlock(
+ brpc::policy::H2StreamContext* sctx, const butil::IOBuf& payload,
+ int stream_id) {
+ brpc::policy::H2FrameHead head;
+ head.payload_size = payload.size();
+ head.type = brpc::policy::H2_FRAME_HEADERS;
+ head.flags = 0x4; // H2_FLAGS_END_HEADERS
+ head.stream_id = stream_id;
+ butil::IOBufBytesIterator it(payload);
+ return sctx->OnHeaders(it, head, payload.size(), 0);
+}
+
+TEST_F(HttpTest, h2_too_many_headers) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_header_count = 8;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
+ _socket->initialize_parsing_context(&ctx);
+
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 1);
+ butil::IOBuf payload;
+ for (int i = 0; i < 8; ++i) {
+ AppendLiteralHeader(&payload, "h" + std::to_string(i), "v");
+ }
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 1);
+ ASSERT_TRUE(res.is_ok()) << brpc::H2ErrorToString(res.error());
+ ASSERT_EQ(8u, sctx->header().HeaderCount());
+ }
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 3);
+ butil::IOBuf payload;
+ for (int i = 0; i < 9; ++i) {
+ AppendLiteralHeader(&payload, "h" + std::to_string(i), "v");
+ }
+ // Refusing the request must not cost the connection its other
+ // streams, so the frame handler asks for a RST_STREAM (non-zero
+ // stream_id) rather than a GOAWAY.
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 3);
+ ASSERT_FALSE(res.is_ok());
+ ASSERT_EQ(brpc::H2_ENHANCE_YOUR_CALM, res.error());
+ ASSERT_EQ(3, res.stream_id());
+ }
+}
+
+TEST_F(HttpTest, h2_too_many_queries_in_path) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_query_count = 4;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
+ _socket->initialize_parsing_context(&ctx);
+
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 1);
+ butil::IOBuf payload;
+ AppendLiteralHeader(&payload, ":path", "/s?a=1&b=2&c=3&d=4");
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 1);
+ ASSERT_TRUE(res.is_ok()) << brpc::H2ErrorToString(res.error());
+ }
+ {
+ std::unique_ptr<brpc::policy::H2StreamContext> sctx(
+ new brpc::policy::H2StreamContext(false));
+ sctx->Init(ctx, 3);
+ butil::IOBuf payload;
+ AppendLiteralHeader(&payload, ":path", "/s?a=1&b=2&c=3&d=4&e=5");
+ brpc::policy::H2ParseResult res =
+ ConsumeHeadersBlock(sctx.get(), payload, 3);
+ ASSERT_FALSE(res.is_ok());
+ ASSERT_EQ(brpc::H2_ENHANCE_YOUR_CALM, res.error());
+ ASSERT_EQ(3, res.stream_id());
+ }
+}
+
+// A refused header block still has to be fed to the HPACK decoder in full.
+// The dynamic table belongs to the connection, so dropping the tail of a block
+// would leave it out of step with the encoding table of the peer and turn
+// every later block into garbage, which is why RFC 9113 section 10.5.1 says
+// the field block MUST be processed unless the connection is closed.
+TEST_F(HttpTest, h2_refused_header_block_keeps_hpack_in_sync) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_http_max_header_count = 2;
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(_socket.get(), nullptr);
+ CHECK_EQ(ctx->Init(), 0);
Review Comment:
Using `CHECK_EQ` in unit tests aborts the entire test process on failure,
which can hide subsequent failures and reduce diagnosability. Prefer
`ASSERT_EQ` (or `ASSERT_*`) here so failures are reported by gtest without
terminating the whole suite.
##########
src/brpc/details/http_message.cpp:
##########
@@ -131,6 +134,14 @@ int HttpMessage::on_header_value(http_parser *parser,
http_message->_cur_value =
&header.AddHeader(http_message->_cur_header);
}
+
+ if (FLAGS_http_max_header_count > 0 &&
+ header.HeaderCount() > FLAGS_http_max_header_count) {
+ LOG(ERROR) << "Too many headers, max="
+ << FLAGS_http_max_header_count;
+ return -1;
+ }
Review Comment:
As in the HTTP/2 path, the check happens after adding the new header, so an
over-limit message still allocates/stores at least one extra header before
failing. If the goal is to strictly cap work/memory on the hot parsing path,
consider checking `HeaderCount()` *before* inserting (or using a pre-increment
check) so the over-limit header is never added.
--
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]
