Address review feedback:

* Add new time.h/c * Update documentation to distinguish between unspecified/infinite * Add overflow/bounds checking for time utils Signed-off-by: Emerson Knapp <[email protected]>
ros2 · Mar 1, 2021 · ab0950d · ab0950d
1 parent 94f4f33
commit ab0950d
Show file tree

Hide file tree

Showing 8 changed files with 310 additions and 172 deletions.
diff --git a/rmw/CMakeLists.txt b/rmw/CMakeLists.txt
@@ -36,6 +36,7 @@ set(rmw_sources
   "src/sanity_checks.c"
   "src/security_options.c"
   "src/subscription_options.c"
+  "src/time.c"
   "src/topic_endpoint_info_array.c"
   "src/topic_endpoint_info.c"
   "src/types.c"

diff --git a/rmw/include/rmw/time.h b/rmw/include/rmw/time.h
@@ -0,0 +1,89 @@
+// Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef RMW__TIME_H_
+#define RMW__TIME_H_
+
+#ifdef __cplusplus
+extern "C"
+{
+#endif  // __cplusplus
+
+#include <stdint.h>
+
+#include "rcutils/time.h"
+
+#include "rmw/macros.h"
+#include "rmw/visibility_control.h"
+
+/// A struct representing a duration or relative time in RMW - does not encode an origin.
+typedef struct RMW_PUBLIC_TYPE rmw_time_t
+{
+  /// Seconds component
+  uint64_t sec;
+
+  /// Nanoseconds component
+  uint64_t nsec;
+} rmw_time_t;
+
+typedef rcutils_time_point_value_t rmw_time_point_value_t;
+typedef rcutils_duration_value_t rmw_duration_t;
+
+/// Constant representing an infinite duration. Use rmw_time_equal for comparisons.
+/**
+  * Different RMW implementations have different representations for infinite durations.
+  * This value is reported for QoS policy durations that are left unspecified.
+  * Do not directly compare `sec == sec && nsec == nsec`, because we don't want to be sensitive
+  * to non-normalized values (nsec > 1 second) - use rmw_time_equal instead.
+  * This value is INT64_MAX nanoseconds = 0x7FFF FFFF FFFF FFFF = d 9 223 372 036 854 775 807
+  */
+static const struct rmw_time_t RMW_DURATION_INFINITE = {9223372036LL, 854775807LL};
+static const struct rmw_time_t RMW_DURATION_UNSPECIFIED = {0LL, 0LL};
+
+/// Check whether two rmw_time_t represent the same time.
+RMW_PUBLIC
+RMW_WARN_UNUSED
+bool
+rmw_time_equal(const rmw_time_t left, const rmw_time_t right);
+
+/// Return the total nanosecond representation of a time.
+/**
+  * \return INT64_MAX if input is too large to store in 64 bits
+  */
+RMW_PUBLIC
+RMW_WARN_UNUSED
+rmw_duration_t
+rmw_time_total_nsec(const rmw_time_t time);
+
+/// Construct rmw_time_t from a total nanoseconds representation.
+/**
+  * rmw_time_t only specifies relative time, so the origin is not relevant for this calculation.
+  * \return RMW_DURATION_INFINITE if input is negative, which is not representable in rmw_time_t
+  */
+RMW_PUBLIC
+RMW_WARN_UNUSED
+rmw_time_t
+rmw_time_from_nsec(const rmw_duration_t nanoseconds);
+
+/// Ensure that an rmw_time_t does not have nanoseconds > 1 second.
+RMW_PUBLIC
+RMW_WARN_UNUSED
+rmw_time_t
+rmw_time_normalize(const rmw_time_t time);
+
+#ifdef __cplusplus
+}
+#endif  // __cplusplus
+
+#endif  // RMW__TIME_H_
diff --git a/rmw/include/rmw/types.h b/rmw/include/rmw/types.h
@@ -33,6 +33,7 @@ extern "C"
 #include "rmw/ret_types.h"
 #include "rmw/security_options.h"
 #include "rmw/serialized_message.h"
+#include "rmw/time.h"
 #include "rmw/visibility_control.h"
 
 // 24 bytes is the most memory needed to represent the GID by any current
@@ -318,53 +319,6 @@ typedef struct RMW_PUBLIC_TYPE rmw_request_id_t
   int64_t sequence_number;
 } rmw_request_id_t;
 
