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