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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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