From d4e7a9b89531dc2935aeaaabd0d3a011dfbb51fc 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 ab2aa882d1..bb103112f2 100644 --- a/rclcpp/src/rclcpp/parameter_service.cpp +++ b/rclcpp/src/rclcpp/parameter_service.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -92,6 +93,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); } @@ -106,14 +111,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) { @@ -121,6 +126,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 020030cf22ff3308bbd0f5bd7c55a1499edeb01a Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Thu, 10 Sep 2026 17:14:06 +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;