Repository navigation
Fix use-after-destruction when erasing MultiFlatMap bucket head - #3617
Merged
chenBright merged 2 commits intoOct 17, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Correct the regression test insertion order so it exercises the reported use-after-destruction case.
No open issues 🎉
What changed in this PR
Fixes MultiFlatMap::erase to prevent use-after-destruction when replacing a bucket head.
Changes:
- Preserves bucket-head lifetime during erase.
- Adds regression coverage using non-trivial values.
- Corrects related comments.
| File | Summary |
|---|---|
test/flat_map_unittest.cpp |
Adds collision-and-erase regression coverage, but insertion order does not exercise bucket-head replacement. |
src/butil/containers/flat_map_inl.h |
Fixes bucket-head lifetime handling during erase. |
Review effort: Lite
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 19, 2026
Bring the apache master line up to f0239d7 into this branch in a single merge, replacing the two stacked merges with master@36c2d12e and the macOS fix only. Everything the branch needs from that line is included: the data-format negotiation (apache#3507), the IPC_V2 data path (apache#3573), the macOS OpenSSL 4.0 build fix (apache#3616: X509 accessors return const pointers in the current macos-26-arm64 runner image, fixed with auto*), and the newer fixes, notably the null-termination of the peer shm_name in HelloMessage::Deserialize (apache#3605), the MPSC queue node release ordering (apache#3620) and the MultiFlatMap bucket-head erasure fix (apache#3617). Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged, with master-side UNLIKELY/LIKELY converted to the module's BAIDU_* macros. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields check the unarmed close_timer/hb_timer slots instead, and the UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored alongside master's apache#3605 hardening of the full_shm_name test. Both sides' suites run. Tested: the close/cleanup/IPC_V2 resolution was verified on the equivalent tree before apache#3605 came in -- cmake WITH_UBRING=ON build, 31/31 unit tests, the same under ASAN (detect_leaks=0), and an example/ubring_performance e2e over the IPC backend (negotiated IPC_V2 path at ~148k QPS, clean teardowns, server kill -9 handled). The new master commits are additive to that resolution; cmake --build build --target brpc_ubring_unittest and ./build/test/brpc_ubring_unittest pass on this exact tree as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
MultiFlatMap::erasedestroyed the bucket-head element before moving a surviving collision node into it. Assigning to the destroyed element caused use-after-destruction:What is changed and the side effects?
Changed:
Keep the bucket-head element alive when another node in the bucket survives.
Side effects:
Performance effects:
Breaking backward compatibility:
Check List: