Improve the robustness of the TopicEndpointInfo constructor #3013
Conversation
Signed-off-by: Barry Xu <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness of the TopicEndpointInfo constructor by adding null pointer checks for the node_name, node_namespace, and topic_type fields from the rcl_topic_endpoint_info_t structure. Previously, passing nullptr values would result in undefined behavior when initializing std::string members, causing exceptions during construction as reported in ros2/rosbag2#2293.
- Added ternary operator checks to safely handle nullptr values
- Empty strings are used as fallback values when nullptr is encountered
- Prevents exceptions during TopicEndpointInfo construction with invalid input
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : node_name_(info.node_name ? info.node_name : ""), | ||
| node_namespace_(info.node_namespace ? info.node_namespace : ""), | ||
| topic_type_(info.topic_type ? info.topic_type : ""), |
There was a problem hiding this comment.
The null pointer checks added to protect against nullptr values for node_name, node_namespace, and topic_type should be covered by tests. Consider adding a test case that creates a TopicEndpointInfo with an rcl_topic_endpoint_info_t structure containing nullptr values for these fields to verify the fix handles this edge case correctly.
There was a problem hiding this comment.
Currently, TopicEndpointInfo has not been tested in any of the test code. Besides, this modification is simple. Is it necessary to add a new test file to test such a simple change?
|
Pulls: #3013
|
jmachowinski
left a comment
jmachowinski
left a comment
There was a problem hiding this comment.
I am not a fan of this patch. Giving in an nullptr is clearly unintended behavior. Instead of silently ignoring this we should rather actively throw an exception.
|
Thank you for your comments. RCLCPP_PUBLIC
explicit TopicEndpointInfo(const rcl_topic_endpoint_info_t & info)
: endpoint_type_(static_cast<rclcpp::EndpointType>(info.endpoint_type)),
qos_profile_({info.qos_profile.history, info.qos_profile.depth}, info.qos_profile),
topic_type_hash_(info.topic_type_hash)
{
if (!info.node_name || !info.node_namespace || !info.topic_type) {
throw std::invalid_argument("Constructor TopicEndpointInfo with invalid topic endpoint info");
}
node_name_ = info.node_name;
node_namespace_ = info.node_namespace;
topic_type_ = info.topic_type;
std::copy(info.endpoint_gid, info.endpoint_gid + RMW_GID_STORAGE_SIZE, endpoint_gid_.begin());
} |
|
@Barry-Xu-2018 your proposal looks exactly like what I had in mind. |
Signed-off-by: Barry Xu <[email protected]>
|
Pulls: #3013
|
5 participants
Description
node_name,node_namespaceandtopic_typemay be nullptr. This will result in initializing a std::string with nullptr, which will cause an exception during construction. This issue is mentioned here ros2/rosbag2#2293.rclcpp/rclcpp/include/rclcpp/node_interfaces/node_graph_interface.hpp
Lines 59 to 62 in c85ff92
Is this user-facing behavior change?
No
Did you use Generative AI?
No
Additional Information
N/A