| Message ID | 0b913772-b3c4-f73f-0812-92fbba0577d0@cs.ucla.edu |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 21 Nov 2017, Paul Eggert wrote: > Attached is an (untested) patch with this simplification in mind. It uses your > idea of a 'clock < 0' test along with a static assertion (though the assertion > is merely that time_t is at least 32 bits wide, since that's all we need now) > to avoid the need for complicated integer-overflow checking. It uses my idea > to use strftime instead of sprintf to simplify and shorten the code and avoid > having confusing gotos and pragmas merely to pacify GCC's false alarms. And it > passes just an unsigned int to sprintf so that GCC doesn't spout off > unnecessarily about format overflow. The strftime use is incorrect - the %M%D needs to be %m%d. This illustrates the need for including testcases, as in my second patch.
On Tue, 21 Nov 2017, Paul Eggert wrote: > I reread the RFC, and found that our patches both mishandled out-of-range > timestamps. The RFC says that, for a timestamp greater than the 32-bit range, > the string should be a decimal representation of the timestamp's low-order 32 > bits. With this in mind we can simplify the fix since we need not worry about Where does it say that? My reading is more that (a) the binary representation is the low-order 32 bits; (b) a decimal representation of the low-order 32-bits is valid as a textual representation, but (c) the timestamp is to be interpreted as whatever time with that low-order 32-bits is within 68 years of the present, and so a YYYYMMDDHHmmSS representation corresponds to that time.
On Tue, 21 Nov 2017, Joseph Myers wrote: > On Tue, 21 Nov 2017, Paul Eggert wrote: > > > I reread the RFC, and found that our patches both mishandled out-of-range > > timestamps. The RFC says that, for a timestamp greater than the 32-bit range, > > the string should be a decimal representation of the timestamp's low-order 32 > > bits. With this in mind we can simplify the fix since we need not worry about > > Where does it say that? My reading is more that (a) the binary > representation is the low-order 32 bits; (b) a decimal representation of > the low-order 32-bits is valid as a textual representation, but (c) the > timestamp is to be interpreted as whatever time with that low-order > 32-bits is within 68 years of the present, and so a YYYYMMDDHHmmSS > representation corresponds to that time. And, incidentally, I don't see how any of this can say anything about what the API should do for values outside the 32-bit range, since the RFC is purely concerned with interpretation and representation of values *within* the range, as those are all that can be transmitted in the DNS protocol. So if a time value outside the range is passed, it seems to make logical sense to produce the most human-readable representation, i.e. YYYYMMDDHHmmSS when possible (while recognizing that this API is problematic post-2038, as then the value 0 on the network should no longer be interpreted as in the year 1970).
On 11/21/2017 02:38 PM, Joseph Myers wrote: > Where does it say that? It's in RFC 4034 section 3.1.5, which says that the time values specify a "date and time in the form of a 32-bit unsigned number of seconds elapsed since 1 January 1970 00:00:00 UTC, ignoring leap seconds." Although section 3.2 says how these time values are represented, it does not give license to represent a date and time that is out of the 32-bit range. Section 3.1.5 goes on to say that the intent is to use serial number arithmetic a la RFC 1982, which boils down to using the low-order 32-bits of the full timestamp. > the RFC is > purely concerned with interpretation and representation of values*within* > the range By referring to RFC 1982, RFC 4034 is clearly concerned with how to deal with timestamp values outside the 32-bit window. I'm still thinking that it would simplify things (and avoid similar confusion in the future) if p_secstodate always used sprintf ("%u", ...) for these timestamps.
On Tue, 21 Nov 2017, Paul Eggert wrote: > On 11/21/2017 02:38 PM, Joseph Myers wrote: > > Where does it say that? > > It's in RFC 4034 section 3.1.5, which says that the time values specify a > "date and time in the form of a 32-bit unsigned number of seconds elapsed > since 1 January 1970 00:00:00 UTC, ignoring leap seconds." Although section That section is about the wire format (and clearly implies that *in the wire format* the low 32 bits are used). > 3.2 says how these time values are represented, it does not give license to > represent a date and time that is out of the 32-bit range. Section 3.1.5 goes > on to say that the intent is to use serial number arithmetic a la RFC 1982, > which boils down to using the low-order 32-bits of the full timestamp. My reading of section 3.2 together with section 3.1.5 would be that *when giving a textual representation of a value from the wire format*, it's either YYYYMMDDHHmmSS for the *actual* time (within 2**31 seconds of the present time), as long as that involves a year from 0001 to 9999, or the decimal value of the 32 bits from the wire. It says nothing about what the textual representation should be for an API that is given extra information beyond the 32 bits on the wire, such as p_secstodate on a 64-bit system. > I'm still thinking that it would simplify things (and avoid similar confusion > in the future) if p_secstodate always used sprintf ("%u", ...) for these > timestamps. I think that would be contrary to the point of this API for use in debugging, which means it should give a more human-readable output when possible.
On 11/21/2017 03:35 PM, Joseph Myers wrote: > My reading of section 3.2 together with section 3.1.5 would be that *when > giving a textual representation of a value from the wire format*, it's > either YYYYMMDDHHmmSS for the*actual* time (within 2**31 seconds of the > present time), as long as that involves a year from 0001 to 9999, or the > decimal value of the 32 bits from the wire. Sorry, I'm not quite following. Surely you don't mean that p_secstodate should have time-dependent behavior, i.e., that it should call clock_gettime or equivalent in order to determine whether the requested timestamp is in the 68-year window around the current time, so that it can generate the YYYYMMDDHHMMSS format. > It says nothing about what the textual representation should be for an API > that is given extra information beyond the 32 bits on the wire, such as > p_secstodate on a 64-bit system. Yes, we're agreed that the RFC does not specify an API. However, the API is intended for use in generating strings that follow the RFC-specified format, and that's all that the API has to do. I see little point to bikeshedding this API to try to support values outside the 32-bit range; doing that is more likely to cause trouble (because it will generate either nonconforming or numerically-incorrect strings) than to cure it.
On Tue, 21 Nov 2017, Paul Eggert wrote: > On 11/21/2017 03:35 PM, Joseph Myers wrote: > > My reading of section 3.2 together with section 3.1.5 would be that *when > > giving a textual representation of a value from the wire format*, it's > > either YYYYMMDDHHmmSS for the*actual* time (within 2**31 seconds of the > > present time), as long as that involves a year from 0001 to 9999, or the > > decimal value of the 32 bits from the wire. > > Sorry, I'm not quite following. Surely you don't mean that p_secstodate should > have time-dependent behavior, i.e., that it should call clock_gettime or No. My suggestion would be that we should say if anything it's the caller's responsibility to implement any such time-dependence and to call p_secstodate with a complete time value rather than just the low 32 bits. And that we should probably obsolete this interface before 2038, which is when such time-dependence might start to matter for the uses in glibc. (Any replacement would then explicitly take a full 64-bit time value on all platforms, possibly with a separate interface that implements the logic to convert a wire-format 32-bit value into a full 64-bit time value. But the replacement doesn't need designing now.) (This code should of course move to using __time64_t and __gmtime64_r or similar once such internal interfaces exist that are explicitly 64-bit on 32-bit platforms - generically, internal code in glibc using time_t etc. should move to explicitly 64-bit interfaces once available so that it continues to work properly on 32-bit platforms past 2038.)
Joseph Myers wrote:
> we should probably obsolete this interface before 2038
This is the best idea I've heard so far. If we do it, it won't really matter
what p_secstodate does with post-2038 (or especially post-2106) timestamps, so
long as it doesn't do something stupid like overrun the array.
On 11/22/2017 12:00 AM, Paul Eggert wrote: > On 11/21/2017 02:38 PM, Joseph Myers wrote: >> Where does it say that? > > It's in RFC 4034 section 3.1.5, which says that the time values specify > a "date and time in the form of a 32-bit unsigned number of seconds > elapsed since 1 January 1970 00:00:00 UTC, ignoring leap seconds." > Although section 3.2 says how these time values are represented, it does > not give license to represent a date and time that is out of the 32-bit > range. Section 3.1.5 goes on to say that the intent is to use serial > number arithmetic a la RFC 1982, which boils down to using the low-order > 32-bits of the full timestamp. p_secstodate was added with an import from BIND in 1996: Tue Aug 13 18:26:45 1996 Ulrich Drepper <drepper@cygnus.com> … (__dn_count_labels, __p_secstodate): New functions. At this time, the DNSSEC RFCs did not require modulo interpretation of the signer field. It was truly restricted to 32 bits, as far as I can see: <https://tools.ietf.org/html/rfc2065> I think we can change the function to always return NULL (or abort the process with an error message) and turn it into into a compat symbol. I very much doubt that there are any users at all because the function does not implement the current DNSSEC specification. (Anything DNSSEC-related still left in libresolv is equally useless because the protocol underwent several incompatible revisions.) Thanks, Florian
On Wed, 22 Nov 2017, Florian Weimer wrote: > I think we can change the function to always return NULL (or abort the process > with an error message) and turn it into into a compat symbol. I very much > doubt that there are any users at all because the function does not implement Well, there are two calls in ns_print.c, so you can't eliminate the function without doing something with those calls.
On 11/22/2017 02:06 PM, Joseph Myers wrote: > On Wed, 22 Nov 2017, Florian Weimer wrote: > >> I think we can change the function to always return NULL (or abort the process >> with an error message) and turn it into into a compat symbol. I very much >> doubt that there are any users at all because the function does not implement > > Well, there are two calls in ns_print.c, so you can't eliminate the > function without doing something with those calls. Oh, right, but it has got these gems: /* KJD - need to complete this */ /* XXX need to dump key, print otherdata length & other data */ So maybe we should just fix the generic/default case to use unknown RR type syntax and just dump those records in that format. I'll send patches for that once my current round of docs/syscall wrapper work is done (too many patches in flight right now). I don't think DNSSEC as a protocol (or TKEY, which is not part of DNSSEC) will survive contact with the year 2038 without changes, so doing any proactive Y2038 work here isn't worthwhile IMHO. Thanks, Florian
On Wed, 22 Nov 2017, Florian Weimer wrote: > So maybe we should just fix the generic/default case to use unknown RR type > syntax and just dump those records in that format. I'll send patches for that > once my current round of docs/syscall wrapper work is done (too many patches > in flight right now). We still need to fix the build of p_secstodate so that regressions with mainline GCC / binutils can be reliably detected again, which is not currently the case and is meaning that new build failures keep appearing while old ones such as this one remain unfixed.
From c6e58222ed279191add7ec97d98b33ed6eb08a17 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Tue, 21 Nov 2017 13:01:14 -0800 Subject: [PATCH] [BZ #22463] * resolv/res_debug.c (p_secstodate): If the timestamp is out of time_t or 32-bit unsigned range, just generate an unsigned decimal integer representing the low-order 32 bits, as required by RFC 4034. Use strftime instead of sprintf to format timestamps, as this is simpler and avoids the need to pacify GCC about overflows that cannot occur. --- ChangeLog | 10 ++++++++++ resolv/res_debug.c | 19 ++++++++++--------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/ChangeLog b/ChangeLog index c72be0c301..5bf75fd3c2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2017-11-21 Paul Eggert <eggert@cs.ucla.edu> + + [BZ #22463] + * resolv/res_debug.c (p_secstodate): If the timestamp is out of + time_t or 32-bit unsigned range, just generate an unsigned decimal + integer representing the low-order 32 bits, as required by RFC + 4034. Use strftime instead of sprintf to format timestamps, as + this is simpler and avoids the need to pacify GCC about overflows + that cannot occur. + 2017-11-21 Adhemerval Zanella <adhemerval.zanella@linaro.org> * nptl/pthreadP.h (ASSERT_PTHREAD_INTERNAL_SIZE): Add workarond for diff --git a/resolv/res_debug.c b/resolv/res_debug.c index 4114c2db46..93fcded381 100644 --- a/resolv/res_debug.c +++ b/resolv/res_debug.c @@ -1052,23 +1052,24 @@ libresolv_hidden_def (__dn_count_labels) /* - * Make dates expressed in seconds-since-Jan-1-1970 easy to read. + * Convert SECS, a count of seconds since 1970-01-01, to the string + * format expected by Internet RFC 4034. * SIG records are required to be printed like this, by the Secure DNS RFC. */ char * p_secstodate (u_long secs) { /* XXX nonreentrant */ static char output[15]; /* YYYYMMDDHHMMSS and null */ + _Static_assert ((time_t) 0x80000000 != 0, + "time_t contains at least 32 bits"); + unsigned int losecs = secs & 0xffffffff; time_t clock = secs; - struct tm *time; - struct tm timebuf; - time = __gmtime_r(&clock, &timebuf); - time->tm_year += 1900; - time->tm_mon += 1; - sprintf(output, "%04d%02d%02d%02d%02d%02d", - time->tm_year, time->tm_mon, time->tm_mday, - time->tm_hour, time->tm_min, time->tm_sec); + if (losecs != secs || clock < 0 + || __gmtime_r (&clock, &timebuf) == NULL) + sprintf (output, "%u", losecs); + else + strftime (output, sizeof output, "%Y%M%D%H%M%S", &timebuf); return (output); } libresolv_hidden_def (__p_secstodate) -- 2.14.3