add Blackboard::getKeyNames() and deprecate getKeys() (dangling StringViews) - #1203
Merged
Merged
Conversation
getKeys() returns StringViews into the keys of the storage map, valid only while the storage lock is held, i.e. never for the caller. ExportBlackboardToJSON built a std::string from them while another thread could unset() the entry: heap-use-after-free (Groot2 thread vs UnsetBlackboard on the tick thread). getKeyNames() returns copies made under the lock. getKeys() is unchanged but deprecated, so the change is purely additive (no ABI/API break). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
facontidavide
force-pushed
the
fix-getkeys-dangling-views
branch
from
September 20, 2026 13:18
ed92f32 to
d1d5ecf
Compare
|
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
1 participant
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.
Follow-up of #1202 (merged); rebased onto
master.Bug
Blackboard::getKeys()returnsstd::vector<StringView>pointing into the keys ofstorage_. The shared lock taken inside only protects the iteration: once the function returns, a concurrentunset()frees the key and the views dangle.The library itself was bitten by this:
ExportBlackboardToJSONbuilt astd::stringfrom each view, so the Groot2 publisher thread could read freed memory while the tick thread runsUnsetBlackboard. ASan on the new test, before the fix:It was found while reviewing #1181 / #1202 and is independent of the entry-lifetime bug fixed there.
Fix (additive, no API/ABI break)
std::vector<std::string> Blackboard::getKeyNames() const: copies made under the storage lock. New non-virtual member, no layout change.getKeys()keeps its signature and behaviour but is marked[[deprecated]](its return type can not be changed compatibly: it is not part of the mangled name).ExportBlackboardToJSONand the tests usegetKeyNames().Test
BlackboardThreadSafety.ExportToJsonWhileKeysAreRemoved: one thread exports to JSON in a loop while another sets/unsets heap-allocated keys. Fails with the ASan report above before the fix, passes after. It relies on #1202 (merged): without it the same interleaving also hits the entry use-after-free.ASan+UBSan 81/81 (Blackboard, JSON, Groot2, parser suites), TSan 36/36, full Debug suite 529/529, pre-commit/clang-tidy clean.
🤖 Generated with Claude Code