| Message ID | 1520316568-8983-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Avoid overflow in wakeup time computation | expand |
Hi, On 06-03-18 07:09, selva.nair@gmail.com wrote: > From: Selva Nair <selva.nair@gmail.com> > > Time interval arithmetic can overflow especially when user > defined intervals are involved. E.g., see Trac #922. > > Avoid this by reordering the arithmetic operation in > event_timeout_trigger(). Also avoid unnecessary casting of time > variable to int. > > Time until wakeup is now calculated like: > > time_t wakeup = (last - now) + delay > > Here delay is of type int, but is +ve by construction. Time backtrack > protection in OpenVPN ensures (last - now) <= 0. Then the above > expression cannot overflow (provided time_t is at least as large > as int). > > A similar expression in interval.h is also changed. > > (This patch grew out of patch 168 by Steffan Karger.) > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > --- > src/openvpn/interval.c | 8 +++++--- > src/openvpn/interval.h | 2 +- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/openvpn/interval.c b/src/openvpn/interval.c > index 00ee627..b728560 100644 > --- a/src/openvpn/interval.c > +++ b/src/openvpn/interval.c > @@ -51,11 +51,12 @@ event_timeout_trigger(struct event_timeout *et, > > if (et->defined) > { > - int wakeup = (int) et->last + et->n - local_now; > + time_t wakeup = et->last - local_now + et->n; I would have preferred braces to not have any doubt about associativity, but can live with this. (I always forget, so had to look up what C did again.) > if (wakeup <= 0) > { > #if INTERVAL_DEBUG > - dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n, et_const_retry); > + dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n, > + et_const_retry); > #endif > if (et_const_retry < 0) > { > @@ -72,7 +73,8 @@ 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 (%d/%d) etcr=%d", > + (int) wakeup, et->n, et_const_retry); > #endif > tv->tv_sec = wakeup; > tv->tv_usec = 0; > diff --git a/src/openvpn/interval.h b/src/openvpn/interval.h > index 826a08b..5623f3a 100644 > --- a/src/openvpn/interval.h > +++ b/src/openvpn/interval.h > @@ -196,7 +196,7 @@ event_timeout_modify_wakeup(struct event_timeout *et, interval_t n) > static inline interval_t > event_timeout_remaining(struct event_timeout *et) > { > - return (int) et->last + et->n - now; > + return (interval_t) (et->last - now + et->n); > } > > /* > As discussed, an elegant way of solving the issue (verified manually with -fsanitize=undefined). Acked-by: Steffan Karger <steffan@karger.me> -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Your patch has been applied to the master and release/2.4 branch (bugfix).
Tested by running with the config from #922, which leads to
./../../openvpn/src/openvpn/interval.c:54:37: runtime error: signed integer overflow: 1521909676 + 1000000000 cannot be represented in type 'int'
without the patch, and no error with it (Linux/amd64).
commit f158c0e1df13ae1b697cdc7f189ddd1575a0c1aa (master)
commit 6d16c87de38610b05ec768af28a76a9389791134 (release/2.4)
Author: Selva Nair
Date: Tue Mar 6 01:09:28 2018 -0500
Avoid overflow in wakeup time computation
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <1520316568-8983-1-git-send-email-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16634.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpn/interval.c b/src/openvpn/interval.c index 00ee627..b728560 100644 --- a/src/openvpn/interval.c +++ b/src/openvpn/interval.c @@ -51,11 +51,12 @@ event_timeout_trigger(struct event_timeout *et, if (et->defined) { - int wakeup = (int) et->last + et->n - local_now; + time_t wakeup = et->last - local_now + et->n; if (wakeup <= 0) { #if INTERVAL_DEBUG - dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n, et_const_retry); + dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n, + et_const_retry); #endif if (et_const_retry < 0) { @@ -72,7 +73,8 @@ 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 (%d/%d) etcr=%d", + (int) wakeup, et->n, et_const_retry); #endif tv->tv_sec = wakeup; tv->tv_usec = 0; diff --git a/src/openvpn/interval.h b/src/openvpn/interval.h index 826a08b..5623f3a 100644 --- a/src/openvpn/interval.h +++ b/src/openvpn/interval.h @@ -196,7 +196,7 @@ event_timeout_modify_wakeup(struct event_timeout *et, interval_t n) static inline interval_t event_timeout_remaining(struct event_timeout *et) { - return (int) et->last + et->n - now; + return (interval_t) (et->last - now + et->n); } /*