Skip to contentSkip to conversation

Avoid overflow in wakeup time computation

PATCH In review [email protected]... 3 participants 3 messages Mar 6, 2018 – Mar 24, 2018

lore
Share & tools
lore

[email protected]...

author
#0 lore raw

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.)

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 +5 -3
index 00ee627..b728560 100644
@@ -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
index 826a08b..5623f3a 100644
@@ -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
Acked-by on v1 1/1
#1 lore raw

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_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 <[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
index 00ee627..b728560 100644

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 -2
         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 +0 -0
index 826a08b..5623f3a 100644

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).

Acked-by Steffan Karger <[email protected]...>

-Steffan

#2 lore raw
In reply to [email protected]...

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