[Peter Wu <peter@xxxxxxxxxxxxx>]

Hi Guy,

Thank you for your detailed reply.

On Mon, Oct 05, 2015 at 08:11:55AM -0700, Guy Harris wrote:
> 
> 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.

Probably because SCO compilers are fixed by now. This page suggests GCC
for opensource projects, I think we can remove the workaround too:
http://osr507doc.sco.com/en/DevSysoview/Ccompil.html

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

Right, the "error" refers to the return value of mktime().

> >    /*
> >     * 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.

See the comment on gmtime(), they use that, but we do not. So we do not
need this restriction.

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

If the validation rejects times, then:

 - netmon and peektagged would reject capture files with an invalid
   timestamp.
 - packet-windows-common will display "Time can't be converted" instead
   of a human-readable time.

It is not a disaster as you will unlikely find such dates, but let's try
to be more forgivable.

Your proposed change is almost the same as the previous one, except the
compile warning (error with default -Werror in cmake). Let's try this:
https://code.wireshark.org/review/10862
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl