Merge branch 'ondrej/fix-isc_time_add-overflow' into 'main'

Fix isc_time_add() overflow

See merge request isc-projects/bind9!5511

configure.ac

@@ -1164,14 +1164,26 @@ AC_LINK_IFELSE(
      [[]],
      [[return (__builtin_clz(0xff) == 24 ? 1 : 0);]]
    )],
-  [have_builtin_clz=yes
+  [AC_MSG_RESULT([yes])
-   AC_MSG_RESULT(yes)],
+   AC_DEFINE(HAVE_BUILTIN_CLZ, 1, [Define to 1 if the compiler supports __builtin_clz.])
-  [have_builtin_clz=no
+  ],
-   AC_MSG_RESULT(no)]
+  [AC_MSG_RESULT([no])]
 )
-if test "yes" = "$have_builtin_clz"; then
+
-	AC_DEFINE(HAVE_BUILTIN_CLZ, 1, [Define to 1 if the compiler supports __builtin_clz.])
+#
-fi
+# Check for __builtin_uadd_overflow
+#
+AC_MSG_CHECKING([compiler support for __builtin_*_overflow()])
+AC_LINK_IFELSE(
+  [AC_LANG_PROGRAM(
+     [[#include <limits.h>]],
+     [[return (__builtin_uadd_overflow(UINT_MAX, UINT_MAX, &(unsigned int){ 0 }));]]
+   )],
+  [AC_MSG_RESULT([yes])
+   AC_DEFINE([HAVE_BUILTIN_OVERFLOW], [1], [define if the compiler supports __builtin_*_overflow().])
+  ],
+  [AC_MSG_RESULT([no])
+  ])
 
 #
 # Activate "rrset-order fixed" or not?

lib/isc/tests/time_test.c

@@ -26,6 +26,111 @@
 #include <isc/time.h>
 #include <isc/util.h>
 
+#include "../time.c"
+
+#define NS_PER_S 1000000000 /*%< Nanoseconds per second. */
+#define MAX_NS	 (NS_PER_S - 1)
+
+struct time_vectors {
+	isc_time_t a;
+	isc_interval_t b;
+	isc_time_t r;
+	isc_result_t result;
+};
+
+const struct time_vectors vectors_add[8] = {
+	{ { 0, 0 }, { 0, 0 }, { 0, 0 }, ISC_R_SUCCESS },
+	{ { 0, MAX_NS }, { 0, MAX_NS }, { 1, MAX_NS - 1 }, ISC_R_SUCCESS },
+	{ { 0, NS_PER_S / 2 }, { 0, NS_PER_S / 2 }, { 1, 0 }, ISC_R_SUCCESS },
+	{ { UINT_MAX, MAX_NS }, { 0, 0 }, { UINT_MAX, MAX_NS }, ISC_R_SUCCESS },
+	{ { UINT_MAX, 0 }, { 0, MAX_NS }, { UINT_MAX, MAX_NS }, ISC_R_SUCCESS },
+	{ { UINT_MAX, 0 }, { 1, 0 }, { 0, 0 }, ISC_R_RANGE },
+	{ { UINT_MAX, MAX_NS }, { 0, 1 }, { 0, 0 }, ISC_R_RANGE },
+	{ { UINT_MAX / 2 + 1, NS_PER_S / 2 },
+	  { UINT_MAX / 2, NS_PER_S / 2 },
+	  { 0, 0 },
+	  ISC_R_RANGE },
+};
+
+const struct time_vectors vectors_sub[7] = {
+	{ { 0, 0 }, { 0, 0 }, { 0, 0 }, ISC_R_SUCCESS },
+	{ { 1, 0 }, { 0, MAX_NS }, { 0, 1 }, ISC_R_SUCCESS },
+	{ { 1, NS_PER_S / 2 },
+	  { 0, MAX_NS },
+	  { 0, NS_PER_S / 2 + 1 },
+	  ISC_R_SUCCESS },
+	{ { UINT_MAX, MAX_NS }, { UINT_MAX, 0 }, { 0, MAX_NS }, ISC_R_SUCCESS },
+	{ { 0, 0 }, { 1, 0 }, { 0, 0 }, ISC_R_RANGE },
+	{ { 0, 0 }, { 0, MAX_NS }, { 0, 0 }, ISC_R_RANGE },
+};
+
+static void
+isc_time_add_test(void **state) {
+	UNUSED(state);
+
+	for (size_t i = 0; i < ARRAY_SIZE(vectors_add); i++) {
+		isc_time_t r = { UINT_MAX, UINT_MAX };
+		isc_result_t result = isc_time_add(&(vectors_add[i].a),
+						   &(vectors_add[i].b), &r);
+		assert_int_equal(result, vectors_add[i].result);
+		if (result != ISC_R_SUCCESS) {
+			continue;
+		}
+
+		assert_int_equal(r.seconds, vectors_add[i].r.seconds);
+		assert_int_equal(r.nanoseconds, vectors_add[i].r.nanoseconds);
+	}
+
+	expect_assert_failure((void)isc_time_add(&(isc_time_t){ 0, MAX_NS + 1 },
+						 &(isc_interval_t){ 0, 0 },
+						 &(isc_time_t){ 0, 0 }));
+	expect_assert_failure((void)isc_time_add(
+		&(isc_time_t){ 0, 0 }, &(isc_interval_t){ 0, MAX_NS + 1 },
+		&(isc_time_t){ 0, 0 }));
+
+	expect_assert_failure((void)isc_time_add((isc_time_t *)NULL,
+						 &(isc_interval_t){ 0, 0 },
+						 &(isc_time_t){ 0, 0 }));
+	expect_assert_failure((void)isc_time_add(&(isc_time_t){ 0, 0 },
+						 (isc_interval_t *)NULL,
+						 &(isc_time_t){ 0, 0 }));
+	expect_assert_failure((void)isc_time_add(
+		&(isc_time_t){ 0, 0 }, &(isc_interval_t){ 0, 0 }, NULL));
+}
+
+static void
+isc_time_sub_test(void **state) {
+	UNUSED(state);
+
+	for (size_t i = 0; i < ARRAY_SIZE(vectors_sub); i++) {
+		isc_time_t r = { UINT_MAX, UINT_MAX };
+		isc_result_t result = isc_time_subtract(
+			&(vectors_sub[i].a), &(vectors_sub[i].b), &r);
+		assert_int_equal(result, vectors_sub[i].result);
+		if (result != ISC_R_SUCCESS) {
+			continue;
+		}
+		assert_int_equal(r.seconds, vectors_sub[i].r.seconds);
+		assert_int_equal(r.nanoseconds, vectors_sub[i].r.nanoseconds);
+	}
+
+	expect_assert_failure((void)isc_time_subtract(
+		&(isc_time_t){ 0, MAX_NS + 1 }, &(isc_interval_t){ 0, 0 },
+		&(isc_time_t){ 0, 0 }));
+	expect_assert_failure((void)isc_time_subtract(
+		&(isc_time_t){ 0, 0 }, &(isc_interval_t){ 0, MAX_NS + 1 },
+		&(isc_time_t){ 0, 0 }));
+
+	expect_assert_failure((void)isc_time_subtract((isc_time_t *)NULL,
+						      &(isc_interval_t){ 0, 0 },
+						      &(isc_time_t){ 0, 0 }));
+	expect_assert_failure((void)isc_time_subtract(&(isc_time_t){ 0, 0 },
+						      (isc_interval_t *)NULL,
+						      &(isc_time_t){ 0, 0 }));
+	expect_assert_failure((void)isc_time_subtract(
+		&(isc_time_t){ 0, 0 }, &(isc_interval_t){ 0, 0 }, NULL));
+}
+
 /* parse http time stamp */
 static void
 isc_time_parsehttptimestamp_test(void **state) {
@@ -295,6 +400,8 @@ isc_time_formatshorttimestamp_test(void **state) {
 int
 main(void) {
 	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(isc_time_add_test),
+		cmocka_unit_test(isc_time_sub_test),
 		cmocka_unit_test(isc_time_parsehttptimestamp_test),
 		cmocka_unit_test(isc_time_formatISO8601_test),
 		cmocka_unit_test(isc_time_formatISO8601ms_test),

lib/isc/time.c

@@ -53,8 +53,10 @@
  *** Intervals
  ***/
 
+#if !defined(UNIT_TESTING)
 static const isc_interval_t zero_interval = { 0, 0 };
 const isc_interval_t *const isc_interval_zero = &zero_interval;
+#endif
 
 void
 isc_interval_set(isc_interval_t *i, unsigned int seconds,
@@ -90,8 +92,10 @@ isc_interval_ms(const isc_interval_t *i) {
  *** Absolute Times
  ***/
 
+#if !defined(UNIT_TESTING)
 static const isc_time_t epoch = { 0, 0 };
 const isc_time_t *const isc_time_epoch = &epoch;
+#endif
 
 void
 isc_time_set(isc_time_t *t, unsigned int seconds, unsigned int nanoseconds) {
@@ -228,25 +232,28 @@ isc_time_compare(const isc_time_t *t1, const isc_time_t *t2) {
 isc_result_t
 isc_time_add(const isc_time_t *t, const isc_interval_t *i, isc_time_t *result) {
 	REQUIRE(t != NULL && i != NULL && result != NULL);
-	INSIST(t->nanoseconds < NS_PER_S && i->nanoseconds < NS_PER_S);
+	REQUIRE(t->nanoseconds < NS_PER_S && i->nanoseconds < NS_PER_S);
 
-	/*
+	/* Seconds */
-	 * Ensure the resulting seconds value fits in the size of an
+#if HAVE_BUILTIN_OVERFLOW
-	 * unsigned int.  (It is written this way as a slight optimization;
+	if (__builtin_uadd_overflow(t->seconds, i->seconds, &result->seconds)) {
-	 * note that even if both values == INT_MAX, then when added
+		return (ISC_R_RANGE);
-	 * and getting another 1 added below the result is UINT_MAX.)
+	}
-	 */
+#else
-	if ((t->seconds > INT_MAX || i->seconds > INT_MAX) &&
+	if (t->seconds > UINT_MAX - i->seconds) {
-	    ((long long)t->seconds + i->seconds > UINT_MAX))
-	{
 		return (ISC_R_RANGE);
 	}
-
 	result->seconds = t->seconds + i->seconds;
+#endif
+
+	/* Nanoseconds */
 	result->nanoseconds = t->nanoseconds + i->nanoseconds;
 	if (result->nanoseconds >= NS_PER_S) {
-		result->seconds++;
+		if (result->seconds == UINT_MAX) {
+			return (ISC_R_RANGE);
+		}
 		result->nanoseconds -= NS_PER_S;
+		result->seconds++;
 	}
 
 	return (ISC_R_SUCCESS);
@@ -256,22 +263,30 @@ isc_result_t
 isc_time_subtract(const isc_time_t *t, const isc_interval_t *i,
 		  isc_time_t *result) {
 	REQUIRE(t != NULL && i != NULL && result != NULL);
-	INSIST(t->nanoseconds < NS_PER_S && i->nanoseconds < NS_PER_S);
+	REQUIRE(t->nanoseconds < NS_PER_S && i->nanoseconds < NS_PER_S);
 
-	if ((unsigned int)t->seconds < i->seconds ||
+	/* Seconds */
-	    ((unsigned int)t->seconds == i->seconds &&
+#if HAVE_BUILTIN_OVERFLOW
-	     t->nanoseconds < i->nanoseconds))
+	if (__builtin_usub_overflow(t->seconds, i->seconds, &result->seconds)) {
-	{
+		return (ISC_R_RANGE);
+	}
+#else
+	if (t->seconds < i->seconds) {
 		return (ISC_R_RANGE);
 	}
-
 	result->seconds = t->seconds - i->seconds;
+#endif
+
+	/* Nanoseconds */
 	if (t->nanoseconds >= i->nanoseconds) {
 		result->nanoseconds = t->nanoseconds - i->nanoseconds;
 	} else {
-		result->nanoseconds = NS_PER_S - i->nanoseconds +
+		if (result->seconds == 0) {
-				      t->nanoseconds;
+			return (ISC_R_RANGE);
+		}
 		result->seconds--;
+		result->nanoseconds = NS_PER_S + t->nanoseconds -
+				      i->nanoseconds;
 	}
 
 	return (ISC_R_SUCCESS);
@@ -318,20 +333,20 @@ isc_time_secondsastimet(const isc_time_t *t, time_t *secondsp) {
 
 	/*
 	 * Ensure that the number of seconds represented by t->seconds
-	 * can be represented by a time_t.  Since t->seconds is an unsigned
+	 * can be represented by a time_t.  Since t->seconds is an
-	 * int and since time_t is mostly opaque, this is trickier than
+	 * unsigned int and since time_t is mostly opaque, this is
-	 * it seems.  (This standardized opaqueness of time_t is *very*
+	 * trickier than it seems.  (This standardized opaqueness of
-	 * frustrating; time_t is not even limited to being an integral
+	 * time_t is *very* frustrating; time_t is not even limited to
-	 * type.)
+	 * being an integral type.)
 	 *
 	 * The mission, then, is to avoid generating any kind of warning
-	 * about "signed versus unsigned" while trying to determine if the
+	 * about "signed versus unsigned" while trying to determine if
-	 * the unsigned int t->seconds is out range for tv_sec, which is
+	 * the unsigned int t->seconds is out range for tv_sec,
-	 * pretty much only true if time_t is a signed integer of the same
+	 * which is pretty much only true if time_t is a signed integer
-	 * size as the return value of isc_time_seconds.
+	 * of the same size as the return value of isc_time_seconds.
 	 *
-	 * If the paradox in the if clause below is true, t->seconds is out
+	 * If the paradox in the if clause below is true, t->seconds is
-	 * of range for time_t.
+	 * out of range for time_t.
 	 */
 	seconds = (time_t)t->seconds;
 
@@ -390,7 +405,8 @@ isc_time_formathttptimestamp(const isc_time_t *t, char *buf, unsigned int len) {
 	REQUIRE(len > 0);
 
 	/*
-	 * 5 spaces, 1 comma, 3 GMT, 2 %d, 4 %Y, 8 %H:%M:%S, 3+ %a, 3+ %b (29+)
+	 * 5 spaces, 1 comma, 3 GMT, 2 %d, 4 %Y, 8 %H:%M:%S, 3+ %a, 3+
+	 * %b (29+)
 	 */
 	now = (time_t)t->seconds;
 	flen = strftime(buf, len, "%a, %d %b %Y %H:%M:%S GMT",