[Openvpn-devel] Check for time_t overflow in event_timeout_trigger()
Commit Message
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(-)
@@ -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.
*/
@@ -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;