-/// Struct representing a time point for rmw
-typedef struct RMW_PUBLIC_TYPE rmw_time_t
-{
-  /// Seconds since the epoch
-  uint64_t sec;
-
-  /// Nanoseconds component of this time point
-  uint64_t nsec;
-} rmw_time_t;
-
-typedef rcutils_time_point_value_t rmw_time_point_value_t;
-typedef rcutils_duration_value_t rmw_duration_t;
-
-/// Constant representing an infinite duration. Use rmw_time_equal for comparisons.
-/**
-  * Different RMW implementations have different representations for infinite durations.
-  * This value is reported for QoS policy durations that are left unspecified.
-  * Do not directly comparee `sec == sec && nsec == nsec`, because we don't want to be sensitive
-  * to non-normalized values (nsec > 1 second) - use rmw_time_equal instead.
-  * This value is INT64_MAX nanoseconds = 0x7FFF FFFF FFFF FFFF = d 9 223 372 036 854 775 807
-  */
-#define RMW_DURATION_INFINITE {9223372036LL, 854775807LL}
-
-/// Check whether two rmw_time_t represent the same time.
-RMW_PUBLIC
-RMW_WARN_UNUSED
-bool
-rmw_time_equal(const rmw_time_t left, const rmw_time_t right);
-
-/// Return the total nanosecond representation of a time.
-RMW_PUBLIC
-RMW_WARN_UNUSED
-rmw_duration_t
-rmw_time_total_nsec(const rmw_time_t time);
-
-/// Construct rmw_time_t from a total nanoseconds representation.
-RMW_PUBLIC
-RMW_WARN_UNUSED
-rmw_time_t
-rmw_time_from_nsec(const rmw_duration_t nanoseconds);
-
-/// Ensure that an rmw_time_t does not have nanoseconds > 1 second.
-RMW_PUBLIC
-RMW_WARN_UNUSED
-rmw_time_t
-rmw_time_normalize(const rmw_time_t time);
-
 /// Meta-data for a service-related take.
 typedef struct RMW_PUBLIC_TYPE rmw_service_info_t
 {
@@ -459,16 +413,14 @@ enum RMW_PUBLIC_TYPE rmw_qos_liveliness_policy_t
   RMW_QOS_POLICY_LIVELINESS_UNKNOWN = 4
 };
 
-#define RMW_QOS_DURATION_UNSPECIFIED {0, 0}
-
-/// QoS Deadline default, 0s indicates deadline policies are not tracked or enforced
-#define RMW_QOS_DEADLINE_DEFAULT RMW_QOS_DURATION_UNSPECIFIED
+/// QoS Deadline default.
+#define RMW_QOS_DEADLINE_DEFAULT RMW_DURATION_UNSPECIFIED
 
-/// QoS Lifespan default, 0s indicate lifespan policies are not tracked or enforced
-#define RMW_QOS_LIFESPAN_DEFAULT RMW_QOS_DURATION_UNSPECIFIED
+/// QoS Lifespan default.
+#define RMW_QOS_LIFESPAN_DEFAULT RMW_DURATION_UNSPECIFIED
 
-/// QoS Liveliness lease duration default, 0s indicate lease durations are not tracked or enforced
-#define RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT RMW_QOS_DURATION_UNSPECIFIED
+/// QoS Liveliness lease duration default.
+#define RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT RMW_DURATION_UNSPECIFIED
 
 /// ROS MiddleWare quality of service profile.
 typedef struct RMW_PUBLIC_TYPE rmw_qos_profile_t
@@ -481,12 +433,27 @@ typedef struct RMW_PUBLIC_TYPE rmw_qos_profile_t
   /// Durability QoS policy setting
   enum rmw_qos_durability_policy_t durability;
   /// The period at which messages are expected to be sent/received
+  /**
+    * RMW_DURATION_UNSPEFICIED will use the RMW implementation's default value,
+    *   which may or may not be infinite.
+    * RMW_DURATION_INFINITE explicitly states that messages never miss a deadline expectation.
+    */
   struct rmw_time_t deadline;
   /// The age at which messages are considered expired and no longer valid
+  /**
+    * RMW_DURATION_UNSPEFICIED will use the RMW implementation's default value,
+    *   which may or may not be infinite.
+    * RMW_DURATION_INFINITE explicitly states that messages do not expire.
+    */
   struct rmw_time_t lifespan;
   /// Liveliness QoS policy setting
   enum rmw_qos_liveliness_policy_t liveliness;
   /// The time within which the RMW node or publisher must show that it is alive
+  /**
+    * RMW_DURATION_UNSPEFICIED will use the RMW implementation's default value,
+    *   which may or may not be infinite.
+    * RMW_DURATION_INFINITE explicitly states that liveliness is not enforced.
+    */
   struct rmw_time_t liveliness_lease_duration;
 
   /// If true, any ROS specific namespacing conventions will be circumvented.

