Hi,
On 15-10-17 14:08, Jeremie Courreges-Anglas wrote:
> On Sun, Oct 15 2017, Matthias Andree <matthias.and...@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 <j...@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
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
