[PATCH 1/2] maint: accommodate gcc's -Wstrict-overflow option

[Jim Meyering]

There's a nasty bug in gcc, mentioned in log and comments below:
(It converts a loop that would obviously terminate into one
that never does, just because a signed accumulator may overflow.
The crazy part is that the offending accumulator is not related
to the loop termination condition. )

    http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33498

Once you read that and understand the implications, you'll
realize why I (the first to reject -Wstrict-overflow as not
worth accommodating) have decided that finally it is better
to use that option than to risk being bitten by the gcc bug.

-Wstrict-overflow does detect vulnerable code like the
example in the bug report.

Accommodating it required changes in 4 programs and tweaks
to configure.ac so that --enable-gcc-warnings now enables
that option in src/, but not in lib/ or in gnulib-tests/.
There would be 3 warnings in lib/ -- for two of them I
have patches.

>From 53e35f67a09b197430d7a70a7a7639d05cb1b80a Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 25 May 2011 12:29:18 +0200
Subject: [PATCH 1/2] maint: accommodate gcc's -Wstrict-overflow option

* src/factor.c (factor_using_pollard_rho): Change type of "i"
to unsigned to avoid warning from gcc's -Wstrict-overflow.
* src/expr.c: Use an unsigned intermediate.
* src/dircolors.c (main): Reorder operations to avoid the risk of
pointer overflow.
* src/tr.c (squeeze_filter): Change NOT_A_CHAR from an anonymous
"enum" to an "int", to avoid this warning:
tr.c:1624:10: error: assuming signed overflow does not occur when
  simplifying conditional to constant [-Werror=strict-overflow]
---
 src/dircolors.c |    2 +-
 src/expr.c      |    8 +++++---
 src/factor.c    |    3 ++-
 src/tr.c        |    2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/dircolors.c b/src/dircolors.c
index d1962ea..9aaa87f 100644
--- a/src/dircolors.c
+++ b/src/dircolors.c
@@ -456,7 +456,7 @@ to select a shell syntax are mutually exclusive"));
   if (print_database)
     {
       char const *p = G_line;
-      while (p < G_line + sizeof G_line)
+      while (p - G_line < sizeof G_line)
         {
           puts (p);
           p += strlen (p) + 1;
diff --git a/src/expr.c b/src/expr.c
index ce65ecf..2331f64 100644
--- a/src/expr.c
+++ b/src/expr.c
@@ -312,15 +312,17 @@ main (int argc, char **argv)

   parse_long_options (argc, argv, PROGRAM_NAME, PACKAGE_NAME, VERSION,
                       usage, AUTHORS, (char const *) NULL);
+
   /* The above handles --help and --version.
      Since there is no other invocation of getopt, handle `--' here.  */
-  if (argc > 1 && STREQ (argv[1], "--"))
+  unsigned int u_argc = argc;
+  if (1 < u_argc && STREQ (argv[1], "--"))
     {
-      --argc;
+      --u_argc;
       ++argv;
     }

-  if (argc <= 1)
+  if (u_argc <= 1)
     {
       error (0, 0, _("missing operand"));
       usage (EXPR_INVALID);
diff --git a/src/factor.c b/src/factor.c
index 3354a4d..2784320 100644
--- a/src/factor.c
+++ b/src/factor.c
@@ -160,7 +160,7 @@ factor_using_pollard_rho (mpz_t n, int a_int)
   mpz_t a;
   mpz_t g;
   mpz_t t1, t2;
-  int k, l, c, i;
+  int k, l, c;

   debug ("[pollard-rho (%d)] ", a_int);

@@ -204,6 +204,7 @@ S2:
       mpz_set (x1, x);
       k = l;
       l = 2 * l;
+      unsigned int i;
       for (i = 0; i < k; i++)
         {
           mpz_mul (x, x, x); mpz_add (x, x, a); mpz_mod (x, x, n);
diff --git a/src/tr.c b/src/tr.c
index 2a729c6..f7593d3 100644
--- a/src/tr.c
+++ b/src/tr.c
@@ -1555,7 +1555,7 @@ squeeze_filter (char *buf, size_t size, size_t (*reader) 
(char *, size_t))
 {
   /* A value distinct from any character that may have been stored in a
      buffer as the result of a block-read in the function squeeze_filter.  */
-  enum { NOT_A_CHAR = CHAR_MAX + 1 };
+  const int NOT_A_CHAR = INT_MAX;

   int char_to_squeeze = NOT_A_CHAR;
   size_t i = 0;
--
1.7.5.2.660.g9f46c


>From ae99bc21fba6d4267a8f73d69afa1396971c57b4 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 25 May 2011 14:34:13 +0200
Subject: [PATCH 2/2] build: --enable-gcc-warnings: enable -Wstrict-overflow
 in src/

* configure.ac (WARN_CFLAGS): Don't turn off -Wstrict-overflow.
(GNULIB_WARN_CFLAGS): Remove -Wstrict-overflow from the list of
warning options used in lib/.
Normally I find that -Wstrict-overflow produces too many false
positives, but considering that it warns of the bug reported in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33498, I now think
it is worthwhile.  The lesser of two evils.
---
 configure.ac |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index c8bd9e3..3dbce5d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -88,8 +88,11 @@ if test "$gl_gcc_warnings" = yes; then
   nw="$nw -Wmissing-format-attribute" # copy.c
   nw="$nw -Wunsafe-loop-optimizations" # a few src/*.c
   nw="$nw -Winline"                 # system.h's 
readdir_ignoring_dot_and_dotdot
-  nw="$nw -Wstrict-overflow"        # expr.c, pr.c, tr.c, factor.c
-  # ?? -Wstrict-overflow
+
+  # Using -Wstrict-overflow is a pain, but the alternative is worse.
+  # For an example, see the code that provoked this report:
+  # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33498
+  # Code like that still infloops with gcc-4.6.0 and -O2.  Scary indeed.

   gl_MANYWARN_ALL_GCC([ws])
   gl_MANYWARN_COMPLEMENT([ws], [$ws], [$nw])
@@ -116,6 +119,7 @@ if test "$gl_gcc_warnings" = yes; then
   # We use a slightly smaller set of warning options for lib/.
   # Remove the following and save the result in GNULIB_WARN_CFLAGS.
   nw=
+  nw="$nw -Wstrict-overflow"
   nw="$nw -Wuninitialized"
   nw="$nw -Wunused-macros"
   nw="$nw -Wmissing-prototypes"
--
1.7.5.2.660.g9f46c