diff mbox

[PROPOSED] Fix integer overflow when adjusting posixrules

Message ID 570DF838.8020303@cs.ucla.edu
State New, archived
Headers show

Commit Message

Paul Eggert April 13, 2016, 7:41 a.m. UTC
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).
diff mbox

Patch

From 83be322adc0d4ac94aaa46b157aff21447f0755d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
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  <eggert@cs.ucla.edu>
 
+	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