From patchwork Wed Apr 13 07:41:44 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Eggert X-Patchwork-Id: 11725 Received: (qmail 28883 invoked by alias); 13 Apr 2016 07:42:03 -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 28861 invoked by uid 89); 13 Apr 2016 07:42:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=2016-04-13, hongjiu.lu@intel.com, hongjiuluintelcom, 1900 X-HELO: zimbra.cs.ucla.edu Subject: Re: [PROPOSED PATCH] Fix integer overflow when adjusting posixrules To: Florian Weimer , libc-alpha@sourceware.org References: <1460317379-31635-1-git-send-email-eggert@cs.ucla.edu> <570DD877.1010709@redhat.com> From: Paul Eggert Message-ID: <570DF838.8020303@cs.ucla.edu> Date: Wed, 13 Apr 2016 00:41:44 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <570DD877.1010709@redhat.com> Florian Weimer wrote: > On 04/10/2016 09:42 PM, Paul Eggert wrote: >> +/* intprops.h -- properties of integer types > > I'm afraid I have serious doubts about the maintainability of this file. The file has been used in core GNU utilities for a decade or so, so there is some evidence for maintainability. It can always be improved of course. > In this case, the argument for the outer cast is not within the range of > type t, so the result of the cast is implementation-defined. For ones' > complement, ~0 can be undefined, it does not have to be 0. Good catch. That code dates back to 2005, when ones' complement platforms were still arguably plausible porting targets for GNU code. Unisys stopped selling ones' complement mainframes late last year, though. ~0 should work on Unisys mainframes; but really, it's time to remove that gingerbread, if only to make the code easier to review. There's a similar issue in time/mktime.c in glibc (also shared with gnulib). Revised patches attached. The first is an updated version of the tzfile fix, and the second fixes mktime. The proposed intprops.h and mktime.c files match what's now in gnulib (which I've already fixed). From 83be322adc0d4ac94aaa46b157aff21447f0755d Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Wed, 13 Apr 2016 00:26:06 -0700 Subject: [PATCH 2/2] mktime assumes two's complement now The code attempted to verify that the current platform is two's complement, but in theory the verification would not have correctly rejected some non-two's-complement hosts, and anyway the verification effort was not worth the hassle. Reported by Florian Weimer in: https://sourceware.org/ml/libc-alpha/2016-04/msg00295.html * time/mktime.c (TYPE_TWOS_COMPLEMENT): Remove. All uses rewritten to assume two's complement, which is all we can reasonably test nowadays anyway. --- ChangeLog | 11 +++++++++++ time/mktime.c | 31 +++++++++++-------------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0a6d632..6f5efb4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,16 @@ 2016-04-13 Paul Eggert + mktime assumes two's complement now + The code attempted to verify that the current platform is two's + complement, but in theory the verification would not have + correctly rejected some non-two's-complement hosts, and anyway the + verification effort was not worth the hassle. + Reported by Florian Weimer in: + https://sourceware.org/ml/libc-alpha/2016-04/msg00295.html + * time/mktime.c (TYPE_TWOS_COMPLEMENT): + Remove. All uses rewritten to assume two's complement, which is + all we can reasonably test nowadays anyway. + Fix integer overflow when adjusting posixrules [BZ #19738] This bug can occur on hosts with 32-bit time_t when reading tz files diff --git a/time/mktime.c b/time/mktime.c index bc7ed56..46f3e06 100644 --- a/time/mktime.c +++ b/time/mktime.c @@ -103,25 +103,20 @@ verify (long_int_is_wide_enough, INT_MAX == INT_MAX * (long_int) 2 / 2); an integer. */ #define TYPE_IS_INTEGER(t) ((t) 1.5 == 1) -/* True if negative values of the signed integer type T use two's - complement, or if T is an unsigned integer type. */ -#define TYPE_TWOS_COMPLEMENT(t) ((t) ~ (t) 0 == (t) -1) - /* True if the arithmetic type T is signed. */ #define TYPE_SIGNED(t) (! ((t) 0 < (t) -1)) -/* The maximum and minimum values for the integer type T. These - macros have undefined behavior if T is signed and has padding bits. - If this is a problem for you, please let us know how to fix it for - your host. */ -#define TYPE_MINIMUM(t) \ - ((t) (! TYPE_SIGNED (t) \ - ? (t) 0 \ - : ~ TYPE_MAXIMUM (t))) -#define TYPE_MAXIMUM(t) \ - ((t) (! TYPE_SIGNED (t) \ - ? (t) -1 \ - : ((((t) 1 << (sizeof (t) * CHAR_BIT - 2)) - 1) * 2 + 1))) +/* Minimum and maximum values for integer types. + These macros have undefined behavior for signed types that either + have padding bits or do not use two's complement. If this is a + problem for you, please let us know how to fix it for your host. */ + +/* The maximum and minimum values for the integer type T. */ +#define TYPE_MINIMUM(t) ((t) ~ TYPE_MAXIMUM (t)) +#define TYPE_MAXIMUM(t) \ + ((t) (! TYPE_SIGNED (t) \ + ? (t) -1 \ + : ((((t) 1 << (sizeof (t) * CHAR_BIT - 2)) - 1) * 2 + 1))) #ifndef TIME_T_MIN # define TIME_T_MIN TYPE_MINIMUM (time_t) @@ -132,10 +127,6 @@ verify (long_int_is_wide_enough, INT_MAX == INT_MAX * (long_int) 2 / 2); #define TIME_T_MIDPOINT (SHR (TIME_T_MIN + TIME_T_MAX, 1) + 1) verify (time_t_is_integer, TYPE_IS_INTEGER (time_t)); -verify (twos_complement_arithmetic, - (TYPE_TWOS_COMPLEMENT (int) - && TYPE_TWOS_COMPLEMENT (long_int) - && TYPE_TWOS_COMPLEMENT (time_t))); #define EPOCH_YEAR 1970 #define TM_YEAR_BASE 1900 -- 2.5.5