Fix p_secstodate overflow handling (bug 22463)
Commit Message
On 11/21/2017 05:33 AM, Joseph Myers wrote:
> RFC 4034 does not provide a specification of the
> function p_secstodate.
Although RFC 4034 does not specify the p_secstodate API, it does specify
the string format and p_secstodate is supposed to generate that format.
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 going past the year 2106 or about tm_year
overflow or anything like that. We can also fix another bug, since the
code is mishandling post-2106 timestamps now on 64-bit time_t platforms
(just as it is mishandling post-2038 timestamps on 32-bit time_t platforms).
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.
Comments
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(-)
@@ -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
@@ -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