niklas/rclcpp
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Guard against integer overflow in duration conversion (#1584) (#1761)

Browse source 
* Guard against integer overflow in duration conversion (#1584)

Guard against overflow when converting from rclcpp::Duration to builtin_interfaces::msg::Duration,
which is a unsigned to signed conversion.

Use non-std int types for consistency

Handle large negative values

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix test

rclcpp::Duration::from_nanoseconds is not available in Foxy.
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This commit is contained in:
Jacob Perron 2021-08-31 18:19:42 -07:00 • committed by GitHub
parent c2285c9a40
commit c191956f63
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 54 additions and 8 deletions
Download patch file Download diff file Expand all files Collapse all files

33
rclcpp/src/rclcpp/duration.cpp
View file

@ -67,13 +67,27 @@ Duration::operator builtin_interfaces::msg::Duration() const
{ {
builtin_interfaces::msg::Duration msg_duration; builtin_interfaces::msg::Duration msg_duration;
constexpr rcl_duration_value_t kDivisor = RCL_S_TO_NS(1); constexpr rcl_duration_value_t kDivisor = RCL_S_TO_NS(1);
constexpr int32_t max_s = std::numeric_limits<int32_t>::max();
constexpr int32_t min_s = std::numeric_limits<int32_t>::min();
constexpr uint32_t max_ns = std::numeric_limits<uint32_t>::max();
const auto result = std::div(rcl_duration_.nanoseconds, kDivisor); const auto result = std::div(rcl_duration_.nanoseconds, kDivisor);
if (result.rem >= 0) { if (result.rem >= 0) {
msg_duration.sec = static_cast<std::int32_t>(result.quot); // saturate if we will overflow
msg_duration.nanosec = static_cast<std::uint32_t>(result.rem); if (result.quot > max_s) {
msg_duration.sec = max_s;
msg_duration.nanosec = max_ns;
} else {
msg_duration.sec = static_cast<int32_t>(result.quot);
msg_duration.nanosec = static_cast<uint32_t>(result.rem);
}
} else { } else {
msg_duration.sec = static_cast<std::int32_t>(result.quot - 1); if (result.quot <= min_s) {
msg_duration.nanosec = static_cast<std::uint32_t>(kDivisor + result.rem); msg_duration.sec = min_s;
msg_duration.nanosec = 0u;
} else {
msg_duration.sec = static_cast<int32_t>(result.quot - 1);
msg_duration.nanosec = static_cast<uint32_t>(kDivisor + result.rem);
}
} }
return msg_duration; return msg_duration;
} }
@ -238,11 +252,14 @@ Duration::to_rmw_time() const
throw std::runtime_error("rmw_time_t cannot be negative"); throw std::runtime_error("rmw_time_t cannot be negative");
} }
// reuse conversion logic from msg creation // Purposefully avoid creating from builtin_interfaces::msg::Duration
builtin_interfaces::msg::Duration msg = *this; // to avoid possible overflow converting from int64_t to int32_t, then back to uint64_t
rmw_time_t result; rmw_time_t result;
result.sec = static_cast<uint64_t>(msg.sec); constexpr rcl_duration_value_t kDivisor = RCL_S_TO_NS(1);
result.nsec = static_cast<uint64_t>(msg.nanosec); const auto div_result = std::div(rcl_duration_.nanoseconds, kDivisor);
result.sec = static_cast<uint64_t>(div_result.quot);
result.nsec = static_cast<uint64_t>(div_result.rem);
return result; return result;
} }

29
rclcpp/test/rclcpp/test_duration.cpp
View file

@ -138,6 +138,7 @@ TEST_F(TestDuration, maximum_duration) {
static const int64_t HALF_SEC_IN_NS = 500 * 1000 * 1000; static const int64_t HALF_SEC_IN_NS = 500 * 1000 * 1000;
static const int64_t ONE_SEC_IN_NS = 1000 * 1000 * 1000; static const int64_t ONE_SEC_IN_NS = 1000 * 1000 * 1000;
static const int64_t ONE_AND_HALF_SEC_IN_NS = 3 * HALF_SEC_IN_NS; static const int64_t ONE_AND_HALF_SEC_IN_NS = 3 * HALF_SEC_IN_NS;
static const int64_t MAX_NANOSECONDS = std::numeric_limits<int64_t>::max();
TEST_F(TestDuration, from_seconds) { TEST_F(TestDuration, from_seconds) {
EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration::from_seconds(0.0)); EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration::from_seconds(0.0));
@ -237,6 +238,34 @@ TEST_F(TestDuration, conversions) {
auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>(); auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), -ONE_AND_HALF_SEC_IN_NS); EXPECT_EQ(chrono_duration.count(), -ONE_AND_HALF_SEC_IN_NS);
} }
{
auto duration = rclcpp::Duration(MAX_NANOSECONDS);
const auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, std::numeric_limits<int32_t>::max());
EXPECT_EQ(duration_msg.nanosec, std::numeric_limits<uint32_t>::max());
auto rmw_time = duration.to_rmw_time();
EXPECT_EQ(rmw_time.sec, 9223372036u);
EXPECT_EQ(rmw_time.nsec, 854775807u);
auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), MAX_NANOSECONDS);
}
{
auto duration = rclcpp::Duration(-MAX_NANOSECONDS);
const auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, std::numeric_limits<int32_t>::min());
EXPECT_EQ(duration_msg.nanosec, 0u);
EXPECT_THROW(duration.to_rmw_time(), std::runtime_error);
auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), -MAX_NANOSECONDS);
}
} }
TEST_F(TestDuration, test_some_constructors) { TEST_F(TestDuration, test_some_constructors) {