Wireshark · Wireshark-dev: Re: [Wireshark-dev] FILETIME to nstime_t conversions gives a warning

[Guy Harris <guy@xxxxxxxxxxxx>]

On Oct 4, 2015, at 6:09 AM, Peter Wu <peter@xxxxxxxxxxxxx> wrote:

> I am getting these compile warnings using Clang 3.7.0:
> 
>    wsutil/nstime.c:235:25: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>        time_t l_time_min = TIME_T_MIN;
>                            ^~~~~~~~~~
>    wsutil/nstime.c:223:36: note: expanded from macro 'TIME_T_MIN'
>                        : ~ (time_t) 0 << (sizeof (time_t) * CHAR_BIT - 1)))
>                          ~~~~~~~~~~~~ ^
>    wsutil/nstime.c:236:25: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>        time_t l_time_max = TIME_T_MAX;
>                            ^~~~~~~~~~
>    wsutil/nstime.c:226:46: note: expanded from macro 'TIME_T_MAX'
>    #define TIME_T_MAX ((time_t) (~ (time_t) 0 - TIME_T_MIN))
>                                                 ^~~~~~~~~~
>    wsutil/nstime.c:223:36: note: expanded from macro 'TIME_T_MIN'
>                        : ~ (time_t) 0 << (sizeof (time_t) * CHAR_BIT - 1)))
>                          ~~~~~~~~~~~~ ^
> 
> Looking a bit further in the code, I became a bit confused on what is
> going on in nstime.c. There is a magic "fixup" constant

Not all that magic - all but 6 hours of it are explained in the comment (if anybody either 1) knows where those 6 hours came from or 2) where I miscalculated so that there are no 6 hours to explain, please feel free to submit a change to the comment to Gerrit for review).

(The code and the comment assume familiarity with 1) UN*X time stamps and 2) Windows FILENAME time stamps, where the former measure some form of "seconds-since-the-Epoch" plus fractions of a second, with the Epoch being January 1, 1970, 00:00:00 UTC, and with "some form of "seconds-since-the-Epoch"" involving different ways of dealing with leap seconds, including POSIX's "if we close our eyes, stick our fingers in our ears, and pretend they don't happen, maybe that'll work", and the latter measure 100-nanosecond-units-since-the-FILETIME-Epoch, with the Epoch being what would be January 1, 1601 in the proleptic Gregorian calendar, 00:00:00 "GMT".

The constant is just the delta between the epochs.)

> and some special handling for the largest and smallest time_t values.

Right before the offending lines is the comment:

    /* The next two lines are a fix needed for the
       broken SCO compiler. JRA. */

(where I'm guessing "JRA" is Jeremy Allison).

> The comments suggest that it is based on Samba code,

Yes - older Samba code.

> but their latest version[1] is much simpler:

Probably because they don't care about SCO's compiler any more.

I'm not sure we should care much about it, either, and can just go with something based on the current Samba code (but with the comment explaining the mixup constant, which the Samba code doesn't bother to do), as long as the new code doesn't fail in cases where the current code would succeed ("succeed" means "calculates the correct seconds value").

>    #ifndef TIME_T_MIN
>    /* we use 0 here, because (time_t)-1 means error */
>    #define TIME_T_MIN 0
>    #endif

There's nothing about (time_t)-1 that inherently means "error" rather than "December 31, 1969, 23:59:59 UTC".

>    /*
>     * we use the INT32_MAX here as on 64 bit systems,
>     * gmtime() fails with INT64_MAX
>     */
>    #ifndef TIME_T_MAX
>    #define TIME_T_MAX MIN(INT32_MAX,_TYPE_MAXIMUM(time_t))
>    #endif

If the Samba people want Bad Things to happen on 64-bit systems in 2038, they're more than welcome to do so.  I'd prefer *not* to do so in Wireshark, even at the expense of some UN*X time in the very very very far future causing problems.

So I'd go with

	#define TIME_T_MIN _TYPE_MINIMUM(time_t)

and

	#define TIME_T_MAX _TYPE_MAXIMUM(time_t)

_TIME_MINIMUM and _TIME_MAXIMUM are defined here:

	https://github.com/samba-team/samba/blob/master/lib/replace/replace.h#L638

as

	/* The extra casts work around common compiler bugs. */
	#define _TYPE_SIGNED(t) (! ((t) 0 < (t) -1))
	/* The outer cast is needed to work around a bug in Cray C 5.0.3.0.
	   It is necessary at least when t == time_t.  */
	#define _TYPE_MINIMUM(t) ((t) (_TYPE_SIGNED (t) \
	  			      ? ~ (t) 0 << (sizeof (t) * CHAR_BIT - 1) : (t) 0))
	#define _TYPE_MAXIMUM(t) ((t) (~ (t) 0 - _TYPE_MINIMUM (t)))

> and the conversion method[2]:

	...

>        t2 = t;
>        t2 += TIME_FIXUP_CONSTANT_INT;
>        t2 *= 1000*1000*10;

So this is converting *from* UN*X time *to* FILETIME.  We want to convert in the *opposite* direction, so we would want to look at nt_time_to_unix_timespec():

	https://github.com/samba-team/samba/blob/master/lib/util/time.c#L756

Our nstime_t is essentially the same as a UN*X struct timespec - seconds since the UN*X epoch plus nanoseconds since that second.

So the only change I see making would be to go with

	/* The extra casts work around common compiler bugs. */
	#define _TYPE_SIGNED(t) (! ((t) 0 < (t) -1))
	/* The outer cast is needed to work around a bug in Cray C 5.0.3.0.
	   It is necessary at least when t == time_t.  */
	#define _TYPE_MINIMUM(t) ((t) (_TYPE_SIGNED (t) \
	  			      ? ~ (t) 0 << (sizeof (t) * CHAR_BIT - 1) : (t) 0))
	#define _TYPE_MAXIMUM(t) ((t) (~ (t) 0 - _TYPE_MINIMUM (t)))

	#ifndef TIME_T_MIN
	#define TIME_T_MIN _TYPE_MINIMUM(time_t)
	#endif
	#ifndef TIME_T_MAX
	#define TIME_T_MAX _TYPE_MAXIMUM(time_t)
	#endif

as a replacement for

	#ifndef TIME_T_MIN
	#define TIME_T_MIN ((time_t) ((time_t)0 < (time_t) -1 ? (time_t) 0 \
	                    : ~ (time_t) 0 << (sizeof (time_t) * CHAR_BIT - 1)))
	#endif
	#ifndef TIME_T_MAX
	#define TIME_T_MAX ((time_t) (~ (time_t) 0 - TIME_T_MIN))
	#endif

but that doesn't rid of the offending shifts of negative values.  The only thing that would do *that* would be to refuse to convert values prior to January 1, 1970, 00:00:00 UTC by defining TIME_T_MIN as 0, and I'm not sure we want to do that.