Bug 30477

Summary: Integer Overflow detection code optimised away, -fwrapv broken
Product: gcc Reporter: Thorsten Glaser <tg>
Component: cAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: normal CC: felix-gcc, gcc-bugs
Priority: P3 Keywords: wrong-code
Version: 3.4.6   
Target Milestone: 4.0.0   
Host: i686-pc-linux-gnu Target: i686-pc-linux-gnu
Build: i686-pc-linux-gnu Known to work: 4.1.1
Known to fail: 3.4.6 Last reconfirmed:

Description Thorsten Glaser 2007-01-15 23:42:52 UTC
Bug originally reported against gcc 4.1.1 by Felix von Leitner,
found at http://blog.fefe.de/?ts=bb5517a4 (filed as PR #30475).

This is sort of a "follow-up" bug report, but with a
different _focus_ and a different _aim_, namely the
gcc developers, especially Andrew Pinski, to provide
a patch against older gcc versions to vendors that
wish to or must continue to use them, which unbreaks
the inability of "-fwrapv" to disable gcc optimising
away code often used in security checks added to an
existing legacy code base. These patches should be
provided publicly, so that any operating system ven-
dor who uses gcc2 or gcc3 can pick them up, because
it is not MirBSD specific.


I found out that gcc 3.4.6 (MirBSD; Propolice) and both
gcc 2.95 and 3.4.3 on DragonFly BSD are vulnerable as
well, but did not want to report that because they are
heavily patched against the FSF version.

However, Andrew Pinski writes in the following comment:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30475#c2
That adding "-fwrapv" to the command line should fix this
important security issue. This, however, does not work
for me on gcc 3.4.6 (MirBSD, of course), but I've got a
shell account on a Debian GNU/Linux 4.0 box, whose sy-
stem gcc 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)
indeed suppresses this optimisation with "-fwrapv".

I then downloaded gcc-core-3.4.6.tar.gz (the pristine
source), extracted and compiled it on that Debian box.