diff --git a/rmw/src/time.c b/rmw/src/time.c
@@ -0,0 +1,68 @@
+// Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "rmw/time.h"
+
+#include "rcutils/time.h"
+
+RMW_PUBLIC
+RMW_WARN_UNUSED
+bool
+rmw_time_equal(const rmw_time_t left, const rmw_time_t right)
+{
+  return rmw_time_total_nsec(left) == rmw_time_total_nsec(right);
+}
+
+RMW_PUBLIC
+RMW_WARN_UNUSED
+rmw_duration_t
+rmw_time_total_nsec(const rmw_time_t time)
+{
+  static const int64_t max_sec = INT64_MAX / RCUTILS_S_TO_NS(1);
+  if ((int64_t)time.sec > max_sec) {
+    // Seconds not representable in nanoseconds
+    return INT64_MAX;
+  }
+
+  const int64_t sec_as_nsec = RCUTILS_S_TO_NS(time.sec);
+  if ((int64_t)time.nsec > (INT64_MAX - sec_as_nsec)) {
+    // overflow
+    return INT64_MAX;
+  }
+  return sec_as_nsec + time.nsec;
+}
+
+RMW_PUBLIC
+RMW_WARN_UNUSED
+rmw_time_t
+rmw_time_from_nsec(const rmw_duration_t nanoseconds)
+{
+  if (nanoseconds < 0) {
+    return RMW_DURATION_INFINITE;
+  }
+
+  // Avoid typing the 1 billion constant
+  rmw_time_t time;
+  time.sec = RCUTILS_NS_TO_S(nanoseconds);
+  time.nsec = nanoseconds % RCUTILS_S_TO_NS(1);
+  return time;
+}
+
+RMW_PUBLIC
+RMW_WARN_UNUSED
+rmw_time_t
+rmw_time_normalize(const rmw_time_t time)
+{
+  return rmw_time_from_nsec(rmw_time_total_nsec(time));
+}
diff --git a/rmw/src/types.c b/rmw/src/types.c
@@ -14,8 +14,6 @@
 
 #include "rmw/types.h"
 
-#include "rcutils/time.h"
-
 RMW_PUBLIC
 RMW_WARN_UNUSED
 rmw_message_info_t
@@ -24,39 +22,3 @@ rmw_get_zero_initialized_message_info(void)
   rmw_message_info_t zero_initialized_message_info = {0, 0, {NULL, {0}}, false};
   return zero_initialized_message_info;
 }
-
-RMW_PUBLIC
-RMW_WARN_UNUSED
-bool
-rmw_time_equal(const rmw_time_t left, const rmw_time_t right)
-{
-  return rmw_time_total_nsec(left) == rmw_time_total_nsec(right);
-}
-
-RMW_PUBLIC
-RMW_WARN_UNUSED
-rmw_duration_t
-rmw_time_total_nsec(const rmw_time_t time)
-{
-  return RCUTILS_S_TO_NS(time.sec) + time.nsec;
-}
-
-RMW_PUBLIC
-RMW_WARN_UNUSED
-rmw_time_t
-rmw_time_from_nsec(const rmw_duration_t nanoseconds)
-{
-  // Avoid typing the 1 billion constant
-  rmw_time_t time;
-  time.sec = RCUTILS_NS_TO_S(nanoseconds);
-  time.nsec = nanoseconds % RCUTILS_S_TO_NS(1);
-  return time;
-}
-
-RMW_PUBLIC
-RMW_WARN_UNUSED
-rmw_time_t
-rmw_time_normalize(const rmw_time_t time)
-{
-  return rmw_time_from_nsec(rmw_time_total_nsec(time));
-}
diff --git a/rmw/test/CMakeLists.txt b/rmw/test/CMakeLists.txt
@@ -121,6 +121,15 @@ if(TARGET test_subscription_options)
   target_link_libraries(test_subscription_options ${PROJECT_NAME})
 endif()
 
+ament_add_gmock(test_time
+  test_time.cpp
+  # Append the directory of librmw so it is found at test time.
+  APPEND_LIBRARY_DIRS "$<TARGET_FILE_DIR:${PROJECT_NAME}>"
+)
+if(TARGET test_time)
+  target_link_libraries(test_time ${PROJECT_NAME})
+endif()
+
 ament_add_gmock(test_types
   test_types.cpp
   # Append the directory of librmw so it is found at test time.