| Message ID | 87fuaymq62.fsf@ritchie.wxcvbn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Fix time_t printing | expand |
Hi, any opinion regarding this diff?
Hi Jeremie,
On Thu, Oct 12, 2017 at 12:11:13PM +0200, Jeremie Courreges-Anglas wrote:
> any opinion regarding this diff?
Quickly skimmed over it, looked reasonable. Lack of time... (so, thanks
for the reminder).
gert
Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas: > 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 <jca@wxcvbn.org> > 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. 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. <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas:<br> </div> <blockquote type="cite" cite="mid:87fuaymq62.fsf@ritchie.wxcvbn.org"> <pre wrap=""> 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 <a class="moz-txt-link-freetext" href="https://www.openbsd.org/papers/eurobsdcon_2013_time_t/mgp00029.html">https://www.openbsd.org/papers/eurobsdcon_2013_time_t/mgp00029.html</a> 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. </pre> </blockquote> <blockquote type="cite"> <div class="moz-text-plain" wrap="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western"> <pre wrap="">From d620431f661375d3564b60f110d1f69575ac78d7 Mon Sep 17 00:00:00 2001 From: Jeremie Courreges-Anglas <a class="moz-txt-link-rfc2396E" href="mailto:jca@wxcvbn.org"><jca@wxcvbn.org></a> 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.</pre> </div> </blockquote> <br> NAK.<br> <br> This is due to the inconsistent and misleading commit log. <br> The subject says "cast to long double", the code does "cast to long long". <br> <br> 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.<br> <br> I propose to either:<br> * cast it to uintmax_t and use the PRIuMAX macro in the *printf function,<br> * cast it to unsigned long long and use %llu to print.<br> <br> 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.<br> <br> </body> </html> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Sun, Oct 15 2017, Matthias Andree <matthias.andree@gmx.de> wrote: > Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas: >> 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 <jca@wxcvbn.org> >> 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. > > 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. > 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. > 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. > 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). From 8ca610cc989601866534a46ac75b6cb30d354f96 Mon Sep 17 00:00:00 2001 From: Jeremie Courreges-Anglas <jca@wxcvbn.org> Date: Thu, 5 Oct 2017 01:43:33 +0200 Subject: [PATCH] Cast time_t to long long in order to print it. time_t is only specified as an integer type per POSIX. To reliably print it, better cast it to "long long", which is at least 64 bits wide and can represent values beyond 2038. Printing as a "long" could cause problems on ILP32 systems using a 64 bits time_t (eg OpenBSD/armv7). Signed-off-by: Jeremie Courreges-Anglas <jca@wxcvbn.org> --- src/openvpn/error.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openvpn/error.c b/src/openvpn/error.c index 04bf0da5..7b46c5ec 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -342,8 +342,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist) struct timeval tv; gettimeofday(&tv, NULL); - fprintf(fp, "%lu.%06lu %x %s%s%s%s", - tv.tv_sec, + fprintf(fp, "%lld.%06lu %x %s%s%s%s", + (long long)tv.tv_sec, (unsigned long)tv.tv_usec, flags, prefix,
On Sun, Oct 15 2017, Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote: > On Sun, Oct 15 2017, Matthias Andree <matthias.andree@gmx.de> wrote: >> Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas: >>> 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 <jca@wxcvbn.org> >>> 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. >> >> 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. > >> 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. > >> 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. Some things that I should probably have stated more clearly: - the proposed patch is fixing an actual problem. openvpn --machine-readable-output currently crashes on OpenBSD/arm (32 bits platform, 64 bits time_t) - 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. >> 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). 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 folks, any other feedback regarding this issue and the methods discussed to fix it?
Hi, On 15-10-17 14:08, Jeremie Courreges-Anglas wrote: > On Sun, Oct 15 2017, Matthias Andree <matthias.andree@gmx.de> wrote: >> Am 05.10.2017 um 01:47 schrieb Jeremie Courreges-Anglas: >>> 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 <jca@wxcvbn.org> >>> 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. >> >> 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. > >> 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. ACK to that attached patch. >> 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. Agreed that we should use a signed type to print time_t, and "long long" seems like good choice. (Too bad there is no canonical solution such as a PRItime or so.) -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, Sorry for being late to respond. On 18-10-17 20:36, Jeremie Courreges-Anglas wrote: > - 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. 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. >>> 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). > > 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. -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, Oct 26 2017, Steffan Karger <steffan.karger@fox-it.com> wrote: > Hi, > > Sorry for being late to respond. > > On 18-10-17 20:36, Jeremie Courreges-Anglas wrote: >> - 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. > > 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. ok, thanks for confirming. >>>> 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). >> >> 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. Then here's the patch. Notes: - (hopefully) all time_t values are printed with %lld/(long long). For suseconds_t, use %ld and (long). The patch is only about fixing printf-like format strings, it does not attempt to fix potential 2038-related overflows in the code. - built-checked with clang. Sometimes the code was hidden by #ifdefs, I tweaked those and added a bunch of "#error foo" to make sure that the modified code was actually built. mbedtls build also tested. - packet_id_interactive_test() doesn't build, and uses sscanf(%lu) to parse a time_t. Could fix this in another diff and convert to %lld/long long. - looks like multi.c and options.c could use a setenv_long_long() function, instead of using setenv_unsigned(). Works well in my client-side tests. From 6aa57eed8692f9790f5c6ad68087f849a6cf9633 Mon Sep 17 00:00:00 2001 From: Jeremie Courreges-Anglas <jca@wxcvbn.org> Date: Thu, 26 Oct 2017 11:40:05 +0200 Subject: [PATCH] Print time_t as long long and suseconds_t as long As per previous commit, this is a simple solution to cope with the various sizes of time_t on different archs, including those that use 64 bits time_t on ILP32 archs to cope with y2038. Also: - convert the time_type/time_format abstraction that used unsigned long to inlined long long code - print suseconds_t as a long, which appears to be the underlying type on most Unix systems around Signed-off-by: Jeremie Courreges-Anglas <jca@wxcvbn.org> --- src/openvpn/common.h | 2 -- src/openvpn/error.c | 4 ++-- src/openvpn/event.c | 10 +++++----- src/openvpn/forward.c | 2 +- src/openvpn/multi.c | 6 +++--- src/openvpn/otime.c | 18 +++++++++--------- src/openvpn/packet_id.c | 16 ++++++++-------- src/openvpn/reliable.c | 4 ++-- src/openvpn/shaper.c | 4 ++-- src/openvpn/shaper.h | 8 ++++---- src/openvpn/ssl_openssl.c | 8 ++++---- 11 files changed, 40 insertions(+), 42 deletions(-) diff --git a/src/openvpn/common.h b/src/openvpn/common.h index bb08c01f..4ea3938d 100644 --- a/src/openvpn/common.h +++ b/src/openvpn/common.h @@ -57,12 +57,10 @@ typedef int interval_t; #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 diff --git a/src/openvpn/error.c b/src/openvpn/error.c index 7b46c5ec..26455455 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -342,9 +342,9 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist) struct timeval tv; gettimeofday(&tv, NULL); - fprintf(fp, "%lld.%06lu %x %s%s%s%s", + fprintf(fp, "%lld.%06ld %x %s%s%s%s", (long long)tv.tv_sec, - (unsigned long)tv.tv_usec, + (long)tv.tv_usec, flags, prefix, prefix_sep, diff --git a/src/openvpn/event.c b/src/openvpn/event.c index d1230704..2fb112b8 100644 --- a/src/openvpn/event.c +++ b/src/openvpn/event.c @@ -1041,10 +1041,10 @@ se_wait_fast(struct event_set *es, const struct timeval *tv, struct event_set_re struct timeval tv_tmp = *tv; int stat; - dmsg(D_EVENT_WAIT, "SE_WAIT_FAST maxfd=%d tv=%d/%d", + dmsg(D_EVENT_WAIT, "SE_WAIT_FAST maxfd=%d tv=%lld/%ld", ses->maxfd, - (int)tv_tmp.tv_sec, - (int)tv_tmp.tv_usec); + (long long)tv_tmp.tv_sec, + (long)tv_tmp.tv_usec); stat = select(ses->maxfd + 1, &ses->readfds, &ses->writefds, NULL, &tv_tmp); @@ -1065,8 +1065,8 @@ se_wait_scalable(struct event_set *es, const struct timeval *tv, struct event_se fd_set write = ses->writefds; int stat; - dmsg(D_EVENT_WAIT, "SE_WAIT_SCALEABLE maxfd=%d tv=%d/%d", - ses->maxfd, (int)tv_tmp.tv_sec, (int)tv_tmp.tv_usec); + dmsg(D_EVENT_WAIT, "SE_WAIT_SCALEABLE maxfd=%d tv=%lld/%ld", + ses->maxfd, (long long)tv_tmp.tv_sec, (long)tv_tmp.tv_usec); stat = select(ses->maxfd + 1, &read, &write, NULL, &tv_tmp); diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 6cc59383..a91a8d9a 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -618,7 +618,7 @@ check_coarse_timers_dowork(struct context *c) process_coarse_timers(c); c->c2.coarse_timer_wakeup = now + c->c2.timeval.tv_sec; - dmsg(D_INTERVAL, "TIMER: coarse timer wakeup %d seconds", (int) c->c2.timeval.tv_sec); + dmsg(D_INTERVAL, "TIMER: coarse timer wakeup %lld seconds", (long long)c->c2.timeval.tv_sec); /* Is the coarse timeout NOT the earliest one? */ if (c->c2.timeval.tv_sec > save.tv_sec) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index c798c438..4545bce1 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2388,14 +2388,14 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns multi_set_pending(m, ANY_OUT(&mi->context) ? mi : NULL); #ifdef MULTI_DEBUG_EVENT_LOOP - printf("POST %s[%d] to=%d lo=%d/%d w=%d/%d\n", + printf("POST %s[%d] to=%d lo=%d/%d w=%lld/%ld\n", id(mi), (int) (mi == m->pending), mi ? mi->context.c2.to_tun.len : -1, mi ? mi->context.c2.to_link.len : -1, (mi && mi->context.c2.fragment) ? mi->context.c2.fragment->outgoing.len : -1, - (int)mi->context.c2.timeval.tv_sec, - (int)mi->context.c2.timeval.tv_usec); + (long long)mi->context.c2.timeval.tv_sec, + (long)mi->context.c2.timeval.tv_usec); #endif } diff --git a/src/openvpn/otime.c b/src/openvpn/otime.c index 3e576cc0..d741ae07 100644 --- a/src/openvpn/otime.c +++ b/src/openvpn/otime.c @@ -88,9 +88,9 @@ const char * tv_string(const struct timeval *tv, struct gc_arena *gc) { struct buffer out = alloc_buf_gc(64, gc); - buf_printf(&out, "[%d/%d]", - (int) tv->tv_sec, - (int )tv->tv_usec); + buf_printf(&out, "[%lld/%ld]", + (long long)tv->tv_sec, + (long)tv->tv_usec); return BSTR(&out); } @@ -103,7 +103,7 @@ const char * tv_string_abs(const struct timeval *tv, struct gc_arena *gc) { return time_string((time_t) tv->tv_sec, - (int) tv->tv_usec, + (long) tv->tv_usec, true, gc); } @@ -132,7 +132,7 @@ time_string(time_t t, int usec, bool show_usec, struct gc_arena *gc) if (show_usec && tv.tv_usec) { - buf_printf(&out, " us=%d", (int)tv.tv_usec); + buf_printf(&out, " us=%ld", (long)tv.tv_usec); } return BSTR(&out); @@ -198,10 +198,10 @@ time_test(void) t = time(NULL); gettimeofday(&tv, NULL); #if 1 - msg(M_INFO, "t=%u s=%u us=%u", - (unsigned int)t, - (unsigned int)tv.tv_sec, - (unsigned int)tv.tv_usec); + msg(M_INFO, "t=%lld s=%lld us=%ld", + (long long)t, + (long long)tv.tv_sec, + (long)tv.tv_usec); #endif } } diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c index a3ff5722..4e0e9868 100644 --- a/src/openvpn/packet_id.c +++ b/src/openvpn/packet_id.c @@ -608,14 +608,14 @@ packet_id_debug_print(int msglevel, } buf_printf(&out, "%c", c); } - buf_printf(&out, "] " time_format ":" packet_id_format, (time_type)p->time, (packet_id_print_type)p->id); + buf_printf(&out, "] %lld:" packet_id_format, (long long)p->time, (packet_id_print_type)p->id); if (pin) { - buf_printf(&out, " " time_format ":" packet_id_format, (time_type)pin->time, (packet_id_print_type)pin->id); + buf_printf(&out, " %lld:" packet_id_format, (long long)pin->time, (packet_id_print_type)pin->id); } - buf_printf(&out, " t=" time_format "[%d]", - (time_type)prev_now, + buf_printf(&out, " t=%lld[%d]", + (long long)prev_now, (int)(prev_now - tv.tv_sec)); buf_printf(&out, " r=[%d,%d,%d,%d,%d]", @@ -668,8 +668,8 @@ packet_id_interactive_test(void) { packet_id_reap_test(&pid.rec); test = packet_id_test(&pid.rec, &pin); - printf("packet_id_test (" time_format ", " packet_id_format ") returned %d\n", - (time_type)pin.time, + printf("packet_id_test (%lld, " packet_id_format ") returned %d\n", + (long long)pin.time, (packet_id_print_type)pin.id, test); if (test) @@ -681,8 +681,8 @@ packet_id_interactive_test(void) { long_form = (count < 20); packet_id_alloc_outgoing(&pid.send, &pin, long_form); - printf("(" time_format "(" packet_id_format "), %d)\n", - (time_type)pin.time, + printf("(%lld(" packet_id_format "), %d)\n", + (long long)pin.time, (packet_id_print_type)pin.id, long_form); if (pid.send.id == 10) diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c index 93541a9d..bfd8c247 100644 --- a/src/openvpn/reliable.c +++ b/src/openvpn/reliable.c @@ -788,14 +788,14 @@ reliable_debug_print(const struct reliable *rel, char *desc) printf("********* struct reliable %s\n", desc); printf(" initial_timeout=%d\n", (int)rel->initial_timeout); printf(" packet_id=" packet_id_format "\n", rel->packet_id); - printf(" now=" time_format "\n", now); + printf(" now=%lld\n", (long long)now); for (i = 0; i < rel->size; ++i) { const struct reliable_entry *e = &rel->array[i]; if (e->active) { printf(" %d: packet_id=" packet_id_format " len=%d", i, e->packet_id, e->buf.len); - printf(" next_try=" time_format, e->next_try); + printf(" next_try=%lld", (long long)e->next_try); printf("\n"); } } diff --git a/src/openvpn/shaper.c b/src/openvpn/shaper.c index 19dd54d0..de2da6db 100644 --- a/src/openvpn/shaper.c +++ b/src/openvpn/shaper.c @@ -76,8 +76,8 @@ shaper_soonest_event(struct timeval *tv, int delay) } } #ifdef SHAPER_DEBUG - dmsg(D_SHAPER_DEBUG, "SHAPER shaper_soonest_event sec=%d usec=%d ret=%d", - (int)tv->tv_sec, (int)tv->tv_usec, (int)ret); + dmsg(D_SHAPER_DEBUG, "SHAPER shaper_soonest_event sec=%lld usec=%ld ret=%d", + (long long)tv->tv_sec, (long)tv->tv_usec, (int)ret); #endif return ret; } diff --git a/src/openvpn/shaper.h b/src/openvpn/shaper.h index 6fac16d5..34f316fd 100644 --- a/src/openvpn/shaper.h +++ b/src/openvpn/shaper.h @@ -147,11 +147,11 @@ shaper_wrote_bytes(struct shaper *s, int nbytes) tv_add(&s->wakeup, &tv); #ifdef SHAPER_DEBUG - dmsg(D_SHAPER_DEBUG, "SHAPER shaper_wrote_bytes bytes=%d delay=%d sec=%d usec=%d", + dmsg(D_SHAPER_DEBUG, "SHAPER shaper_wrote_bytes bytes=%d delay=%ld sec=%lld usec=%ld", nbytes, - (int)tv.tv_usec, - (int)s->wakeup.tv_sec, - (int)s->wakeup.tv_usec); + (long)tv.tv_usec, + (long long)s->wakeup.tv_sec, + (long)s->wakeup.tv_usec); #endif } } diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 92a662b5..0bfb6939 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -1363,8 +1363,8 @@ bio_debug_data(const char *mode, BIO *bio, const uint8_t *buf, int len, const ch if (len > 0) { open_biofp(); - fprintf(biofp, "BIO_%s %s time=" time_format " bio=" ptr_format " len=%d data=%s\n", - mode, desc, time(NULL), (ptr_type)bio, len, format_hex(buf, len, 0, &gc)); + fprintf(biofp, "BIO_%s %s time=%lld bio=" ptr_format " len=%d data=%s\n", + mode, desc, (long long)time(NULL), (ptr_type)bio, len, format_hex(buf, len, 0, &gc)); fflush(biofp); } gc_free(&gc); @@ -1374,8 +1374,8 @@ static void bio_debug_oc(const char *mode, BIO *bio) { open_biofp(); - fprintf(biofp, "BIO %s time=" time_format " bio=" ptr_format "\n", - mode, time(NULL), (ptr_type)bio); + fprintf(biofp, "BIO %s time=%lld bio=" ptr_format "\n", + mode, (long long)time(NULL), (ptr_type)bio); fflush(biofp); }
Your patch has been applied to the master and release/2.4 branch.
commit 4ac769fb848619dcb39589af29302d8c2d698258 (master)
commit ff857aaa3bb1b496aee042a2af2830350abc5d69 (release/2.4)
Author: Jeremie Courreges-Anglas
Date: Thu Oct 5 01:43:33 2017 +0200
Cast time_t to long long in order to print it.
Signed-off-by: Jeremie Courreges-Anglas <jca@wxcvbn.org>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <87efq4havl.fsf@ritchie.wxcvbn.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15640.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
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...
Your patch has been applied to the master branch.
commit 31b5c0e9a0c10f59a0e987a9e1f82e9268b30e61 (master)
Author: Jeremie Courreges-Anglas
Date: Thu Oct 26 11:40:05 2017 +0200
Print time_t as long long and suseconds_t as long
Signed-off-by: Jeremie Courreges-Anglas <jca@wxcvbn.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <87k1zi18lt.fsf@ritchie.wxcvbn.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15667.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
On Sat, Nov 04 2017, Gert Doering <gert@greenie.muc.de> wrote: > 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 :-) Cool, thank you for your time. > 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... FWIW I'm fine with whatever approach you prefer. openvpn-2.5.0 will hopefully be released before 2038. :) Here's another small diff, I forgot one suseconds_t occurrence. From cbe8237ff59129501e1e92c8fd6a3488d94c4c0f Mon Sep 17 00:00:00 2001 From: Jeremie Courreges-Anglas <jca@wxcvbn.org> 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(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index a91a8d9a..1b7455bb 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -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
Hi, On 05-11-17 10:14, Jeremie Courreges-Anglas wrote: > Here's another small diff, I forgot one suseconds_t occurrence. > > From cbe8237ff59129501e1e92c8fd6a3488d94c4c0f Mon Sep 17 00:00:00 2001 > From: Jeremie Courreges-Anglas <jca@wxcvbn.org> > 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(-) > > diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c > index a91a8d9a..1b7455bb 100644 > --- a/src/openvpn/forward.c > +++ b/src/openvpn/forward.c > @@ -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 > -- > 2.14.3 Makes sense. 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 branch.
commit 10f12d2a61a4e2b44fa7dd4d0c32139258aa6aec
Author: Jeremie Courreges-Anglas
Date: Sun Nov 5 10:03:26 2017 +0100
Cast and print another suseconds_t as long
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <87375t6qas.fsf@ritchie.wxcvbn.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15749.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
FWIW I have added a "portability" section to the CodeStyle page, and a subsection about time_t & suseconds_t. https://community.openvpn.net/openvpn/wiki/CodeStyle#Printingtime_tandsuseconds_tvalues Feedback welcome.
From d620431f661375d3564b60f110d1f69575ac78d7 Mon Sep 17 00:00:00 2001 From: Jeremie Courreges-Anglas <jca@wxcvbn.org> 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. Printing as a "long" could cause problems on ILP32 systems using a 64 bits time_t (eg OpenBSD/armv7). Signed-off-by: Jeremie Courreges-Anglas <jca@wxcvbn.org> --- src/openvpn/error.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openvpn/error.c b/src/openvpn/error.c index 04bf0da5..7b46c5ec 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -342,8 +342,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist) struct timeval tv; gettimeofday(&tv, NULL); - fprintf(fp, "%lu.%06lu %x %s%s%s%s", - tv.tv_sec, + fprintf(fp, "%lld.%06lu %x %s%s%s%s", + (long long)tv.tv_sec, (unsigned long)tv.tv_usec, flags, prefix, -- 2.14.2
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.