Fix time_t printing

any opinion regarding this diff?

When building openvpn-2.4.4 on OpenBSD, I noticed the following warning:

--8<--
cc -DHAVE_CONFIG_H -I. -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn -I../.. -I../../include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/compat -I/usr/local/include -I/usr/local/include -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\" -O2 -pipe -std=c99 -MT error.o -MD -MP -MF .deps/error.Tpo -c -o error.o /usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c
/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c:346:25: warning: format specifies type 'unsigned long' but the argument has type 'time_t' (aka 'long long') [-Wformat]

                        tv.tv_sec,
                        ^~~~~~~~~

1 warning generated.
mv -f .deps/error.Tpo .deps/error.Po
-->8--

OpenBSD uses long long for time_t on all architectures, 32 or 64 bits, in order to cope with dates beyond 2038. This is also the case on NetBSD and Linux x32.

The warning is not innocuous, as a mismatch between the format and the type of parameters passed to variadic functions can result in nasty problems (crashes, etc). For example, the code below crashes on OpenBSD/arm (32 bits long).

--8<--
#include <stdio.h>
#include <time.h>

int
main(void)

{
        time_t t;

        time(&t);
        printf("%ld %s\n", t, "foobar");
        return 0;
}

-->8--

The diff below fixes the potential issue and the warning. The method used is a cast to (long long), a method successfully used since OpenBSD switched to a 64 bits time_t. More data at

https://www.openbsd.org/papers/eurobsdcon_2013_time_t/mgp00029.html

openvpn already uses long long in a few places. Note that I did not audit the whole openvpn tree for other possible time_t problems, but I can't spot similar warnings in the build logs.
From d620431f661375d3564b60f110d1f69575ac78d7 Mon Sep 17 00:00:00 2001
From: Jeremie Courreges-Anglas <[email protected]...>
Date: Thu, 5 Oct 2017 01:43:33 +0200
Subject: [PATCH] Cast time_t to long double in order to print it.

The underlying type of time_t can be anything from unsigned 32 bits to signed 64 bits to float. To reliably print it, better cast it to "long long", which is at least 64 bits wide and can represent values beyond 2038.

Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas:

Quoted 49 lines

When building openvpn-2.4.4 on OpenBSD, I noticed the following warning:

--8<--
cc -DHAVE_CONFIG_H -I. -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn -I../.. -I../../include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/compat -I/usr/local/include -I/usr/local/include -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\" -O2 -pipe -std=c99 -MT error.o -MD -MP -MF .deps/error.Tpo -c -o error.o /usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c
/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c:346:25: warning: format specifies type 'unsigned long' but the argument has type 'time_t' (aka 'long long') [-Wformat]

                        tv.tv_sec,
                        ^~~~~~~~~

1 warning generated.
mv -f .deps/error.Tpo .deps/error.Po
-->8--

OpenBSD uses long long for time_t on all architectures, 32 or 64 bits, in order to cope with dates beyond 2038. This is also the case on NetBSD and Linux x32.

The warning is not innocuous, as a mismatch between the format and the type of parameters passed to variadic functions can result in nasty problems (crashes, etc). For example, the code below crashes on OpenBSD/arm (32 bits long).

--8<--
#include <stdio.h>
#include <time.h>

int
main(void)

{
        time_t t;

        time(&t);
        printf("%ld %s\n", t, "foobar");
        return 0;
}

-->8--

The diff below fixes the potential issue and the warning. The method used is a cast to (long long), a method successfully used since OpenBSD switched to a 64 bits time_t. More data at

https://www.openbsd.org/papers/eurobsdcon_2013_time_t/mgp00029.html

openvpn already uses long long in a few places. Note that I did not audit the whole openvpn tree for other possible time_t problems, but I can't spot similar warnings in the build logs.
From d620431f661375d3564b60f110d1f69575ac78d7 Mon Sep 17 00:00:00 2001
From: Jeremie Courreges-Anglas <[email protected]...>
Date: Thu, 5 Oct 2017 01:43:33 +0200
Subject: [PATCH] Cast time_t to long double in order to print it.

Quoted 4 lines

The underlying type of time_t can be anything from unsigned 32 bits to signed 64 bits to float. To reliably print it, better cast it to "long long", which is at least 64 bits wide and can represent values beyond 2038.

