Skip to content

Don't let malformed set_parameters requests crash the node #3256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
baszalmstra wants to merge 1 commit into
base: rolling
Choose a base branch
from
+76 −8
Open

Don't let malformed set_parameters requests crash the node #3256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions rclcpp/src/rclcpp/parameter_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "rclcpp/parameter_service.hpp"

#include <algorithm>
#include <exception>
#include <memory>
#include <string>
#include <vector>
Expand Down Expand Up @@ -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) {

@skyegalaxy skyegalaxy Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could scope this to just a handful of relevant exceptions

@baszalmstra baszalmstra Sep 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then as soon as a new exception is added you can introduce the same crash. What does scoping add here?

@impala454 impala454 Sep 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment here #3257 (comment)

@jmachowinski jmachowinski Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catchalls are considered back practice as you might also catch exceptions that were not meant to be catched.
You can also give better error messages depending on the type of exception you are catching...

@baszalmstra baszalmstra Sep 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that backporting is desirable, what is the best type to use in this case? std::runtime_error?

@impala454 impala454 Sep 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catchalls are considered back practice as you might also catch exceptions that were not meant to be catched. You can also give better error messages depending on the type of exception you are catching...

Totally agreed but arguably completely missed exceptions also bad practice. I think catching broadly here fixes the current issue but to properly fix this with best practices, all throws in this repo should be re-evalued to be sure no generic exceptions are being thrown. Obviously this was already started from humble > rolling with that TODO I linked fixed, just this would be another case. I suppose the backport to humble for this exact issue could include fixing that generic throw. I will leave it to the folks implementing but that's my $0.02.

RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what());
result.successful = false;
result.reason = ex.what();
Expand All @@ -108,21 +109,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::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);
Expand Down
62 changes: 62 additions & 0 deletions rclcpp/test/rclcpp/test_parameter_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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));
Expand Down