validate backup size in BlackboardRestore - #1182
Open
aysha-afrah26 wants to merge 1 commit into
Open
Conversation
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.
BlackboardBackup and BlackboardRestore are meant to be used as a pair, but the restore side drives its loop with tree.subtrees.size() while indexing the backup vector the caller passed in. The only thing tying those two sizes together is an assert, and since CMakeLists.txt forces a Release build when no build type is given, NDEBUG is set in the default and ROS builds and that check is gone. Both sizes are XML-determined, since a tree contributes one blackboard per subtree plus one for the root, so restoring into a tree that was rebuilt from different XML than the backup came from walks off the end of the vector: ASan reports a heap-buffer-overflow read at bt_factory.cpp:779, and cloneInto is then called through the stale shared_ptr that read produced. I ran into it while looking at the rewind pattern from t17_blackboard_backup, where the backup and the tree are not necessarily created together. ImportTreeFromJSON right below already validates the same relationship and throws, so I made BlackboardRestore do the same instead of leaving the guard to a caller. The new test builds a one-subtree backup, restores it into a four-subtree tree, and checks that the matching case still works.