NAK.

This is due to the inconsistent and misleading commit log.
The subject says "cast to long double", the code does "cast to long long".

The long commit log states time_t could be a float, which it cannot be according to IEEE Std 1003.1, 2013 edition, which, in the sys/types.h section, requires that time_t shall be an integer type (no mention if it's signed or unsigned). (clock_t could be a floating-point type however).  This is a POSIX restriction over ISO 9899.

I propose to either:
* cast it to uintmax_t and use the PRIuMAX macro in the *printf function,
* cast it to unsigned long long and use %llu to print.

While we are at it, note that tv.tv_usec, per POSIX, is a SIGNED integer type capable of representing numbers in the range [-1; 1,000,000] so casting tv_usec to unsigned long without checking if it's -1 is technically wrong, although I'd be surprised to see gettimeofday() return a negative tv_usec - but UINT_MAX or ULONG_MAX likewise.

Quoted 55 lines · Matthias Andree

Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas:

Quoted 49 lines

When building openvpn-2.4.4 on OpenBSD, I noticed the following warning:

--8<--
cc -DHAVE_CONFIG_H -I. -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn -I../.. -I../../include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/compat -I/usr/local/include -I/usr/local/include -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\" -O2 -pipe -std=c99 -MT error.o -MD -MP -MF .deps/error.Tpo -c -o error.o /usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c
/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c:346:25: warning: format specifies type 'unsigned long' but the argument has type 'time_t' (aka 'long long') [-Wformat]

                        tv.tv_sec,
                        ^~~~~~~~~

1 warning generated.
mv -f .deps/error.Tpo .deps/error.Po
-->8--

OpenBSD uses long long for time_t on all architectures, 32 or 64 bits, in order to cope with dates beyond 2038. This is also the case on NetBSD and Linux x32.

The warning is not innocuous, as a mismatch between the format and the type of parameters passed to variadic functions can result in nasty problems (crashes, etc). For example, the code below crashes on OpenBSD/arm (32 bits long).

--8<--
#include <stdio.h>
#include <time.h>

int
main(void)

{
        time_t t;

        time(&t);
        printf("%ld %s\n", t, "foobar");
        return 0;
}

-->8--

The diff below fixes the potential issue and the warning. The method used is a cast to (long long), a method successfully used since OpenBSD switched to a 64 bits time_t. More data at

https://www.openbsd.org/papers/eurobsdcon_2013_time_t/mgp00029.html

openvpn already uses long long in a few places. Note that I did not audit the whole openvpn tree for other possible time_t problems, but I can't spot similar warnings in the build logs.
From d620431f661375d3564b60f110d1f69575ac78d7 Mon Sep 17 00:00:00 2001
From: Jeremie Courreges-Anglas <[email protected]...>
Date: Thu, 5 Oct 2017 01:43:33 +0200
Subject: [PATCH] Cast time_t to long double in order to print it.

Quoted 4 lines

The underlying type of time_t can be anything from unsigned 32 bits to signed 64 bits to float. To reliably print it, better cast it to "long long", which is at least 64 bits wide and can represent values beyond 2038.

Quoted 4 lines

NAK.

This is due to the inconsistent and misleading commit log.
The subject says "cast to long double", the code does "cast to long long".

oops, sorry about that.

Quoted 5 lines

The long commit log states time_t could be a float, which it cannot be according to IEEE Std 1003.1, 2013 edition, which, in the sys/types.h section, requires that time_t shall be an integer type (no mention if it's signed or unsigned). (clock_t could be a floating-point type however).  This is a POSIX restriction over ISO 9899.

ISO C only says that time_t is a numeric type (hence a floating point type would be possible), but POSIX is more precise indeed.

Thanks for your feedback. New proposal attached, hopefully fixing the commit message.

Quoted 3 lines

I propose to either:
* cast it to uintmax_t and use the PRIuMAX macro in the *printf function,
* cast it to unsigned long long and use %llu to print.

I see no good reason to interpret time_t as an unsigned value. Most unix-like systems (original unix, BSD, Linux, solaris...) use a signed integer. A negative time_t value ought to be meaningful, representing dates before epoch. Also I've seen people using time_t to store the difference between two timestamps, relying on a signed implementation. (I'm not saying this is the case in openvpn.)

Printing negative values as unsigned would make them meaningless/hard to diagnose. The "long long" idiom is careful about this.

I don't have much to say about the intmax_t approach, all I know is that "long long" works well as a generic solution since it doesn't need stdint.h - and format specifiers that some people find ugly.

Quoted 5 lines

While we are at it, note that tv.tv_usec, per POSIX, is a SIGNED integer type capable of representing numbers in the range [-1; 1,000,000] so casting tv_usec to unsigned long without checking if it's -1 is technically wrong, although I'd be surprised to see gettimeofday() return a negative tv_usec - but UINT_MAX or ULONG_MAX likewise.

It makes sense to respect the fact that suseconds_t can store a negative value. But then the same can be said about time_t (since its signedness is not defined by posix).

Quoted 55 lines · Matthias Andree

Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas:

Quoted 49 lines

When building openvpn-2.4.4 on OpenBSD, I noticed the following warning:

--8<--
cc -DHAVE_CONFIG_H -I. -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn -I../.. -I../../include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/include -I/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/compat -I/usr/local/include -I/usr/local/include -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\" -O2 -pipe -std=c99 -MT error.o -MD -MP -MF .deps/error.Tpo -c -o error.o /usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c
/usr/ports/pobj/openvpn-2.4.4/openvpn-2.4.4/src/openvpn/error.c:346:25: warning: format specifies type 'unsigned long' but the argument has type 'time_t' (aka 'long long') [-Wformat]

                        tv.tv_sec,
                        ^~~~~~~~~

1 warning generated.
mv -f .deps/error.Tpo .deps/error.Po
-->8--

OpenBSD uses long long for time_t on all architectures, 32 or 64 bits, in order to cope with dates beyond 2038. This is also the case on NetBSD and Linux x32.

The warning is not innocuous, as a mismatch between the format and the type of parameters passed to variadic functions can result in nasty problems (crashes, etc). For example, the code below crashes on OpenBSD/arm (32 bits long).

--8<--
#include <stdio.h>
#include <time.h>

int
main(void)

{
        time_t t;

        time(&t);
        printf("%ld %s\n", t, "foobar");
        return 0;
}

-->8--

The diff below fixes the potential issue and the warning. The method used is a cast to (long long), a method successfully used since OpenBSD switched to a 64 bits time_t. More data at

https://www.openbsd.org/papers/eurobsdcon_2013_time_t/mgp00029.html

openvpn already uses long long in a few places. Note that I did not audit the whole openvpn tree for other possible time_t problems, but I can't spot similar warnings in the build logs.
From d620431f661375d3564b60f110d1f69575ac78d7 Mon Sep 17 00:00:00 2001
From: Jeremie Courreges-Anglas <[email protected]...>
Date: Thu, 5 Oct 2017 01:43:33 +0200
Subject: [PATCH] Cast time_t to long double in order to print it.

Quoted 4 lines

The underlying type of time_t can be anything from unsigned 32 bits to signed 64 bits to float. To reliably print it, better cast it to "long long", which is at least 64 bits wide and can represent values beyond 2038.

Quoted 4 lines

NAK.

This is due to the inconsistent and misleading commit log.
The subject says "cast to long double", the code does "cast to long long".

oops, sorry about that.

Quoted 5 lines

The long commit log states time_t could be a float, which it cannot be according to IEEE Std 1003.1, 2013 edition, which, in the sys/types.h section, requires that time_t shall be an integer type (no mention if it's signed or unsigned). (clock_t could be a floating-point type however).  This is a POSIX restriction over ISO 9899.

ISO C only says that time_t is a numeric type (hence a floating point type would be possible), but POSIX is more precise indeed.

Thanks for your feedback. New proposal attached, hopefully fixing the commit message.

Quoted 3 lines

I propose to either:
* cast it to uintmax_t and use the PRIuMAX macro in the *printf function,
* cast it to unsigned long long and use %llu to print.

I see no good reason to interpret time_t as an unsigned value. Most unix-like systems (original unix, BSD, Linux, solaris...) use a signed integer. A negative time_t value ought to be meaningful, representing dates before epoch. Also I've seen people using time_t to store the difference between two timestamps, relying on a signed implementation. (I'm not saying this is the case in openvpn.)

Printing negative values as unsigned would make them meaningless/hard to diagnose. The "long long" idiom is careful about this.

I don't have much to say about the intmax_t approach, all I know is that "long long" works well as a generic solution since it doesn't need stdint.h - and format specifiers that some people find ugly.

- there are other places when a time_t is printed in openvpn. Usually it is cast to (int), which is not a nice thing to do if you plan to support dates after 2038.

I also stumbled upon this in src/openvpn/common.h

  /*
   * Printf formats for special types
   */
  #ifdef _WIN64
  #define ptr_format              "0x%I64x"
  #else
  #define ptr_format              "0x%08lx"
  #endif
  #define time_format             "%lu"
  #define fragment_header_format  "0x%08x"
  
  /* these are used to cast the arguments
   * and MUST match the formats above */
  typedef unsigned long time_type;
  #ifdef _WIN64
  typedef unsigned long long ptr_type;
  #else
  typedef unsigned long ptr_type;
  #endif

time_format and time_type are indeed used in a few places where a time_t is printed. Technically, "unsigned long" suffers from the same problem as "long" on platforms with a 32 bits "long", pushing the limit to 2106 instead of 2038.

I would suggest using "long long" here too, but do you folks think that the whole "time_format/time_type" indirection makes sense? After all, those are currently the same on all platforms.

I have a diff to fix places in src/openvpn where time_t variables are cast to (int), but it kinda depends on the approach you folks prefer to use.

Quoted 6 lines

Quoted 5 lines

While we are at it, note that tv.tv_usec, per POSIX, is a SIGNED integer type capable of representing numbers in the range [-1; 1,000,000] so casting tv_usec to unsigned long without checking if it's -1 is technically wrong, although I'd be surprised to see gettimeofday() return a negative tv_usec - but UINT_MAX or ULONG_MAX likewise.

Quoted 3 lines

It makes sense to respect the fact that suseconds_t can store a negative value. But then the same can be said about time_t (since its signedness is not defined by posix).

I guess it makes sense to also choose a generic way to print suseconds_t, whose size isn't defined by posix either, and deal with both types at once. I would suggest using %ld and a cast to (long), as this is the underlying type on at least OpenBSD and Linux.

Hi,

Sorry for being late to respond.

Quoted 31 lines · Jeremie Courreges-Anglas

- there are other places when a time_t is printed in openvpn. Usually it is cast to (int), which is not a nice thing to do if you plan to support dates after 2038.

I also stumbled upon this in src/openvpn/common.h

  /*
   * Printf formats for special types
   */
  #ifdef _WIN64
  #define ptr_format              "0x%I64x"
  #else
  #define ptr_format              "0x%08lx"
  #endif
  #define time_format             "%lu"
  #define fragment_header_format  "0x%08x"
  
  /* these are used to cast the arguments
   * and MUST match the formats above */
  typedef unsigned long time_type;
  #ifdef _WIN64
  typedef unsigned long long ptr_type;
  #else
  typedef unsigned long ptr_type;
  #endif

time_format and time_type are indeed used in a few places where a time_t is printed. Technically, "unsigned long" suffers from the same problem as "long" on platforms with a 32 bits "long", pushing the limit to 2106 instead of 2038.

Quoted 7 lines

I would suggest using "long long" here too, but do you folks think that the whole "time_format/time_type" indirection makes sense? After all, those are currently the same on all platforms.

I have a diff to fix places in src/openvpn where time_t variables are cast to (int), but it kinda depends on the approach you folks prefer to use.

Not sure about the other devs, but I think we should get rid of this indirection. Someone who forgets to cast to long long will likely also not use the time_format define. Better be consistent in casting to long long and printing using %lld (which is obviously correct for a long long). Patch is definitely welcome.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

While we are at it, note that tv.tv_usec, per POSIX, is a SIGNED integer type capable of representing numbers in the range [-1; 1,000,000] so casting tv_usec to unsigned long without checking if it's -1 is technically wrong, although I'd be surprised to see gettimeofday() return a negative tv_usec - but UINT_MAX or ULONG_MAX likewise.

It makes sense to respect the fact that suseconds_t can store a negative value. But then the same can be said about time_t (since its signedness is not defined by posix).

Quoted 4 lines

I guess it makes sense to also choose a generic way to print suseconds_t, whose size isn't defined by posix either, and deal with both types at once. I would suggest using %ld and a cast to (long), as this is the underlying type on at least OpenBSD and Linux.

Sounds good to me.

ACK, thanks. Reviewed by "staring at the code" (changes are only what it says on the tin), compiling with clang and gcc (no format related warnings), and running tests, of course :-)

