Fix p_secstodate overflow handling (bug 22463)

Joseph Myers wrote:
> If __gmtime_r returns NULL (because the year overflows the range of
> int), this will dereference a null pointer.

'clock' must be in the range 0 .. 2**32-1, so the year cannot overflow the range 
of int. This range is enforced by all callers of p_secstodate, and is required 
by Internet RFC 4034 section 3.2.

> With current GCC
> mainline, there is a compilation failure because of the possible
> buffer overrun.

That's a false alarm since the buffer cannot overrun. In Gnulib I work around 
such alarms by using the 'assume' macro defined by verify.h. Or one can use 
__builtin_unreachable () directly, as is done in the attached patch. (The 
'assume' macro is more readable, but is one more thing to add....)

> I couldn't find a specification for how this function is meant to
> behave.  This patch makes the overflow cases use <overflow> as the
> output string

The RFC says that if gmtime_r fails, one can just output the time_t value as an 
unsigned decimal integer.

Although all the GCC diagnostics for p_secstodate are false alarms, there is a 
bug here that GCC does not report: on platforms with 32-bit signed time_t, the 
code mishandles timestamps after 2038. Proposed patches attached. Although I 
haven't tested them, the first patch just adds a Gnulib include file that is 
well-tested so it should be OK.

The second patch fixes the abovementioned bug. Also, to pacify GCC it uses a 
call to __builtin_unreachable; we could simply make the static buffer larger, or 
use snprintf, but I hate surrendering to false alarms.

If you'd rather not add intprops.h to glibc, the TIME_T_MAX macros from 
elsewhere in glibc could be duped here. However, I think it's time to 
incorporate intprops.h, as glibc is running into several overflow-related 
problems for which intprops.h has well-tested solutions.

On Tue, 21 Nov 2017, Paul Eggert wrote:

> Joseph Myers wrote:
> > If __gmtime_r returns NULL (because the year overflows the range of
> > int), this will dereference a null pointer.
> 
> 'clock' must be in the range 0 .. 2**32-1, so the year cannot overflow the
> range of int. This range is enforced by all callers of p_secstodate, and is
> required by Internet RFC 4034 section 3.2.

p_secstodate is a public interface, exported from libresolv.so as a 
non-compat symbol (under the name __p_secstodate, with a #define of 
p_secstodate in the public resolv.h header).  Thus, arguments outside that 
range need to be handled in some safe way, absent a specification saying 
they are undefined, and as far as I can see they can produce buffer 
overruns at present.  RFC 4034 does not provide a specification of the 
function p_secstodate.

> That's a false alarm since the buffer cannot overrun. In Gnulib I work around
> such alarms by using the 'assume' macro defined by verify.h. Or one can use
> __builtin_unreachable () directly, as is done in the attached patch. (The
> 'assume' macro is more readable, but is one more thing to add....)

I tried using __builtin_unreachable.  One version of the patch had

+       if (time->tm_mon < 0 || time->tm_mon > 11
+           || time->tm_mday < 1 || time->tm_mday > 31
+           || time->tm_hour < 0 || time->tm_hour > 23
+           || time->tm_min < 0 || time->tm_min > 59
+           || time->tm_sec < 0 || time->tm_sec > 60)
+               __builtin_unreachable ();

but it didn't help to avoid the errors (I don't know why VRP didn't 
operate effectively in that case).

> code mishandles timestamps after 2038. Proposed patches attached. Although I
> haven't tested them, the first patch just adds a Gnulib include file that is
> well-tested so it should be OK.

It adds a gnulib include file that there have been multiple objections to 
including in glibc in the past.

On Tue, 21 Nov 2017, Paul Eggert wrote:

> If you'd rather not add intprops.h to glibc, the TIME_T_MAX macros from
> elsewhere in glibc could be duped here. However, I think it's time to

Both intprops.h and TIME_T_MAX seem massively over-engineered for the 
present bug; neither the buffer overrun, not the build failure, nor 
handling of out-of-range u_long values should need a 500-line patch.  
Since time_t is always at least as wide as long in glibc, "clock < 0" is 
entirely sufficient to detect the case of a u_long value that does not fit 
in time_t.