iframe-proxy

songgaoye · 2026-10-14T11:02:22Z

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.

Refreshing it with HeaderMap::seek() is not sufficient: HeaderMap is a
multi-map and does not preserve the identity or insertion order of duplicate
Set-Cookie fields. The refreshed pointer may therefore select a different
cookie.

What is changed and the side effects?

Changed:

Store Set-Cookie fields separately in insertion order and keep HeaderMap
synchronized for header iteration and serialization.

Make GetHeader(SET_COOKIE) and GetAllSetCookieHeader() read the stable
ordered storage. This also makes Swap() and Clear() safe without retaining
a stale cached pointer.

Add regression coverage for actual map growth, multiple Set-Cookie fields,
fragmented HTTP parsing, Swap(), copy construction, and assignment.

Side effects:

Performance effects:

The change only adds separate storage and synchronization for Set-Cookie
fields. Ordinary headers keep the existing path.
Breaking backward compatibility:

None.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings

This test intends to exercise HeaderMap growth/rehash, but inserting 128 headers may not reliably… · New
Since the regression includes Clear() leaving _first_set_cookie stale, this test should assert… · New
The old_bucket_count + bucket_count() comparison + refresh pattern is duplicated (also in… · New
Indentation inside the loop is inconsistent with surrounding lines in this block. Aligning… · New

What changed in this PR

Fixes a stale cached pointer for the first Set-Cookie header in HttpHeader after HeaderMap mutations (rehash/growth), Swap(), and Clear(), and adds regression tests for these cases.

Changes:

Refresh _first_set_cookie after HeaderMap growth and after HttpHeader::Swap()
Clear _first_set_cookie in HttpHeader::Clear()
Add unit tests covering map growth, Swap(), and Clear()

🧠 Review effort: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

songgaoye · 2026-10-15T03:06:06Z

The CI failure is unrelated to this PR. It was fixed in #3610.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings

HttpMessage calls HttpHeader::HasSetCookieHeaders() and HttpHeader::SyncSetCookieHeaders().… · New
AddHeader() now unconditionally appends into _set_cookie_headers and syncs SET_COOKIE,… · New
This test forces a HeaderMap growth by inserting up to 4096 headers and asserting… · New
The counting variables are handled inconsistently here: set_cookie_value1_count is declared… · New

4 resolved since last review

🧠 Review effort: Lite

Copilot

🟡 Changes recommended

Rebuilding all cookie entries after each append can cause quadratic work for multi-cookie responses.

1 open issue

Avoid quadratic Set-Cookie synchronization during multi-cookie responses · New

4 resolved since last review

AddHeader() now unconditionally appends into _set_cookie_headers and syncs SET_COOKIE,…
HttpMessage calls HttpHeader::HasSetCookieHeaders() and HttpHeader::SyncSetCookieHeaders().…
This test forces a HeaderMap growth by inserting up to 4096 headers and asserting…
The counting variables are handled inconsistently here: set_cookie_value1_count is declared…

_{Review effort: Lite}

wwbmmm · 2026-10-18T02:27:11Z

LGTM

chenBright · 2026-10-18T14:32:24Z

+    if (!_set_cookie_headers_dirty) {
+        return;
+    }
+    _headers.erase(SET_COOKIE);


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:

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.

chenBright requested a lite review from Copilot October 14, 2026 11:40

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread test/brpc_http_message_unittest.cpp Outdated

Comment thread test/brpc_http_message_unittest.cpp

Comment thread src/brpc/http_header.cpp Outdated

Comment thread test/brpc_http_message_unittest.cpp Outdated

Copilot stopped reviewing on behalf of chenBright due to an error October 14, 2026 12:53
Workflow completed but conclusion is not available. This may indicate rate limiting or API issues.

chenBright reviewed Oct 15, 2026

View reviewed changes

Comment thread src/brpc/http_header.cpp Outdated

songgaoye force-pushed the fix/http-header-set-cookie-cache branch from bfe433d to 70608b9 Compare October 15, 2026 05:50

chenBright requested a lite review from Copilot October 16, 2026 03:20

Copilot AI reviewed Oct 16, 2026

View reviewed changes

Comment thread src/brpc/details/http_message.cpp

Comment thread src/brpc/http_header.cpp

Comment thread test/brpc_http_message_unittest.cpp Outdated

Comment thread test/brpc_http_message_unittest.cpp Outdated

Copilot stopped reviewing on behalf of chenBright due to an error October 16, 2026 03:42
The job was not acquired by a runner even after multiple attempts.

songgaoye added 4 commits October 16, 2026 14:01

Fix stale Set-Cookie pointer after HttpHeader mutations

0daecb7

Strengthen HttpHeader rehash regression test

762c824

Fix Set-Cookie header cache invalidation

4be80aa

Preserve AddHeader behavior for non-cookie headers

b6001dc

songgaoye force-pushed the fix/http-header-set-cookie-cache branch from f9c55d5 to b6001dc Compare October 16, 2026 06:02

chenBright requested a lite review from Copilot October 17, 2026 10:07

Copilot started reviewing on behalf of chenBright October 17, 2026 10:08

View session

Copilot AI reviewed Oct 17, 2026

View reviewed changes

Comment thread src/brpc/http_header.cpp

songgaoye added 2 commits October 17, 2026 18:32

Merge branch 'master' into fix/http-header-set-cookie-cache

5922252

Avoid repeated Set-Cookie header synchronization

e9a1128

songgaoye requested a review from chenBright October 18, 2026 01:13

chenBright reviewed Oct 18, 2026

View reviewed changes

songgaoye marked this pull request as draft October 21, 2026 15:03

File	Description
test/brpc_http_message_unittest.cpp	Adds regression tests to catch stale `_first_set_cookie` after growth, `Swap()`, and `Clear()`.
src/brpc/http_header.h	Declares `RefreshFirstSetCookie()` helper to rebuild the cached pointer safely.
src/brpc/http_header.cpp	Implements pointer refresh logic on rehash/growth and `Swap()`, and clears cache on `Clear()`.

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

Fix stale Set-Cookie pointer after HttpHeader mutations - #3606

Fix stale Set-Cookie pointer after HttpHeader mutations#3606
songgaoye wants to merge 6 commits into
apache:masterfrom
songgaoye:fix/http-header-set-cookie-cache

songgaoye commented Oct 14, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

songgaoye commented Oct 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

wwbmmm commented Oct 18, 2026

Uh oh!

chenBright Oct 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

songgaoye commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and the side effects?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

songgaoye commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

wwbmmm commented Oct 18, 2026

Uh oh!

chenBright Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

songgaoye commented Oct 14, 2026 •

edited

Loading

songgaoye commented Oct 15, 2026 •

edited

Loading