Don't let malformed set_parameters requests crash the node

rclcpp/src/rclcpp/parameter_service.cpp

@@ -16,6 +16,7 @@
 
 #include <algorithm>
 #include <memory>
+#include <stdexcept>
 #include <string>
 #include <vector>
 
@@ -94,6 +95,10 @@ ParameterService::ParameterService(
           RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what());
           result.successful = false;
           result.reason = ex.what();
+        } catch (const std::runtime_error & ex) {
+          RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what());
+          result.successful = false;
+          result.reason = ex.what();
         }
         response->results.push_back(result);
       }
@@ -108,21 +113,26 @@ ParameterService::ParameterService(
       const std::shared_ptr<rcl_interfaces::srv::SetParametersAtomically::Request> & request,
       std::shared_ptr<rcl_interfaces::srv::SetParametersAtomically::Response> response)
     {
-      std::vector<rclcpp::Parameter> pvariants;
-      std::transform(
-        request->parameters.cbegin(), request->parameters.cend(),
-        std::back_inserter(pvariants),
-        [](const rcl_interfaces::msg::Parameter & p) {
-          return rclcpp::Parameter::from_parameter_msg(p);
-        });
       try {
+        std::vector<rclcpp::Parameter> pvariants;
+        std::transform(
+          request->parameters.cbegin(), request->parameters.cend(),
+          std::back_inserter(pvariants),
+          [](const rcl_interfaces::msg::Parameter & p) {
+            return rclcpp::Parameter::from_parameter_msg(p);
+          });
         auto result = node_params->set_parameters_atomically(pvariants);
         response->result = result;
       } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) {
         RCLCPP_WARN(
           rclcpp::get_logger("rclcpp"), "Failed to set parameters atomically: %s", ex.what());
         response->result.successful = false;
         response->result.reason = "One or more parameters were not declared before setting";
+      } catch (const std::runtime_error & ex) {
+        RCLCPP_WARN(
+          rclcpp::get_logger("rclcpp"), "Failed to set parameters atomically: %s", ex.what());
+        response->result.successful = false;
+        response->result.reason = ex.what();
       }
     },
     qos_profile, nullptr);

rclcpp/test/rclcpp/test_parameter_service.cpp

@@ -20,6 +20,10 @@
 #include <utility>
 #include <vector>
 
+#include "rcl_interfaces/msg/parameter.hpp"
+#include "rcl_interfaces/srv/set_parameters.hpp"
+#include "rcl_interfaces/srv/set_parameters_atomically.hpp"
+
 #include "../../src/rclcpp/parameter_service_names.hpp"
 #include "rclcpp/node.hpp"
 #include "rclcpp/parameter.hpp"
@@ -98,6 +102,64 @@ TEST_F(TestParameterService, set_parameters_atomically) {
   EXPECT_EQ(0, client->get_parameter("parameter1", 100));
 }
 
+// Regression: an empty name must fail the request, not crash the node.
+TEST_F(TestParameterService, set_parameters_empty_name_returns_failure) {
+  const std::vector<rclcpp::Parameter> parameters = {
+    rclcpp::Parameter("", 0),
+  };
+  const auto results = client->set_parameters(parameters, 10s);
+  ASSERT_EQ(1u, results.size());
+  EXPECT_FALSE(results[0].successful);
+}
+
+TEST_F(TestParameterService, set_parameters_atomically_empty_name_returns_failure) {
+  const std::vector<rclcpp::Parameter> parameters = {
+    rclcpp::Parameter("", 0),
+  };
+  const auto result = client->set_parameters_atomically(parameters, 10s);
+  EXPECT_FALSE(result.successful);
+}
+
+// Unknown value type; the typed clients can't build one, so use a raw client.
+TEST_F(TestParameterService, set_parameters_unknown_type_returns_failure) {
+  auto raw_client = node->create_client<rcl_interfaces::srv::SetParameters>(
+    std::string(node->get_name()) + "/" + rclcpp::parameter_service_names::set_parameters);
+  ASSERT_TRUE(raw_client->wait_for_service(10s));
+
+  auto request = std::make_shared<rcl_interfaces::srv::SetParameters::Request>();
+  rcl_interfaces::msg::Parameter parameter;
+  parameter.name = "parameter1";
+  parameter.value.type = 42;  // not a valid rclcpp::ParameterType
+  request->parameters.push_back(parameter);
+
+  auto future = raw_client->async_send_request(request);
+  ASSERT_EQ(
+    rclcpp::spin_until_future_complete(node, future, 10s),
+    rclcpp::FutureReturnCode::SUCCESS);
+  const auto response = future.get();
+  ASSERT_EQ(1u, response->results.size());
+  EXPECT_FALSE(response->results[0].successful);
+}
+
+TEST_F(TestParameterService, set_parameters_atomically_unknown_type_returns_failure) {
+  auto raw_client = node->create_client<rcl_interfaces::srv::SetParametersAtomically>(
+    std::string(node->get_name()) + "/" + rclcpp::parameter_service_names::set_parameters_atomically);
+  ASSERT_TRUE(raw_client->wait_for_service(10s));
+
+  auto request = std::make_shared<rcl_interfaces::srv::SetParametersAtomically::Request>();
+  rcl_interfaces::msg::Parameter parameter;
+  parameter.name = "parameter1";
+  parameter.value.type = 42;  // not a valid rclcpp::ParameterType
+  request->parameters.push_back(parameter);
+
+  auto future = raw_client->async_send_request(request);
+  ASSERT_EQ(
+    rclcpp::spin_until_future_complete(node, future, 10s),
+    rclcpp::FutureReturnCode::SUCCESS);
+  const auto response = future.get();
+  EXPECT_FALSE(response->result.successful);
+}
+
 TEST_F(TestParameterService, list_parameters) {
   const size_t number_parameters_in_basic_node = client->list_parameters({}, 1, 10s).names.size();
   node->declare_parameter("parameter1", rclcpp::ParameterValue(42));