Fix use-after-move in ConfigRegistry::do_register - #13622
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, standards-correct for try_emplace semantics, and directly eliminates the verified use-after-move scenario without altering registration behavior.
Pull request overview
Fixes a use-after-move in ConfigRegistry::do_register() when duplicate config registrations occur, ensuring the warning message reliably reports the incoming (rejected) registration owner.
Changes:
- Replace
_entries.emplace(...)with_entries.try_emplace(...)so the incomingEntryis not moved-from on duplicate keys. - Use a precomputed
owner_strfor the duplicate-registration warning to avoid reading owner information from a potentially moved-fromEntry. - Add an in-code comment documenting why
try_emplaceis required here.
File summaries
| File | Description |
|---|---|
| src/mgmt/config/ConfigRegistry.cc | Switches to try_emplace in do_register() to prevent consuming the incoming Entry on duplicates and to keep owner reporting correct. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
933d3d3 to
ab99330
Compare
std::unordered_map::emplace may construct its node before discovering the key is already present, consuming the moved-from Entry even though nothing was inserted. The duplicate-registration warning then read plugin_name out of that gutted Entry and reported the incoming owner as "core". try_emplace leaves the argument untouched when the key exists, so the warning can name the registration it rejected. The read after the move came in with apache#13146, so the clang-analyzer job's Clang-Tidy stage (bugprone-use-after-move) now fails on master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ab99330 to
d016c90
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, correct for the reported emplace/move behavior, and the warning message now reliably uses a stable owner string.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
bryancall
left a comment
There was a problem hiding this comment.
Approving. try_emplace is the right choice here, since it is the only one of the two that promises to leave the argument alone when the key is already present, and reading the owner from owner_str makes the rejected registration nameable again.
One non-blocking note: owner_str is captured before the move, so in the else branch it is a pointer into the local entry's string buffer, and it is valid only because try_emplace does not touch that argument. If this ever goes back to emplace, that read becomes a pointer into a moved-from string rather than the empty-string read it is today. A short comment on the try_emplace line noting that the warning below depends on it would make that harder to undo by accident.
std::unordered_map::emplace may construct its node before discovering
the key is already present, consuming the moved-from Entry even though
nothing was inserted. The duplicate-registration warning then read
plugin_name out of that gutted Entry and reported the incoming owner as
"core". try_emplace leaves the argument untouched when the key exists,
so the warning can name the registration it rejected.
The read after the move came in with #13146, so the clang-analyzer job's
Clang-Tidy stage (bugprone-use-after-move) now fails on master.