Avoid overflow in wakeup time computation
PATCH 1/1 Avoid overflow in wakeup time computation
From: Selva Nair <[email protected]...>
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.)
--- src/openvpn/interval.c | 8 +++++--- src/openvpn/interval.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)
src/openvpn/interval.c +5 -3
@@ -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 ;
src/openvpn/interval.h +1 -1
@@ -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); }/*
Signature · 1 line
2.1.4
Hi,
Quoted 33 lines · On 06-03-18 07:09, [email protected]... wrote:
From: Selva Nair <[email protected]...>
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_twakeup = (last - now) + delayHere 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_tis at least as large as int).A similar expression in
interval.his also changed.(This patch grew out of patch 168 by Steffan Karger.)
Signed-off-by Selva Nair <[email protected]...>--- src/openvpn/interval.c | 8 +++++--- src/openvpn/interval.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)src/openvpn/interval.c +0
-0
src/openvpn/interval.c event_timeout_trigger(struct event_timeout *et, · line 54
Quoted 5 lines
src/openvpn/interval.c +1
-1@@ -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.)
Quoted 25 lines
src/openvpn/interval.c +4
-2if (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 ;src/openvpn/interval.h +0
-0
src/openvpn/interval.h event_timeout_modify_wakeup(struct event_timeout *et, interval_t n) · line 202
Quoted 8 lines
src/openvpn/interval.h +1
-1@@ -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).
-Steffan
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 <[email protected]...>
Acked-by: Steffan Karger <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg16634.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering