From 665e37784a6f1772e0efd2685c10fb0c17fdac96 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 3 Sep 2021 08:49:57 -0400 Subject: [PATCH] Make sure to check for empty extension before accessing front. (#1764) The documentation for std::string::front() says that calling it on an empty string is undefined behavior. It actually seems to work on all platforms except Windows Debug, where it throws an error. Make sure to check for empty first. We also notice that there is no reason to check the existing_sub_namespace for empty; the rest of the code handles that fine. So remove that check. Finally, we add an anonymous namespace so that these functions do not pollute the global namespace. Signed-off-by: Chris Lalancette --- rclcpp/src/rclcpp/node.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/rclcpp/src/rclcpp/node.cpp b/rclcpp/src/rclcpp/node.cpp index 46f1af12df..5d1f9a3077 100644 --- a/rclcpp/src/rclcpp/node.cpp +++ b/rclcpp/src/rclcpp/node.cpp @@ -47,6 +47,9 @@ using rclcpp::Node; using rclcpp::NodeOptions; using rclcpp::exceptions::throw_from_rcl_error; +namespace +{ + RCLCPP_LOCAL std::string extend_sub_namespace(const std::string & existing_sub_namespace, const std::string & extension) @@ -54,18 +57,18 @@ extend_sub_namespace(const std::string & existing_sub_namespace, const std::stri // Assumption is that the existing_sub_namespace does not need checking // because it would be checked already when it was set with this function. - // check if the new sub-namespace extension is absolute - if (extension.front() == '/') { + if (extension.empty()) { throw rclcpp::exceptions::NameValidationError( "sub_namespace", extension.c_str(), - "a sub-namespace should not have a leading /", + "sub-nodes should not extend nodes by an empty sub-namespace", 0); - } else if (existing_sub_namespace.empty() && extension.empty()) { + } else if (extension.front() == '/') { + // check if the new sub-namespace extension is absolute throw rclcpp::exceptions::NameValidationError( "sub_namespace", extension.c_str(), - "sub-nodes should not extend nodes by an empty sub-namespace", + "a sub-namespace should not have a leading /", 0); } @@ -76,7 +79,7 @@ extend_sub_namespace(const std::string & existing_sub_namespace, const std::stri new_sub_namespace = existing_sub_namespace + "/" + extension; } - // remove any trailing `/` so that new extensions do no result in `//` + // remove any trailing `/` so that new extensions do not result in `//` if (new_sub_namespace.back() == '/') { new_sub_namespace = new_sub_namespace.substr(0, new_sub_namespace.size() - 1); } @@ -104,6 +107,8 @@ create_effective_namespace(const std::string & node_namespace, const std::string } } +} // namespace + Node::Node( const std::string & node_name, const NodeOptions & options)