From patchwork Tue Nov 21 05:15:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Eggert X-Patchwork-Id: 24396 Received: (qmail 103880 invoked by alias); 21 Nov 2017 05:16:07 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 103867 invoked by uid 89); 21 Nov 2017 05:16:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, KB_WAM_FROM_NAME_SINGLEWORD, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=Act, hate, Secure, 1.5 X-HELO: zimbra.cs.ucla.edu Subject: Re: Fix p_secstodate overflow handling (bug 22463) To: Joseph Myers , libc-alpha@sourceware.org References: From: Paul Eggert Message-ID: Date: Mon, 20 Nov 2017 21:15:58 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: 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 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. From fa6f9fb3fd4d9faeaf83d80c31b7d8891e9410b7 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 20 Nov 2017 20:58:44 -0800 Subject: [PATCH 2/2] [BZ #22463] * resolv/res_debug.c: Include . (libresolv_hidden_def): If the timestamp is out of time_t range, just generate an unsigned decimal integer. Use strftime instead of sprintf to format timestamps. Use __builtin_unreachable to pacify GCC. --- ChangeLog | 7 +++++++ resolv/res_debug.c | 32 +++++++++++++++++++++++--------- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/ChangeLog b/ChangeLog index eece2d8..e47525d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,12 @@ 2017-11-20 Paul Eggert + [BZ #22463] + * resolv/res_debug.c: Include . + (libresolv_hidden_def): If the timestamp is out of time_t range, + just generate an unsigned decimal integer. + Use strftime instead of sprintf to format timestamps. + Use __builtin_unreachable to pacify GCC. + New internal include file * include/intprops.h: New file, taken from Gnulib. diff --git a/resolv/res_debug.c b/resolv/res_debug.c index 4114c2d..01285b7 100644 --- a/resolv/res_debug.c +++ b/resolv/res_debug.c @@ -99,6 +99,7 @@ #include #include +#include #include #include #include @@ -1052,23 +1053,36 @@ 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 string format. + * SECS must be less than 2**32. * 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 */ - time_t clock = secs; + + /* A null-terminated string containing either YYYYMMDDHHMMSS + or an unsigned decimal integer with at most 10 digits. + See Internet RFC 4034 section 3.2. */ + static char output[sizeof "YYYYMMDDHHMMSS"]; + 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 (secs <= TYPE_MAXIMUM (time_t)) { + time_t clock = secs; + time = __gmtime_r (&clock, &timebuf); + } else if (secs <= 0xffffffff) + time = NULL; + else + __builtin_unreachable (); + + if (time == NULL) + sprintf (output, "%lu", secs); + else + strftime (output, sizeof output, "%Y%M%D%H%M%S", &timebuf); + return (output); } libresolv_hidden_def (__p_secstodate) -- 2.7.4