diff mbox series

[Openvpn-devel] Check for time_t overflow in event_timeout_trigger()

Message ID 20180102222002.21454-1-steffan@karger.me
State Superseded
Headers show
Series [Openvpn-devel] Check for time_t overflow in event_timeout_trigger() | expand

Commit Message

Steffan Karger Jan. 2, 2018, 11:20 a.m. UTC 
As reported in trac #922, the wakeup computation in
event_timeout_trigger() could overflow.  Since time_t and int are signed
types, that is officially undefined behvaiour.

On systems with a 64-bit signed time_t (most if not all 64-bit system),
the overflow was caused by the (unnecessary) cast to "int".  Removing
that, changing the time of "wakeup" to time_t, and assuming that the
system clock on our host system will never be set to the year
292471210579 or later, this can no longer overflow on systems with a
64-bit time_t.

For systems with a signed 32-bit time_t however, we need an additional
check to prevent signed overflow (and thus undefined behaviour).  Since
time_t is only specified by C99 to be "an arithmetic type capable of
representing times", and no TIME_MAX or TIME_MIN macros are available,
checking for overflow is not trivial at all.  This patch takes the
practical approach, and assumes that time_t is of type "long int" (aka
"long") or "long long int" (aka "long long").  POSIX requires time_t to
be an "integer type", and all systems I know of use a long int or long
long int.  To be sure that this assumption holds, this patch uses static
asserts.

So much for this "simple" overflow check.

Trac: #922
Signed-off-by: Steffan Karger <steffan@karger.me>
---
 src/openvpn/integer.h  | 14 ++++++++++++++
 src/openvpn/interval.c |  5 +++--
 2 files changed, 17 insertions(+), 2 deletions(-)
diff mbox series

Patch

diff --git a/src/openvpn/integer.h b/src/openvpn/integer.h
index 9bb00a38..51085450 100644
--- a/src/openvpn/integer.h
+++ b/src/openvpn/integer.h
@@ -77,6 +77,20 @@  constrain_int(int x, int min, int max)
     }
 }
 
+/** Return true if the addition of a and b would overflow. */
+static inline bool
+time_t_add_overflow(time_t a, time_t b) {
+    static_assert(((time_t) -1) < 0, "OpenVPN assumes time_t is signed");
+    static_assert(((time_t) .9) == 0, "OpenVPN assumes time_t is integer type");
+    static_assert(sizeof(time_t) == sizeof(long) || sizeof(time_t) == sizeof(long long),
+        "OpenVPN assumes that time_t is of type long int or long long int");
+    static const time_t TIME_MAX = sizeof(time_t) == sizeof(long) ?
+            LONG_MAX : LLONG_MAX;
+    static const time_t TIME_MIN = sizeof(time_t) == sizeof(long) ?
+            LONG_MIN : LLONG_MIN;
+    return (a > 0 && b > TIME_MAX - a) || (a < 0 && b < TIME_MIN - a);
+}
+
 /*
  * Functions used for circular buffer index arithmetic.
  */
diff --git a/src/openvpn/interval.c b/src/openvpn/interval.c
index 16343865..1f067cd2 100644
--- a/src/openvpn/interval.c
+++ b/src/openvpn/interval.c
@@ -51,7 +51,8 @@  event_timeout_trigger(struct event_timeout *et,
 
     if (et->defined)
     {
-        int wakeup = (int) et->last + et->n - local_now;
+        ASSERT(!time_t_add_overflow(et->last, et->n));
+        time_t wakeup = et->last + et->n - local_now;
         if (wakeup <= 0)
         {
 #if INTERVAL_DEBUG
@@ -72,7 +73,7 @@  event_timeout_trigger(struct event_timeout *et,
         if (tv && wakeup < tv->tv_sec)
         {
 #if INTERVAL_DEBUG
-            dmsg(D_INTERVAL, "EVENT event_timeout_wakeup (%d/%d) etcr=%d", wakeup, et->n, et_const_retry);
+            dmsg(D_INTERVAL, "EVENT event_timeout_wakeup (%ld/%d) etcr=%d", wakeup, et->n, et_const_retry);
 #endif
             tv->tv_sec = wakeup;
             tv->tv_usec = 0;