From 69c3a0b7c4fa94b6a8842f9b8cbe48e21bdce01c Mon Sep 17 00:00:00 2001 From: Nursharmin Ramli Date: Wed, 17 Apr 2024 15:35:05 +0200 Subject: [PATCH 1/5] Check for negative time in nanoseconds ctor Signed-off-by: Nursharmin Ramli --- rclcpp/src/rclcpp/time.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rclcpp/src/rclcpp/time.cpp b/rclcpp/src/rclcpp/time.cpp index 978651516c..4548878c0b 100644 --- a/rclcpp/src/rclcpp/time.cpp +++ b/rclcpp/src/rclcpp/time.cpp @@ -60,6 +60,10 @@ Time::Time(int32_t seconds, uint32_t nanoseconds, rcl_clock_type_t clock_type) Time::Time(int64_t nanoseconds, rcl_clock_type_t clock_type) : rcl_time_(init_time_point(clock_type)) { + if (nanoseconds < 0) { + throw std::runtime_error("cannot store a negative time point in rclcpp::Time"); + } + rcl_time_.nanoseconds = nanoseconds; } From 19340391bfd3ea4e4b77ce047f56a8f3db9a5a7e Mon Sep 17 00:00:00 2001 From: Nursharmin Ramli Date: Wed, 17 Apr 2024 15:48:18 +0200 Subject: [PATCH 2/5] Update tests for negative check in Time ctor Signed-off-by: Nursharmin Ramli --- rclcpp/test/rclcpp/test_time.cpp | 44 ++------------------------------ 1 file changed, 2 insertions(+), 42 deletions(-) diff --git a/rclcpp/test/rclcpp/test_time.cpp b/rclcpp/test/rclcpp/test_time.cpp index 7d656fb9c4..90eb158b24 100644 --- a/rclcpp/test/rclcpp/test_time.cpp +++ b/rclcpp/test/rclcpp/test_time.cpp @@ -138,6 +138,8 @@ TEST_F(TestTime, conversions) { EXPECT_ANY_THROW(rclcpp::Time(-1, 1)); + EXPECT_ANY_THROW(rclcpp::Time(-1)); + EXPECT_ANY_THROW( { rclcpp::Time assignment(1, 2); @@ -168,48 +170,6 @@ TEST_F(TestTime, conversions) { EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS); EXPECT_EQ(rclcpp::Time(time_msg).nanoseconds(), ONE_AND_HALF_SEC_IN_NS); } - - { - // Can rclcpp::Time be negative or not? The following constructor works: - rclcpp::Time time(-HALF_SEC_IN_NS); - auto time_msg = static_cast(time); - EXPECT_EQ(time_msg.sec, -1); - EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS); - - // The opposite conversion throws... - EXPECT_ANY_THROW( - { - rclcpp::Time negative_time(time_msg); - }); - } - - { - // Can rclcpp::Time be negative or not? The following constructor works: - rclcpp::Time time(-ONE_SEC_IN_NS); - auto time_msg = static_cast(time); - EXPECT_EQ(time_msg.sec, -1); - EXPECT_EQ(time_msg.nanosec, 0u); - - // The opposite conversion throws... - EXPECT_ANY_THROW( - { - rclcpp::Time negative_time(time_msg); - }); - } - - { - // Can rclcpp::Time be negative or not? The following constructor works: - rclcpp::Time time(-ONE_AND_HALF_SEC_IN_NS); - auto time_msg = static_cast(time); - EXPECT_EQ(time_msg.sec, -2); - EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS); - - // The opposite conversion throws... - EXPECT_ANY_THROW( - { - rclcpp::Time negative_time(time_msg); - }); - } } TEST_F(TestTime, operators) { From 255f6c83ebc76da900e525648e8ba30425ce86bf Mon Sep 17 00:00:00 2001 From: Nursharmin Ramli Date: Fri, 19 Apr 2024 19:07:45 +0200 Subject: [PATCH 3/5] Add throw to documentation Signed-off-by: Nursharmin Ramli --- rclcpp/include/rclcpp/time.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/rclcpp/include/rclcpp/time.hpp b/rclcpp/include/rclcpp/time.hpp index a931f3ac9c..7ebf9f21b8 100644 --- a/rclcpp/include/rclcpp/time.hpp +++ b/rclcpp/include/rclcpp/time.hpp @@ -49,6 +49,7 @@ class Time /** * \param nanoseconds since time epoch * \param clock_type clock type + * \throws std::runtime_error if nanoseconds are negative */ RCLCPP_PUBLIC explicit Time(int64_t nanoseconds = 0, rcl_clock_type_t clock_type = RCL_SYSTEM_TIME); From 632c4906cabaa264711be6c2323fef81335b57ed Mon Sep 17 00:00:00 2001 From: Nursharmin Ramli Date: Fri, 19 Apr 2024 19:16:08 +0200 Subject: [PATCH 4/5] Remove tests for Time underflow Signed-off-by: Nursharmin Ramli --- rclcpp/test/rclcpp/test_time.cpp | 38 ++++---------------------------- 1 file changed, 4 insertions(+), 34 deletions(-) diff --git a/rclcpp/test/rclcpp/test_time.cpp b/rclcpp/test/rclcpp/test_time.cpp index 90eb158b24..8406de4efb 100644 --- a/rclcpp/test/rclcpp/test_time.cpp +++ b/rclcpp/test/rclcpp/test_time.cpp @@ -286,31 +286,18 @@ TEST_F(TestTime, overflow_detectors) { TEST_F(TestTime, overflows) { rclcpp::Time max_time(std::numeric_limits::max()); - rclcpp::Time min_time(std::numeric_limits::min()); rclcpp::Duration one(1ns); rclcpp::Duration two(2ns); - // Cross min/max + // Cross max EXPECT_THROW(max_time + one, std::overflow_error); - EXPECT_THROW(min_time - one, std::underflow_error); - EXPECT_THROW(max_time - min_time, std::overflow_error); - EXPECT_THROW(min_time - max_time, std::underflow_error); EXPECT_THROW(rclcpp::Time(max_time) += one, std::overflow_error); - EXPECT_THROW(rclcpp::Time(min_time) -= one, std::underflow_error); EXPECT_NO_THROW(max_time - max_time); - EXPECT_NO_THROW(min_time - min_time); - // Cross zero in both directions + // Cross zero rclcpp::Time one_time(1); - EXPECT_NO_THROW(one_time - two); - EXPECT_NO_THROW(rclcpp::Time(one_time) -= two); - - rclcpp::Time minus_one_time(-1); - EXPECT_NO_THROW(minus_one_time + two); - EXPECT_NO_THROW(rclcpp::Time(minus_one_time) += two); - - EXPECT_NO_THROW(one_time - minus_one_time); - EXPECT_NO_THROW(minus_one_time - one_time); + EXPECT_THROW(one_time - two, std::runtime_error); + EXPECT_THROW(rclcpp::Time(one_time) -= two, std::runtime_error); rclcpp::Time two_time(2); EXPECT_NO_THROW(one_time - two_time); @@ -392,41 +379,24 @@ TEST_F(TestTime, test_overflow_underflow_throws) { RCLCPP_EXPECT_THROW_EQ( test_time = rclcpp::Time(INT64_MAX) + rclcpp::Duration(1ns), std::overflow_error("addition leads to int64_t overflow")); - RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Time(INT64_MIN) + rclcpp::Duration(-1ns), - std::underflow_error("addition leads to int64_t underflow")); RCLCPP_EXPECT_THROW_EQ( test_time = rclcpp::Time(INT64_MAX) - rclcpp::Duration(-1ns), std::overflow_error("time subtraction leads to int64_t overflow")); - RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Time(INT64_MIN) - rclcpp::Duration(1ns), - std::underflow_error("time subtraction leads to int64_t underflow")); test_time = rclcpp::Time(INT64_MAX); RCLCPP_EXPECT_THROW_EQ( test_time += rclcpp::Duration(1ns), std::overflow_error("addition leads to int64_t overflow")); - test_time = rclcpp::Time(INT64_MIN); - RCLCPP_EXPECT_THROW_EQ( - test_time += rclcpp::Duration(-1ns), - std::underflow_error("addition leads to int64_t underflow")); test_time = rclcpp::Time(INT64_MAX); RCLCPP_EXPECT_THROW_EQ( test_time -= rclcpp::Duration(-1ns), std::overflow_error("time subtraction leads to int64_t overflow")); - test_time = rclcpp::Time(INT64_MIN); - RCLCPP_EXPECT_THROW_EQ( - test_time -= rclcpp::Duration(1ns), - std::underflow_error("time subtraction leads to int64_t underflow")); RCLCPP_EXPECT_THROW_EQ( test_time = rclcpp::Duration::from_nanoseconds(INT64_MAX) + rclcpp::Time(1), std::overflow_error("addition leads to int64_t overflow")); - RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Duration::from_nanoseconds(INT64_MIN) + rclcpp::Time(-1), - std::underflow_error("addition leads to int64_t underflow")); } class TestClockSleep : public ::testing::Test From d148af2cf7a5c683e3672a59379f477d704398e9 Mon Sep 17 00:00:00 2001 From: Nursharmin Ramli Date: Fri, 19 Apr 2024 19:33:17 +0200 Subject: [PATCH 5/5] Add negative time check in operator Signed-off-by: Nursharmin Ramli --- rclcpp/src/rclcpp/time.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rclcpp/src/rclcpp/time.cpp b/rclcpp/src/rclcpp/time.cpp index 4548878c0b..e2780c04d8 100644 --- a/rclcpp/src/rclcpp/time.cpp +++ b/rclcpp/src/rclcpp/time.cpp @@ -253,6 +253,9 @@ Time::operator+=(const rclcpp::Duration & rhs) } rcl_time_.nanoseconds += rhs.nanoseconds(); + if (rcl_time_.nanoseconds < 0) { + throw std::runtime_error("cannot store a negative time point in rclcpp::Time"); + } return *this; } @@ -268,6 +271,9 @@ Time::operator-=(const rclcpp::Duration & rhs) } rcl_time_.nanoseconds -= rhs.nanoseconds(); + if (rcl_time_.nanoseconds < 0) { + throw std::runtime_error("cannot store a negative time point in rclcpp::Time"); + } return *this; }