From a64b31d4f5c195c212ebd3138a237d70d7436586 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra <4995967+baszalmstra@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:13:36 +0200 Subject: [PATCH 1/2] Don't let malformed set_parameters requests crash the node (#3256) 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> Signed-off-by: Alejandro Hernandez Cordero Co-authored-by: Alejandro Hernandez Cordero (cherry picked from commit 2e5a8b9ea68d03ebd24627a91abc8f105fe81661) # Conflicts: # rclcpp/test/rclcpp/test_parameter_service.cpp --- rclcpp/src/rclcpp/parameter_service.cpp | 24 +++++-- rclcpp/test/rclcpp/test_parameter_service.cpp | 66 +++++++++++++++++++ 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/rclcpp/src/rclcpp/parameter_service.cpp b/rclcpp/src/rclcpp/parameter_service.cpp index 5bc6f82a67..cf7f18f5f3 100644 --- a/rclcpp/src/rclcpp/parameter_service.cpp +++ b/rclcpp/src/rclcpp/parameter_service.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -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,14 +113,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 +128,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::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); diff --git a/rclcpp/test/rclcpp/test_parameter_service.cpp b/rclcpp/test/rclcpp/test_parameter_service.cpp index 6b0838b0db..1cc550320d 100644 --- a/rclcpp/test/rclcpp/test_parameter_service.cpp +++ b/rclcpp/test/rclcpp/test_parameter_service.cpp @@ -20,7 +20,14 @@ #include #include +<<<<<<< HEAD #include "rclcpp/rclcpp.hpp" +======= +#include "rcl_interfaces/msg/parameter.hpp" +#include "rcl_interfaces/srv/set_parameters.hpp" +#include "rcl_interfaces/srv/set_parameters_atomically.hpp" + +>>>>>>> 2e5a8b9 (Don't let malformed set_parameters requests crash the node (#3256)) #include "../../src/rclcpp/parameter_service_names.hpp" using namespace std::chrono_literals; @@ -93,6 +100,65 @@ 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)); From 3e4cb7373d7cde0b7219341fa98fcec094c17f87 Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Thu, 10 Sep 2026 17:13:24 +0200 Subject: [PATCH 2/2] Update test_parameter_service.cpp Signed-off-by: Janosch Machowinski --- rclcpp/test/rclcpp/test_parameter_service.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/rclcpp/test/rclcpp/test_parameter_service.cpp b/rclcpp/test/rclcpp/test_parameter_service.cpp index 1cc550320d..5a844f596e 100644 --- a/rclcpp/test/rclcpp/test_parameter_service.cpp +++ b/rclcpp/test/rclcpp/test_parameter_service.cpp @@ -20,14 +20,7 @@ #include #include -<<<<<<< HEAD #include "rclcpp/rclcpp.hpp" -======= -#include "rcl_interfaces/msg/parameter.hpp" -#include "rcl_interfaces/srv/set_parameters.hpp" -#include "rcl_interfaces/srv/set_parameters_atomically.hpp" - ->>>>>>> 2e5a8b9 (Don't let malformed set_parameters requests crash the node (#3256)) #include "../../src/rclcpp/parameter_service_names.hpp" using namespace std::chrono_literals;