I'm a bit undecided whether this should go to release/2.4 as well, as it is "refactoring" and our general policy is "refactoring goes into master" (but then it's straightforward enough). Leaving it in master for now, but I can be convinced that we want this in 2.4...

Signed-off-by Jeremie Courreges-Anglas <[email protected]...>

---
 src/openvpn/misc.c    | 4 ++--
 src/openvpn/misc.h    | 2 +-
 src/openvpn/multi.c   | 7 ++-----
 src/openvpn/options.c | 2 +-
 4 files changed, 6 insertions(+), 9 deletions(-)

src/openvpn/misc.c +2 -2

index 8c7f6116..001fe1c4 100644
@@ -553,10 +553,10 @@ setenv_int(struct env_set *es, const char *name, int value)
 }
 
 void
-setenv_unsigned(struct env_set *es, const char *name, unsigned int value)
+setenv_long_long(struct env_set *es, const char *name, long long value)
 {
     char buf[64];
-    openvpn_snprintf(buf, sizeof(buf), "%u", value);
+    openvpn_snprintf(buf, sizeof(buf), "%lld", value);
     setenv_str(es, name, buf);
 }
 

src/openvpn/misc.h +1 -1

index eb39ce3f..f6c810a2 100644
@@ -98,7 +98,7 @@ void setenv_counter(struct env_set *es, const char *name, counter_type value);
 
 void setenv_int(struct env_set *es, const char *name, int value);
 
