Skip to content
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

Jump to bottom

Guard against integer overflow in duration conversion #1584

Merged
merged 6 commits into from
Mar 22, 2021
Merged

Guard against integer overflow in duration conversion #1584

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05fad8b
Guard against integer overflow in duration conversion
jacobperron Mar 19, 2021
1a40751
Fix lint
jacobperron Mar 19, 2021
05c0169
Clarify comment
jacobperron Mar 20, 2021
6bf4ea1
Use non-std int types for consistency
jacobperron Mar 20, 2021
e4636b1
handle large negative values
jacobperron Mar 20, 2021
b621885
Saturate lower bound of unsigned nanoseconds to zero
jacobperron Mar 22, 2021
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
33 changes: 25 additions & 8 deletions rclcpp/src/rclcpp/duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,27 @@ Duration::operator builtin_interfaces::msg::Duration() const
{
builtin_interfaces::msg::Duration msg_duration;
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);
if (result.rem >= 0) {
msg_duration.sec = static_cast<std::int32_t>(result.quot);
msg_duration.nanosec = static_cast<std::uint32_t>(result.rem);
// saturate if we will overflow
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 {
msg_duration.sec = static_cast<std::int32_t>(result.quot - 1);
msg_duration.nanosec = static_cast<std::uint32_t>(kDivisor + result.rem);
if (result.quot < min_s) {
msg_duration.sec = min_s;
msg_duration.nanosec = max_ns;
} else {
msg_duration.sec = static_cast<int32_t>(result.quot - 1);
msg_duration.nanosec = static_cast<uint32_t>(kDivisor + result.rem);
}
jacobperron marked this conversation as resolved.
Show resolved Hide resolved
}
return msg_duration;
}
Expand Down Expand Up @@ -238,11 +252,14 @@ Duration::to_rmw_time() const
throw std::runtime_error("rmw_time_t cannot be negative");
}

// reuse conversion logic from msg creation
builtin_interfaces::msg::Duration msg = *this;
// Purposefully avoid creating from builtin_interfaces::msg::Duration
// to avoid possible overflow converting from int64_t to int32_t, then back to uint64_t
rmw_time_t result;
result.sec = static_cast<uint64_t>(msg.sec);
result.nsec = static_cast<uint64_t>(msg.nanosec);
constexpr rcl_duration_value_t kDivisor = RCL_S_TO_NS(1);
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;
}

Expand Down
29 changes: 29 additions & 0 deletions rclcpp/test/rclcpp/test_duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ TEST_F(TestDuration, maximum_duration) {
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_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) {
EXPECT_EQ(rclcpp::Duration(0ns), rclcpp::Duration::from_seconds(0.0));
Expand Down Expand Up @@ -267,6 +268,34 @@ TEST_F(TestDuration, conversions) {
auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), -ONE_AND_HALF_SEC_IN_NS);
}

{
auto duration = rclcpp::Duration::from_nanoseconds(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::from_nanoseconds(-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, std::numeric_limits<uint32_t>::max());

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) {
Expand Down