diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index 890eeb217..95289a4be 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -286,15 +286,16 @@ inline void Blackboard::set(const std::string& key, const T& value) rootBlackboard()->set(key.substr(1, key.size() - 1), value); return; } - std::shared_lock storage_lock(storage_mutex_); - - // check local storage - auto it = storage_.find(key); - if(it == storage_.end()) + // getEntry() follows the remapping to the parent blackboard, exactly like + // the readers do. A key that resolves to an existing entry (local or + // remapped) must go through the type check below: createEntryImpl() would + // follow the same remapping and return the pre-existing entry, and writing + // to it blindly bypasses the type lock and the string conversion. + auto entry_ptr = getEntry(key); + if(!entry_ptr) { // create a new entry Any new_value(value); - storage_lock.unlock(); std::shared_ptr entry; // if a new generic port is created with a string, it's type should be AnyTypeAllowed if constexpr(std::is_same_v) @@ -319,10 +320,6 @@ inline void Blackboard::set(const std::string& key, const T& value) { // this is not the first time we set this entry, we need to check // if the type is the same or not. - // Copy shared_ptr to prevent use-after-free if another thread - // calls unset() while we hold the reference (BUG-2 fix). - auto entry_ptr = it->second; - storage_lock.unlock(); Entry& entry = *entry_ptr; std::scoped_lock scoped_lock(entry.entry_mutex); diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index 693b63c9e..020cd722b 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -1064,3 +1064,34 @@ TEST(BlackboardTest, GetLockedPortContentWithDefault_Issue942) // The value should be accessible from the blackboard ASSERT_EQ(tree.rootBlackboard()->get("value"), 42); } + +TEST(BlackboardTest, SetThroughSubtreeRemappingChecksType) +{ + // set() must apply the same type check and string conversion when the key + // is remapped to the parent blackboard, like does. + auto parent_bb = Blackboard::create(); + parent_bb->set("value", 42); // strongly typed as int + + auto child_bb = Blackboard::create(parent_bb); + child_bb->addSubtreeRemapping("local", "value"); + + // a string that can't be converted to int is rejected, as it is on the parent + ASSERT_ANY_THROW(parent_bb->set("value", std::string("garbage"))); + ASSERT_ANY_THROW(child_bb->set("local", std::string("garbage"))); + + // the entry must keep both its declared type and its stored type + const auto entry = parent_bb->getEntry("value"); + ASSERT_EQ(entry->info.type(), typeid(int)); + ASSERT_EQ(entry->value.type(), typeid(int)); + + // a convertible string is parsed to the declared type, not stored as string + child_bb->set("local", std::string("99")); + ASSERT_EQ(entry->value.type(), typeid(int)); + ASSERT_EQ(parent_bb->get("value"), 99); + + // a safe numeric conversion is accepted, as it is on the parent + parent_bb->set("small", static_cast(1)); + child_bb->addSubtreeRemapping("local_small", "small"); + ASSERT_NO_THROW(child_bb->set("local_small", 100)); + ASSERT_EQ(parent_bb->get("small"), 100); +}