tglaser@hephaistos:~/test $ bin/bin/gcc -v
Reading specs from /home/t/tglaser/test/bin/lib/gcc/i686-pc-linux-gnu/3.4.6/specs
Configured with: ../gcc-3.4.6/configure --prefix=/home/t/tglaser/test/bin --enable-languages=c --disable-nls --disable-shared
Thread model: posix
gcc version 3.4.6
tglaser@hephaistos:~/test $ rm -f a.out; bin/bin/gcc -O0 int.c && ./a.out
200 100
a.out: int.c:4: foo: Assertion `a+100 > a' failed.
Aborted
134|tglaser@hephaistos:~/test $ rm -f a.out; bin/bin/gcc -O1 int.c && ./a.out
200 100
-2147483549 2147483647
255|tglaser@hephaistos:~/test $ rm -f a.out; bin/bin/gcc -O1 -fwrapv int.c && ./a.out
200 100
-2147483549 2147483647
255|tglaser@hephaistos:~/test $ cat int.c
#include <assert.h>

int foo(int a) {
  assert(a+100 > a);
  printf("%d %d\n",a+100,a);
  return a;
}

int main() {
  foo(100);
  foo(0x7fffffff);
}
tglaser@hephaistos:~/test $ rm -f a.out; bin/bin/gcc -Os -fwrapv int.c && ./a.out 
200 100
-2147483549 2147483647
Comment 1 Andrew Pinski 2007-01-15 23:56:33 UTC
Fixed in 4.0.0, 3.4.x is no longer being maintained by the FSF and has not for a while now.

If you want to figure out how which patch fixed it in 4.0.0, you can do that by doing a binary regression search on the source.  There has been so many patches between 3.4.x and 4.0.x it is hard to keep track of each one.

If you don't want to move to a newer GCC, that is your issue.  Any GCC before 3.3 does not even have -fwrapv so providing all the fixes in one patch is hard because  options handling has changed a couple of time, fold-const.c changed a lot, etc.
Comment 2 Andrew Pinski 2007-01-15 23:57:39 UTC
Also why should we support older GCC when we can barrely support the current ones?
Comment 3 Thorsten Glaser 2007-01-16 02:33:57 UTC
Subject: Re:  Integer Overflow detection code optimised away,
 -fwrapv broken

Andrew Pinski (pinskia at gcc dot gnu dot org) dixit:

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

>Fixed in 4.0.0, 3.4.x is no longer being maintained by the FSF and has not for
>a while now.

>If you don't want to move to a newer GCC, that is your issue.

Interesting. So that basically means that, while you are publically
admitting that you wrote the (faulty, from a security and pragmatic
point of view) optimisation code, you are not willing to provide an
instruction to disable it just because gcc 3.4 is no longer suppor-
ted, and try to coerce us into "upgrading", while…

>Also why should we support older GCC when we can barrely support the current
>ones?

… you're publically stating, in the same PR that you can't even get
current versions of gcc to perform correctly and produce sane code?

I think this is a very poor attitude of yours. If someone adds code
doing a possibly unsafe optimisation he usually also adds a flag to
disable that certain optimisation. You didn't do this when breaking
gcc in the past. It's a shame that this was only discovered recent-
ly, especially due to security considerations. The real shame is an
attitude of "we won't fix it, either use -O0, or upgrade to current
versions of gcc, which, by the way, are poorly supported and do not
compile existing¹ programmes correctly at all"?

>         Resolution|                            |FIXED

Is this a practical joke or what? I specifically did *not* open the
bug report against gcc4. You people should know better than to tell
users to test incremental diffs of what happened between 3.4 and 4,
because I've never seen a gcc developer who didn't talk of that new
version of a "major change".

Especially you as the author of code in question could probably put
together a patch which solves the problem for gcc 3.4.6, the latest
one of the affected series; interested parties could back-port this
to other versions then.

¹) I know of at least qemu - there are probably many more. Besides,
   upgrading gcc involves upgrading g++ which is a PITA, despite it
   being an "improvement of adhering to the language specification"
   this BREAKS EXISTING CODE. Not everyone can afford this.

For what it's worth: I'm writing as developer of the MirOS Project,
but I am also a developer of FreeWRT, an embedded GNU/Linux distri-
bution kit which is used by the president of the FSF Europe, and it
*also* uses gcc 3.x on most platforms because upgrading simply *IS*
*NOT* *POSSIBLE*.

I sincerely hope some other developer reading this will have a dif-
ferent position on this topic. Otherwise, the GCC steering commitee
might have bad public relations in a not-so-far future.

Sincerely,
//mirabile
Comment 4 pinskia@gmail.com 2007-01-16 03:04:15 UTC
Subject: Re:  Integer Overflow detection code optimised away,
	-fwrapv broken

On Tue, 2007-01-16 at 02:33 +0000, tg at mirbsd dot org wrote:
> The real shame is an
> attitude of "we won't fix it, either use -O0, or upgrade to current
> versions of gcc, which, by the way, are poorly supported and do not
> compile existing¹ programmes correctly at all"? 

If you consider 4.0.x a current version of GCC, you must be joking, it
was released on "April 20, 2005" almost 2 years ago.


> I specifically did *not* open the bug report against gcc4.
Right but 3.4.x is no longer maintained.  This is like any other project
where old versions are no longer maintained.  Ask for an example Mozilla
to fix a bug in a release branch who's first release was almost three
years ago (FireFox 1.0.x for an example)?  Do you support a release
branch product for three years since you are a person who supports an
OS.  I doubt that you do.  I know of only one project who supports an
release branch for longer debian (5 years or so) and the developers of
debian actually contribute to GCC also and they are able to figure out
what patches they need to backport.  

So how long do you support a release branch of your OS?


> I know of at least qemu
Then fix qemu; x86 has not many registers so it is very easy to get into
a case where inline-asm breaks.



> > Also why should we support older GCC when we can barrely support the
> > current ones?
As for this comment, I was more talking about the bugs which are minor
or don't effect major targets or even bugs which are not even
regressions.

> Especially you as the author of code in question
I did not write this code, I just know of it.


> Besides,
>    upgrading gcc involves upgrading g++ which is a PITA, despite it
>    being an "improvement of adhering to the language specification"
>    this BREAKS EXISTING CODE. Not everyone can afford this.

And we get request from users for fixing g++ to adhere to the language
standard;  I can name a few bugs which were not filed by a GCC developer
but would break existing code (and ones which were fixed did that).
So it is a trade off, break existing code or go by the standard.  We
decided it is better to go by the standard and hope people's code is
actually standards based code.  This is the problem with C/C++ right now
is that people don't write standards based code, unlike say Ada, Fortran
(which most do or with very well known extensions) and Java (which is
not officially standardized but there is a specification).  C/C++ are
being taught as if it is not a standardized language and there is not a
specification for it and people don't use specs as a reference.



Thanks,
Andrew Pinski


Comment 5 tg@mirbsd.de 2007-01-16 03:39:50 UTC
Subject: Re:  Integer Overflow detection code optimised away,
 -fwrapv broken

pinskia at gmail dot com dixit:

>If you consider 4.0.x

I didn't say anything about 4.0, just gcc4 instead of gcc3.
And many people (e.g. most embedded systems developers or
persons with a large legacy codebase) just can't easily upgrade.

>Right but 3.4.x is no longer maintained.  This is like any other project
>where old versions are no longer maintained.  Ask for an example Mozilla

Just that, in my opinion, core technology like especially gcc
which changes over time (in contrast to binutils, where it
pretty much doesn't matter if you upgrade) should be taken
with a little bit more effort, especially if it's used by
SO many people.

>So how long do you support a release branch of your OS?

We are two persons, thus, releases are more like snapshots
that are especially stable. We do backport relevant changes
if desired by users, though.

>> Especially you as the author of code in question
>I did not write this code, I just know of it.

You did: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27257#c2

>So it is a trade off, break existing code or go by the standard.  We

I'm actually for "go by the standard", but, like I said,
core technology, legacy codebase, should deserve a little
bit more attention, especially if it's security relevant.

Hey, I mean, is -fwrapv not actually supposed to counter
this specific optimisation?

bye,
//mirabile
Comment 6 Andrew Pinski 2007-01-16 03:48:50 UTC
Subject: Re:  Integer Overflow detection code optimised away, -fwrapv broken

> Subject: Re:  Integer Overflow detection code optimised away,
>  -fwrapv broken
> >> Especially you as the author of code in question
> >I did not write this code, I just know of it.
> 
> You did: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27257#c2

Actually there are two different code, one I wrote which is does
folding of a-10 > 0 into a>10 and other code which folds a-10>a into true,
I wrote the first one.

> >So it is a trade off, break existing code or go by the standard.  We
> 
> I'm actually for "go by the standard", but, like I said,
> core technology, legacy codebase, should deserve a little
> bit more attention, especially if it's security relevant.

What is core technology?  Linux kernel does exactly the same
as GCC except there they don't do many bug fixes releases at all.
in fact  they only do major releases.  Yes you could consider 2.16.20
a minor release bug considering a lot changed from 2.16.19, like PS3
support.  Binutils on the other hand does less releases than either,
though they will do a minor (bug fix) release if needed, though sometimes
newer binutils is needed to support newer hardware, that is true of GCC
also.  Do you want to backport the SPU port to 3.4.0, I don't think so,
it depends on bug fixes and other new internal features in GCC.

-- Pinski
Comment 7 Thorsten Glaser 2007-01-16 04:08:39 UTC
Subject: Re:  Integer Overflow detection code optimised away,
 -fwrapv broken

pinskia at physics dot uc dot edu dixit:

>> >> Especially you as the author of code in question
>> >I did not write this code, I just know of it.
>> 
>> You did: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=27257#c2
>
>Actually there are two different code, one I wrote which is does
>folding of a-10 > 0 into a>10 and other code which folds a-10>a into true,
>I wrote the first one.

Okay. Then, maybe, the person who wrote the other one
will stand up and speak.

[ not commenting on this Linux stuff, I don't use Linux ]

bye,
//mirabile
Comment 8 Richard Biener 2007-01-16 10:36:26 UTC
If you rely on support and maintainance for gcc releases that have been discontinued by the FSF you need to get to your system vendor providing the old gcc or to an external contractor.
Comment 9 Thorsten Glaser 2007-01-16 16:56:47 UTC
Subject: Re:  Integer Overflow detection code optimised away,
 -fwrapv broken

rguenth at gcc dot gnu dot org dixit:

>If you rely on support and maintainance for gcc releases that have been
>discontinued by the FSF you need to get to your system vendor providing the old
>gcc or to an external contractor.

Look, I'm the founder and head developer of a free BSD operating sy-
stem with a mere two active developers, _and_ a core developer of an
embedded GNU/Linux operating system with much less than a dozen devs
so I cannot either do it myself or gather funds to do this.

Neither can we switch to gcc4 simply because it breaks too much exi-
sting codebase. Furthermore, in FreeWRT, the Linux 2.4 kernel, which
MUST be used for certain targets (also Linux 2.6 is said to be bloa-
ted and thusly unusable due to its sheer size, which I can only cite
because I don't do Linux myself), also cannot be compiled with gcc4.
True, that's the fault of the Linux developers, but GCC is core tech
in widespread use, and as such, you should weigh better between cost
and result of your actions.

bye,
//mirabile
Comment 10 Richard Biener 2007-01-16 17:18:44 UTC
We do weight between cost and result which is a reason we keep branches in active maintainance for a long time.  But we need to focus on where the majority of our users are, which is gcc 4.1 nowadays.  We don't have ressources to maintain older releases for an infinite time.
Comment 11 Thorsten Glaser 2007-01-16 17:34:11 UTC
Subject: Re:  Integer Overflow detection code optimised away,
 -fwrapv broken

rguenth at gcc dot gnu dot org dixit:

>But we need to focus on where the majority of our users are, which is
>gcc 4.1 nowadays.

I highly doubt that, unless "your users" is only applying
to users of the GNU OS (i.e. GNU/HURD) ;-)

Even then, you _might_ _want_ to fix a critical security
problem in an older version _even_ if it's discontinued.
Or at least supply a patch to interested parties.

bye,
//mirabile
Comment 12 Thorsten Glaser 2007-01-16 17:49:11 UTC
Reopening this bug because http://gcc.gnu.org/ml/gcc/2006-12/msg00749.html
states that:
"For example, GCC itself assumes wrapv semantics internally,"

This implies that gcc2 and gcc3 cannot compile gcc correctly,
unless using -O0. Adding -fwrapv to autoconf's defaults won't
help gcc2 and gcc3 users either, without a fix for this PR.
Comment 13 Andrew Pinski 2007-01-16 18:00:20 UTC
(In reply to comment #12)
> Reopening this bug because http://gcc.gnu.org/ml/gcc/2006-12/msg00749.html
> states that:
> "For example, GCC itself assumes wrapv semantics internally,"

And those places are getting fixed.  Again this specific bug was fixed in 4.0.0.
Comment 14 gdr@cs.tamu.edu 2007-01-16 18:01:35 UTC
Subject: Re:  Integer Overflow detection code optimised away, -fwrapv broken

"rguenth at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:

| We do weight between cost and result which is a reason we keep branches in
| active maintainance for a long time.  But we need to focus on where the
| majority of our users are, which is gcc 4.1 nowadays.

Fully agreed.

We must however do something about the integer overflow thingy,
because we stillface the needs of our user base. 

-- Gaby
Comment 15 tg@mirbsd.de 2007-01-22 23:54:17 UTC
Subject: Re:  Integer Overflow detection code optimised away,
 -fwrapv broken

pinskia at gcc dot gnu dot org dixit:

>fold-const.c changed a lot, etc.

>Actually there are two different code, one I wrote which is does
>folding of a-10 > 0 into a>10 and other code which folds a-10>a into true,
>I wrote the first one.

I found the second one in CVSweb, and it's not the cause for
this unsafe "optimisation". I even changed fold-const.c to
have some wrapper around fold() which debug_tree()s me the
input and output, and the '100' stays in (at -O1, which does
exhibit the faulty behaviour already):

[…] arg 1 <integer_cst 0xa7d85b7c constant 100>> arg 1 <parm_decl 0xa7d1cec4 a>>

Now I don't know any gcc internals, but I suppose this isn't
done in fold-const.c… thanks to fprintf, my beloved debugger ;)

bye,
//mirabile
--
  "Using Lynx is like wearing a really good pair of shades: cuts out
   the glare and harmful UV (ultra-vanity), and you feel so-o-o COOL."
                                         -- Henry Nelson, March 1999
Comment 16 Thorsten Glaser 2007-01-25 14:28:55 UTC
Interestingly enough, nbd of OpenWrt has found that the bug
doesn't appear (i.e. the assert is triggered) if the function
is inlined (at -O3, with -finline-functions, or the attribute).

I've used a simpler test programme while trying to look at
it myself:
tglaser@hephaistos:~ $ cat x.i
int foo(int a) { return (((a + 100) > a) ? 0 : 1); }
int main(void) { return (foo(0x7fffffff)); }

If that one returns 0, bug, if 1, correct.

I suppose this is not a problem in fold-const.c but in some
other code "optimisation".

We'd really appreciate if someone can help with this.
Comment 17 Richard Biener 2007-01-25 14:49:48 UTC
Backporting the fix for PR28651 should fix it I guess.
Comment 18 Thorsten Glaser 2007-01-25 16:09:14 UTC
Subject:  Integer Overflow detection code optimised away, -fwrapv
 broken

Dixi:

>Commit ID:	10045B8CAF141886704
>CVSROOT:	/cvs
>Module name:	gcc
>Changes by:	tg@herc.mirbsd.org	2007/01/25 15:21:11 UTC
>
>Modified files:
>	gcc            : simplify-rtx.c
>
>Log message:
>   ------- Comment [100]#17 From [101]Richard Guenther 2007-01-25 14:49 -------
>Backporting the fix for [102]PR28651 should fix it I guess.

Yes it does, thanks.

>To generate a diff of this changeset, execute the following commands:
>cvs -R rdiff -ur1.5 -r1.6 gcc/gcc/simplify-rtx.c

That is:

----- cutting here may damage your screen surface -----
Index: gcc/gcc/simplify-rtx.c
diff -u gcc/gcc/simplify-rtx.c:1.5 gcc/gcc/simplify-rtx.c:1.6
--- gcc/gcc/simplify-rtx.c:1.5	Thu Mar 30 19:50:29 2006
+++ gcc/gcc/simplify-rtx.c	Thu Jan 25 15:21:10 2007
@@ -2686,18 +2686,18 @@
      a register or a CONST_INT, this can't help; testing for these cases will
      prevent infinite recursion here and speed things up.
 
-     If CODE is an unsigned comparison, then we can never do this optimization,
-     because it gives an incorrect result if the subtraction wraps around zero.
-     ANSI C defines unsigned operations such that they never overflow, and
-     thus such cases can not be ignored.  */
+     We can only do this for EQ and NE comparisons as otherwise we may
+     lose or introduce overflow which we cannot disregard as undefined as
+     we do not know the signedness of the operation on either the left or
+     the right hand side of the comparison.  */
 
   if (INTEGRAL_MODE_P (mode) && trueop1 != const0_rtx
+      && (code == EQ || code == NE)
       && ! ((GET_CODE (op0) == REG || GET_CODE (trueop0) == CONST_INT)
 	    && (GET_CODE (op1) == REG || GET_CODE (trueop1) == CONST_INT))
       && 0 != (tem = simplify_binary_operation (MINUS, mode, op0, op1))
-      /* We cannot do this for == or != if tem is a nonzero address.  */
-      && ((code != EQ && code != NE) || ! nonzero_address_p (tem))
-      && code != GTU && code != GEU && code != LTU && code != LEU)
+      /* We cannot do this if tem is a nonzero address.  */
+      && ! nonzero_address_p (tem))
     return simplify_relational_operation (signed_condition (code),
 					  mode, tem, const0_rtx);
 
----- cutting here may damage your screen surface -----

This applies to gcc 3.4.6 - if you need other versions, YMMV.

bye,
//mirabile
Comment 19 Jackie Rosen 2014-02-16 13:16:47 UTC Comment hidden (spam)