Copilot commented on code in PR #3524:
URL: https://github.com/apache/brpc/pull/3524#discussion_r3951528220
##########
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 source-level breaking
public API change for any downstream callers. If this project needs to preserve
compatibility, consider keeping a `void SetH2Path(...)` wrapper (that calls the
new implementation and discards the result) under a new name for the
status-returning variant (e.g., `TrySetH2Path`), or introduce an overload that
preserves the old signature while still setting `status()` on failure.
##########
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:
Logging at `ERROR` on malformed/over-limit inputs can become a
log-amplification vector under active probing/DoS (especially since this
executes on the parse hot path). Consider rate-limiting (e.g., `LOG_EVERY_N` /
throttled logging) and/or lowering severity (e.g., `WARNING`) for client-caused
rejections while still returning the error; apply the same approach to the
HTTP/2 path where similar `LOG(ERROR)` lines were added.
##########
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);
+}
Review Comment:
Two test-helper concerns here: (1) these free functions are in the global
namespace, which increases the chance of symbol collisions across test
translation units—prefer placing them in an anonymous namespace (or as
`static`) in this file; (2) `ASSERT_*` inside helper functions is a known gtest
footgun because a fatal assertion only returns from the helper, not the calling
test body, which can lead to follow-on crashes/noisy failures. Prefer
`EXPECT_*` here or return a `testing::AssertionResult` and `ASSERT_TRUE(...)`
in the test.
##########
src/brpc/socket.cpp:
##########
@@ -794,6 +794,7 @@ int Socket::OnCreated(const SocketOptions& options) {
_unwritten_bytes.store(0, butil::memory_order_relaxed);
_keepalive_options = options.keepalive_options;
_tcp_user_timeout_ms = options.tcp_user_timeout_ms;
+ _http_request_method = HTTP_METHOD_GET;
Review Comment:
This initialization of `_http_request_method` appears unrelated to the
stated PR goal (bounding headers/queries as HashDoS mitigations). If it’s
required for the new behavior (or fixes an uninitialized-read), it would help
to either (a) document the linkage in the PR description/commit message, or (b)
move this change into a separate PR to keep the mitigation change focused.
##########
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:
Grammar: change 'Reject an URL' to 'Reject a URL' in the flag description.
--
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]
