You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "songgaoye (via GitHub)" <gi...@apache.org> on 2026/10/14 11:02:25 UTC
[PR] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
songgaoye opened a new pull request, #3606:
URL: https://github.com/apache/brpc/pull/3606
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
`HttpHeader` caches the first `Set-Cookie` value as a pointer into its
`HeaderMap`. The pointer becomes stale when the map grows, and it points to
the wrong header after `HttpHeader::Swap()`. `Clear()` also leaves the cached
pointer stale.
### What is changed and the side effects?
Changed:
Refresh the cached first `Set-Cookie` pointer after `HeaderMap` growth and
`HttpHeader::Swap()`, and clear it in `HttpHeader::Clear()`.
Add regression coverage for map growth, `Swap()`, and `Clear()`.
Side effects:
* Performance effects:
* Breaking backward compatibility:
* * *
### Check List:
* Please make sure your changes are compilable.
* When providing us with a new feature, it is best to add related tests.
* Please follow [Contributor Covenant Code of Conduct](https://www.contributor-covenant.org/version/1/4/code-of-conduct/).
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4272201133
##########
src/brpc/http_header.cpp:
##########
@@ -136,22 +139,34 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
std::string* val = _headers.seek(key);
if (nullptr == val) {
+ const size_t old_bucket_count = _headers.bucket_count();
val = _headers.insert({ key, "" });
if (is_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
Review Comment:
the duplicated three-line rehash check keeps each mutation
path explicit
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4279431415
##########
src/brpc/http_header.cpp:
##########
@@ -136,22 +139,34 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
std::string* val = _headers.seek(key);
if (nullptr == val) {
+ const size_t old_bucket_count = _headers.bucket_count();
val = _headers.insert({ key, "" });
if (is_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
+ const size_t old_bucket_count = _headers.bucket_count();
std::string* val = _headers.insert({ key, "" });
if (IsSetCookie(key) && nullptr == _first_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
return *val;
}
+void HttpHeader::RefreshFirstSetCookie() {
+ _first_set_cookie = _headers.seek(SET_COOKIE);
Review Comment:
Good catch!
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4290679230
##########
src/brpc/details/http_message.cpp:
##########
@@ -186,6 +186,9 @@ int HttpMessage::on_headers_complete(http_parser *parser) {
parser->http_major = 1;
}
HttpHeader& headers = http_message->header();
+ if (headers.HasSetCookieHeaders()) {
+ headers.SyncSetCookieHeaders();
+ }
Review Comment:
`HttpMessage` calls `HttpHeader::HasSetCookieHeaders()` and `HttpHeader::SyncSetCookieHeaders()`. In `http_header.h` these helpers appear to be internal (defined alongside other non-public helpers), so this may fail to compile due to access control. Make these APIs publicly accessible in a controlled way (e.g., a public `FinalizeHeadersParsing()` that performs the sync), or explicitly grant friendship to the caller that needs it.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -879,9 +900,65 @@ TEST(HttpMessageTest, http_header) {
}
ASSERT_EQ(1, set_cookie_value2_count);
ASSERT_EQ(1, set_cookie_value3_count);
+ brpc::HttpHeader copied_header(header);
+ copied_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = copied_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ brpc::HttpHeader assigned_header;
+ assigned_header = header;
+ assigned_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = assigned_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
header.RemoveHeader(brpc::HttpHeader::SET_COOKIE);
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
- ASSERT_EQ(header._first_set_cookie, nullptr);
+ ASSERT_TRUE(header.GetAllSetCookieHeader().empty());
+
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ const std::string* first_set_cookie_after_reset =
+ header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_NE(nullptr, first_set_cookie_after_reset);
+ header.AppendHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ const size_t bucket_count = header._headers.bucket_count();
+ for (int i = 0; i < 4096 &&
+ header._headers.bucket_count() == bucket_count; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ ASSERT_NE(bucket_count, header._headers.bucket_count());
Review Comment:
This test forces a `HeaderMap` growth by inserting up to 4096 headers and asserting `bucket_count()` changes. That assumption can be implementation-dependent (initial bucket counts / growth factors can vary), which may make the test flaky across platforms or future container changes. A more robust approach is to force rehash explicitly (if the container supports it), or to loop until a growth condition that is guaranteed by the container API, while keeping an upper bound and asserting based on an explicit rehash trigger rather than a hardcoded insert count.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -879,9 +900,65 @@ TEST(HttpMessageTest, http_header) {
}
ASSERT_EQ(1, set_cookie_value2_count);
ASSERT_EQ(1, set_cookie_value3_count);
+ brpc::HttpHeader copied_header(header);
+ copied_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = copied_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ brpc::HttpHeader assigned_header;
+ assigned_header = header;
+ assigned_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = assigned_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
header.RemoveHeader(brpc::HttpHeader::SET_COOKIE);
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
- ASSERT_EQ(header._first_set_cookie, nullptr);
+ ASSERT_TRUE(header.GetAllSetCookieHeader().empty());
+
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ const std::string* first_set_cookie_after_reset =
+ header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_NE(nullptr, first_set_cookie_after_reset);
+ header.AppendHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ const size_t bucket_count = header._headers.bucket_count();
+ for (int i = 0; i < 4096 &&
+ header._headers.bucket_count() == bucket_count; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ ASSERT_NE(bucket_count, header._headers.bucket_count());
+ ASSERT_EQ(set_cookie_value1, *first_set_cookie_after_reset);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ all_set_cookie = header.GetAllSetCookieHeader();
+ ASSERT_EQ(2u, all_set_cookie.size());
+ ASSERT_EQ(set_cookie_value1, *all_set_cookie[0]);
+ ASSERT_EQ(set_cookie_value2, *all_set_cookie[1]);
+ int set_cookie_value1_count = 0;
+ set_cookie_value2_count = 0;
Review Comment:
The counting variables are handled inconsistently here: `set_cookie_value1_count` is declared locally, but `set_cookie_value2_count` is assigned without a local declaration in this block, which makes the code harder to follow and more sensitive to surrounding scope changes. Prefer declaring both counters in the same local scope (and similarly for any others) to keep the test self-contained and avoid accidental reuse of earlier variables.
##########
src/brpc/http_header.cpp:
##########
@@ -128,28 +161,29 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
return _content_type;
}
- bool is_set_cookie = IsSetCookie(key);
- // Only returns the first Set-Cookie header field for compatibility.
- if (is_set_cookie && nullptr != _first_set_cookie) {
- return *_first_set_cookie;
- }
-
std::string* val = _headers.seek(key);
if (nullptr == val) {
val = _headers.insert({ key, "" });
- if (is_set_cookie) {
- _first_set_cookie = val;
- }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
- std::string* val = _headers.insert({ key, "" });
- if (IsSetCookie(key) && nullptr == _first_set_cookie) {
- _first_set_cookie = val;
+ if (!_set_cookie_headers) {
+ _set_cookie_headers.reset(new SetCookieHeaders);
+ }
+ _set_cookie_headers->push_back(std::make_pair(key, std::string()));
+ SyncSetCookieHeaders();
+ return _set_cookie_headers->back().second;
+}
Review Comment:
`AddHeader()` now unconditionally appends into `_set_cookie_headers` and syncs `SET_COOKIE`, regardless of `key`. If `AddHeader()` is ever used for non-`Set-Cookie` headers, this will incorrectly route those headers into the `Set-Cookie` store and also mutate the `SET_COOKIE` entries in `_headers`. Fix by making `AddHeader()` conditional: only use `_set_cookie_headers` when `IsSetCookie(key)` is true; otherwise preserve the prior behavior of inserting into `_headers` and returning that value slot. Alternatively, split into `AddSetCookieHeader()` (new) and keep `AddHeader()` generic.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4270433018
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
Review Comment:
This test intends to exercise `HeaderMap` growth/rehash, but inserting 128 headers may not reliably trigger a rehash depending on the map's initial bucket count and growth policy, making the regression coverage flaky/ineffective. Consider asserting that a rehash actually occurred (e.g., by tracking bucket count before/after if accessible in the test) or looping until bucket count changes with a reasonable upper bound.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+
+ brpc::HttpHeader other_header;
+ other_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ header.Swap(other_header);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
+ value = other_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ header.Clear();
+ ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
Review Comment:
Since the regression includes `Clear()` leaving `_first_set_cookie` stale, this test should assert the internal cached pointer is reset (e.g., `ASSERT_EQ(header._first_set_cookie, nullptr);`) in addition to verifying `GetHeader(SET_COOKIE)` is false. Otherwise the specific bug fix in `Clear()` isn't directly validated.
##########
src/brpc/http_header.cpp:
##########
@@ -136,22 +139,34 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
std::string* val = _headers.seek(key);
if (nullptr == val) {
+ const size_t old_bucket_count = _headers.bucket_count();
val = _headers.insert({ key, "" });
if (is_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
Review Comment:
The `old_bucket_count` + `bucket_count()` comparison + refresh pattern is duplicated (also in `AddHeader`). Consider extracting a small helper (e.g., `MaybeRefreshFirstSetCookieOnRehash(old_bucket_count)`) to keep the refresh-on-rehash behavior consistent and easier to update.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
Review Comment:
Indentation inside the loop is inconsistent with surrounding lines in this block. Aligning indentation to the file's prevailing style improves readability and avoids style-only diffs later.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4307412240
##########
src/brpc/http_header.cpp:
##########
@@ -128,28 +170,39 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
return _content_type;
}
- bool is_set_cookie = IsSetCookie(key);
- // Only returns the first Set-Cookie header field for compatibility.
- if (is_set_cookie && nullptr != _first_set_cookie) {
- return *_first_set_cookie;
- }
-
std::string* val = _headers.seek(key);
if (nullptr == val) {
val = _headers.insert({ key, "" });
- if (is_set_cookie) {
- _first_set_cookie = val;
- }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
- std::string* val = _headers.insert({ key, "" });
- if (IsSetCookie(key) && nullptr == _first_set_cookie) {
- _first_set_cookie = val;
+ if (!IsSetCookie(key)) {
+ return *_headers.insert({ key, "" });
}
- return *val;
+ if (!_set_cookie_headers) {
+ _set_cookie_headers.reset(new SetCookieHeaders);
+ }
+ if (!_set_cookie_headers_dirty) {
+ _headers.erase(SET_COOKIE);
+ _set_cookie_headers_dirty = true;
+ }
+ _set_cookie_headers->push_back(std::make_pair(key, std::string()));
+ return _set_cookie_headers->back().second;
+}
+
+void HttpHeader::SyncSetCookieHeaders() {
+ if (!_set_cookie_headers_dirty) {
+ return;
+ }
+ _headers.erase(SET_COOKIE);
Review Comment:
`AppendHeader()` now inserts incrementally, but `SetHeader()` still forces a full synchronization. With the default 16-bucket map, the following sequence reaches the unsafe deletion path:
```cpp
brpc::HttpHeader h;
h.SetHeader("Set-Cookie", "sid=" + std::string(128, 'x'));
h.SetHeader("User-Agent", "client");
h.SetHeader("Set-Cookie", "sid=new");
```
`Set-Cookie` and `User-Agent` collide. During the final update, `_headers.erase(SET_COOKIE)` removes the Cookie bucket head while retaining the `User-Agent` node.
`MultiFlatMap::erase()` destroys the head’s strings at `flat_map_inl.h:473`, then assigns the surviving node into those destroyed objects at lines 501–503 without reconstructing the element. With a heap-backed Cookie value, this can cause a double-free or heap corruption.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4305233396
##########
src/brpc/http_header.cpp:
##########
@@ -128,28 +161,32 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
return _content_type;
}
- bool is_set_cookie = IsSetCookie(key);
- // Only returns the first Set-Cookie header field for compatibility.
- if (is_set_cookie && nullptr != _first_set_cookie) {
- return *_first_set_cookie;
- }
-
std::string* val = _headers.seek(key);
if (nullptr == val) {
val = _headers.insert({ key, "" });
- if (is_set_cookie) {
- _first_set_cookie = val;
- }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
- std::string* val = _headers.insert({ key, "" });
- if (IsSetCookie(key) && nullptr == _first_set_cookie) {
- _first_set_cookie = val;
+ if (!IsSetCookie(key)) {
+ return *_headers.insert({ key, "" });
+ }
+ if (!_set_cookie_headers) {
+ _set_cookie_headers.reset(new SetCookieHeaders);
+ }
+ _set_cookie_headers->push_back(std::make_pair(key, std::string()));
+ SyncSetCookieHeaders();
+ return _set_cookie_headers->back().second;
+}
+
+void HttpHeader::SyncSetCookieHeaders() {
+ _headers.erase(SET_COOKIE);
+ if (_set_cookie_headers) {
+ for (const auto& header : *_set_cookie_headers) {
+ _headers.insert(header);
+ }
Review Comment:
Good catch! fix it.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4272169763
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+
+ brpc::HttpHeader other_header;
+ other_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ header.Swap(other_header);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
+ value = other_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ header.Clear();
+ ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
Review Comment:
This is already covered by the preceding Clear()
test in the same test case:
ASSERT_EQ(header._first_set_cookie, nullptr);
It directly verifies that Clear() resets the cached pointer after a
Set-Cookie header has been added, so an additional assertion after the
Swap() scenario would be redundant.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4272169763
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+
+ brpc::HttpHeader other_header;
+ other_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ header.Swap(other_header);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
+ value = other_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ header.Clear();
+ ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
Review Comment:
Thanks for the suggestion. This is already covered by the preceding Clear()
test in the same test case:
ASSERT_EQ(header._first_set_cookie, nullptr);
It directly verifies that Clear() resets the cached pointer after a
Set-Cookie header has been added, so an additional assertion after the
Swap() scenario would be redundant.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4279431415
##########
src/brpc/http_header.cpp:
##########
@@ -136,22 +139,34 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
std::string* val = _headers.seek(key);
if (nullptr == val) {
+ const size_t old_bucket_count = _headers.bucket_count();
val = _headers.insert({ key, "" });
if (is_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
+ const size_t old_bucket_count = _headers.bucket_count();
std::string* val = _headers.insert({ key, "" });
if (IsSetCookie(key) && nullptr == _first_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
return *val;
}
+void HttpHeader::RefreshFirstSetCookie() {
+ _first_set_cookie = _headers.seek(SET_COOKIE);
Review Comment:
Good catch
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3606:
URL: https://github.com/apache/brpc/pull/3606
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4291330512
##########
src/brpc/details/http_message.cpp:
##########
@@ -186,6 +186,9 @@ int HttpMessage::on_headers_complete(http_parser *parser) {
parser->http_major = 1;
}
HttpHeader& headers = http_message->header();
+ if (headers.HasSetCookieHeaders()) {
+ headers.SyncSetCookieHeaders();
+ }
Review Comment:
This is a false positive. `HttpMessage` is already declared as a friend of `HttpHeader`, so it can access these private helpers. No API visibility change is needed.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on PR #3606:
URL: https://github.com/apache/brpc/pull/3606#issuecomment-6183466030
The CI failure is unrelated to this PR. It was fixed in https://github.com/apache/brpc/pull/3610
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4278835538
##########
src/brpc/http_header.cpp:
##########
@@ -136,22 +139,34 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
std::string* val = _headers.seek(key);
if (nullptr == val) {
+ const size_t old_bucket_count = _headers.bucket_count();
val = _headers.insert({ key, "" });
if (is_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
+ const size_t old_bucket_count = _headers.bucket_count();
std::string* val = _headers.insert({ key, "" });
if (IsSetCookie(key) && nullptr == _first_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
return *val;
}
+void HttpHeader::RefreshFirstSetCookie() {
+ _first_set_cookie = _headers.seek(SET_COOKIE);
Review Comment:
`HeaderMap` does not preserve insertion order within collision chains, so `seek(SET_COOKIE)` may select a different cookie.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4302310603
##########
src/brpc/http_header.cpp:
##########
@@ -128,28 +161,32 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
return _content_type;
}
- bool is_set_cookie = IsSetCookie(key);
- // Only returns the first Set-Cookie header field for compatibility.
- if (is_set_cookie && nullptr != _first_set_cookie) {
- return *_first_set_cookie;
- }
-
std::string* val = _headers.seek(key);
if (nullptr == val) {
val = _headers.insert({ key, "" });
- if (is_set_cookie) {
- _first_set_cookie = val;
- }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
- std::string* val = _headers.insert({ key, "" });
- if (IsSetCookie(key) && nullptr == _first_set_cookie) {
- _first_set_cookie = val;
+ if (!IsSetCookie(key)) {
+ return *_headers.insert({ key, "" });
+ }
+ if (!_set_cookie_headers) {
+ _set_cookie_headers.reset(new SetCookieHeaders);
+ }
+ _set_cookie_headers->push_back(std::make_pair(key, std::string()));
+ SyncSetCookieHeaders();
+ return _set_cookie_headers->back().second;
+}
+
+void HttpHeader::SyncSetCookieHeaders() {
+ _headers.erase(SET_COOKIE);
+ if (_set_cookie_headers) {
+ for (const auto& header : *_set_cookie_headers) {
+ _headers.insert(header);
+ }
Review Comment:
`SyncSetCookieHeaders()` rebuilds every `Set-Cookie` entry, and it is called after each append/add. Constructing or parsing n cookies therefore performs O(n²) copying and map work, unlike the previous one-entry insertion path. Keep the map update incremental and/or defer one full synchronization until header parsing finishes (for example with a dirty flag) so multi-cookie responses do not pay this quadratic cost.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4291411797
##########
src/brpc/http_header.cpp:
##########
@@ -128,28 +161,29 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) {
return _content_type;
}
- bool is_set_cookie = IsSetCookie(key);
- // Only returns the first Set-Cookie header field for compatibility.
- if (is_set_cookie && nullptr != _first_set_cookie) {
- return *_first_set_cookie;
- }
-
std::string* val = _headers.seek(key);
if (nullptr == val) {
val = _headers.insert({ key, "" });
- if (is_set_cookie) {
- _first_set_cookie = val;
- }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
- std::string* val = _headers.insert({ key, "" });
- if (IsSetCookie(key) && nullptr == _first_set_cookie) {
- _first_set_cookie = val;
+ if (!_set_cookie_headers) {
+ _set_cookie_headers.reset(new SetCookieHeaders);
+ }
+ _set_cookie_headers->push_back(std::make_pair(key, std::string()));
+ SyncSetCookieHeaders();
+ return _set_cookie_headers->back().second;
+}
Review Comment:
apply it.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4366186113
##########
src/brpc/http_header.h:
##########
@@ -202,7 +215,13 @@ friend void policy::ProcessHttpRequest(InputMessageBase *msg);
std::string _content_type;
std::string _unresolved_path;
std::pair<int, int> _version;
- std::string* _first_set_cookie;
+ // HeaderMap rehashing invalidates value pointers and does not preserve
+ // duplicate-key order. deque keeps existing value pointers stable when
+ // another Set-Cookie header is appended.
+ typedef std::deque<std::pair<std::string, std::string> > SetCookieHeaders;
Review Comment:
Consider simplifying `SetCookieHeaders` to `std::deque<std::string>`. All entries represent `Set-Cookie` headers, so storing the key for each entry is redundant. `SyncSetCookieHeaders()` can use the existing `SET_COOKIE` constant when inserting values into `_headers`.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "songgaoye (via GitHub)" <gi...@apache.org>.
songgaoye commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4272195399
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
Review Comment:
The reported loop indentation is already consistent with the
surrounding 4-space style.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3606:
URL: https://github.com/apache/brpc/pull/3606#issuecomment-6342163167
[replied by brpc-oncall robot] Agreed that this sequence is unsafe, and thanks for the precise reproducer.
The corruption does not come from the new `Set-Cookie` storage itself, but from `butil::FlatMap`'s multi-map `erase()`: when the erased key owns the bucket head and a colliding entry survives, the old implementation destroys the head element and then move-assigns the surviving node into that already-destroyed object. With the default 16 buckets, `set-cookie` and `user-agent` both land in bucket 3, so the sequence above hits exactly that path (this was also latently reachable before this PR, but only through `RemoveHeader()`).
Two follow-ups:
1. This defect is already fixed on master by #3617 (the head element is no longer destroyed when a survivor is moved up). This branch's base predates that fix — the code quoted above (`flat_map_inl.h:473` destroying the head unconditionally) is the pre-fix version. Note that CI builds the PR's merge ref against current master, so the current CI run cannot expose it. Please rebase/merge master so the new `_headers.erase(SET_COOKIE)` calls in `AddHeader()` and `SyncSetCookieHeaders()` run on the fixed implementation; no workaround is needed inside `http_header.cpp`.
2. Since `SetHeader()`/`SyncSetCookieHeaders()` now trigger `_headers.erase(SET_COOKIE)` on every cookie update (previously only `RemoveHeader()` did), this collision path becomes far more common. Please add the sequence above as a regression test in `test/brpc_http_message_unittest.cpp` (Set-Cookie -> colliding header -> Set-Cookie) so the header code keeps exercising it.
--
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] Fix stale Set-Cookie pointer after HttpHeader mutations (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3606:
URL: https://github.com/apache/brpc/pull/3606#issuecomment-6244717396
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