From 1434554120f83eee27345add5325a63f89c05f65 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra <4995967+baszalmstra@users.noreply.github.com> Date: Fri, 4 Sep 2026 11:26:51 +0200 Subject: [PATCH] Prevent parameter service from crashing on malformed set requests The set_parameters and set_parameters_atomically service handlers only caught ParameterNotDeclaredException. A request carrying a parameter with an empty name (InvalidParametersException) or an unknown value type (UnknownTypeError, thrown by Parameter::from_parameter_msg) let the exception escape the service callback, unwind through the executor's spin(), and terminate the node -- an unauthenticated remote DoS on any node exposing the default parameter services. Broaden both handlers to catch std::exception and return an unsuccessful SetParametersResult instead. In the atomic handler, move the from_parameter_msg conversion inside the try so an unknown type raised during conversion is handled too. Adds regression tests covering empty name (typed client) and unknown type (raw client) for both services. Signed-off-by: Bas Zalmstra <4995967+baszalmstra@users.noreply.github.com> --- rclcpp/src/rclcpp/parameter_service.cpp | 22 ++++--- rclcpp/test/rclcpp/test_parameter_service.cpp | 62 +++++++++++++++++++ 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/rclcpp/src/rclcpp/parameter_service.cpp b/rclcpp/src/rclcpp/parameter_service.cpp index 1cc08910d0..435f175e1d 100644 --- a/rclcpp/src/rclcpp/parameter_service.cpp +++ b/rclcpp/src/rclcpp/parameter_service.cpp @@ -15,6 +15,7 @@ #include "rclcpp/parameter_service.hpp" #include +#include #include #include #include @@ -90,7 +91,7 @@ ParameterService::ParameterService( try { result = node_params->set_parameters_atomically( {rclcpp::Parameter::from_parameter_msg(p)}); - } catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) { + } catch (const std::exception & ex) { RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what()); result.successful = false; result.reason = ex.what(); @@ -108,14 +109,14 @@ ParameterService::ParameterService( const std::shared_ptr & request, std::shared_ptr response) { - std::vector 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 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) { @@ -123,6 +124,11 @@ ParameterService::ParameterService( 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::exception & 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); diff --git a/rclcpp/test/rclcpp/test_parameter_service.cpp b/rclcpp/test/rclcpp/test_parameter_service.cpp index 2aff8ecd0d..9abe22fcf9 100644 --- a/rclcpp/test/rclcpp/test_parameter_service.cpp +++ b/rclcpp/test/rclcpp/test_parameter_service.cpp @@ -20,6 +20,10 @@ #include #include +#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 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 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( + 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::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( + 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::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));