-void setenv_unsigned(struct env_set *es, const char *name, unsigned int value);
+void setenv_long_long(struct env_set *es, const char *name, long long value);
 
 void setenv_str(struct env_set *es, const char *name, const char *value);
 

src/openvpn/multi.c +2 -5

index 4545bce1..82a0b9d9 100644
@@ -565,10 +565,7 @@ multi_client_disconnect_setenv(struct multi_context *m,
     setenv_stats(&mi->context);
 
     /* setenv connection duration */
-    {
-        const unsigned int duration = (unsigned int) now - mi->created;
-        setenv_unsigned(mi->context.c2.es, "time_duration", duration);
-    }
+    setenv_long_long(mi->context.c2.es, "time_duration", now - mi->created);
 }
 
 static void

@@ -1769,7 +1766,7 @@ multi_client_connect_setenv(struct multi_context *m,
     {
         const char *created_ascii = time_string(mi->created, 0, false, &gc);
         setenv_str(mi->context.c2.es, "time_ascii", created_ascii);
-        setenv_unsigned(mi->context.c2.es, "time_unix", (unsigned int)mi->created);
+        setenv_long_long(mi->context.c2.es, "time_unix", mi->created);
     }
 
     gc_free(&gc);

src/openvpn/options.c +0 -0

index 7aa311aa..641a26e2 100644

Here's another small diff, I forgot one suseconds_t occurrence.

From cbe8237ff59129501e1e92c8fd6a3488d94c4c0f Mon Sep 17 00:00:00 2001
From: Jeremie Courreges-Anglas <[email protected]...>
Date: Sun, 5 Nov 2017 10:03:26 +0100
Subject: [PATCH] Cast and print another suseconds_t as long

Missed in 31b5c0e

---
 src/openvpn/forward.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

src/openvpn/forward.c +1 -1

index a91a8d9a..1b7455bb 100644
@@ -649,7 +649,7 @@ check_timeout_random_component_dowork(struct context *c)
     c->c2.timeout_random_component.tv_usec = (time_t) get_random() & 0x0003FFFF;
     c->c2.timeout_random_component.tv_sec = 0;
 
-    dmsg(D_INTERVAL, "RANDOM USEC=%d", (int) c->c2.timeout_random_component.tv_usec);
+    dmsg(D_INTERVAL, "RANDOM USEC=%ld", (long) c->c2.timeout_random_component.tv_usec);
 }
 
 static inline void

Signature · 1 line

2.14.3