From 6171ddb521b91ca8806a5284d21b814c0d75a004 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 ac1e03736f8297b9904d84ec25f15d7b44f838c2 Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Thu, 10 Sep 2026 17:12:23 +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;