New versions of portage cause warnings from GCC such as "warning: call to __builtin___memcpy_chk will always overflow destination buffer" to become fatal. While I appreciate the sentiment behind this change, there are several issues with it. 1) We are maintainers, not programmers. Even some of us that are programmers don't have the technical expertise to fix these errors properly. The only thing most of us can do is file a bug upstream and wait. Creating barriers such as this impedes our ability to do our jobs and is unacceptable. 2) There is a long history of false positives with overflow warnings in GCC, and every new version changes the behaviour slightly due to differences in optimization. Every new compiler version has the potential to open a new can of worms. GCC warnings should remain warnings. 3) It breaks several high profile packages such as cmake and xorg-server. 4) This change was made without any announcement to or discussion by the developer community. Anything with the potential to affect as many devs as this does needs to be at least mentioned in something other than Diego's blog. On a personal note, it took the gcc-porting team 9 months to get the tree ready for gcc 4.5. Now we have to start testing all over again. Please revert this change.
ack +1
The fatal behavior is disabled in git now: http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=2287239ef4850da216a3ea5b83f2c445c9a04e45 Maybe this is something that should be triggered by FEATURES=stricter?
That sounds like a good idea.
(In reply to comment #2) > The fatal behavior is disabled in git now: > > http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=2287239ef4850da216a3ea5b83f2c445c9a04e45 This is in 2.1.9.5 and 2.2_rc81. I'll leave this bug open until we've implemented some sort of compromise.
(In reply to comment #0) > 4) This change was made without any announcement to or discussion by the > developer community. It was on -dev: http://mid.gmane.org/201006200526.39160.vapier@gentoo.org
No, that added the warnings. I'm complaining about the later decision to call die when encountering them. I have no problem with the warnings themselves as long as they stay warnings. Portage is a package manager, not an upstream code quality improvement system. While improving upstream code is a commendable goal, forcing downstream developers to do that work by breaking their packages is not the way to go about it.
Can you point out an example of false positives? I haven't really seen any, what might have looked as false positive at a first glance turned out to be a quite nice real issue: http://blog.flameeyes.eu/2010/09/12/some-_fortify_source-far-fetched-warnings-are-funny The fact that "high profile" packages have such warnings and have been ignored does not really fare well either. Why are they ignored at all? Please do note that if you hit the "will always overflow" warning as far as I can see there _is_ a code path where the fortify-caused abort happens. Again, if you show me an actual false positive (which does not rely on the "it'll never happen because all in all the code can't reach here if [long list of conditions that might be valid but are almost impossible to certify]"), I'll be the first to say that the warnings should go the way of the dodo. By the way, nice work for "the gcc-porting team" to get the tree ready; please, do ignore the fact that out of 116 bugs blocking the tracker, 79 were reported by me. Did I make honorary member, or am I missing something regarding the role of the porting team versus the QA one?
(In reply to comment #5) > (In reply to comment #0) > > 4) This change was made without any announcement to or discussion by the > > developer community. > > It was on -dev: > http://mid.gmane.org/201006200526.39160.vapier@gentoo.org > I would not call Mike's post talking about warnings as a discussion of making them errors that will result in failure for the user.
and as a package manager, preventing source code that overflows buffers sounds like a PM issue -- we dont want users installing packages with security issues. as Diego said, it really doesnt matter what packages are failing. the only thing that matters is whether the warning is a false positive. are there any ? or are we wrongly letting people install insecure software ?
Sorry, you're right, I was thinking of -Wstrict-overflow. :/ If all these buffer overflow warnings are security issues, then why are we not immediately stabilizing every package fixed on bug #259417? I'm not being rhetorical. Seriously, is this something we should be doing? Diego: How am i ignoring your contribution? The gcc-porting team includes everyone who works on reporting, tracking, and fixing GCC porting bugs, including you. In fact, I once asked that it be made a sub-project of QA. I can make you a shiny hat if you'd like. I don't want the warnings to go away. I want them to not be sprung on users that have no way to fix them. Breaking the tree is not an acceptable form of QA. You bitch[1] and whine[2] when a new glibc version gets added to the tree that breaks some packages yet you applaud[3] this, which is no different in the eyes of a user. If this is a security issue, fine, it needs to be fixed. Let's do it in a way that doesn't piss off the community. Announce a deprecation period, help people fix their packages, punt the stragglers, profit. You'd have to have done it anyways if this version of portage was ever planned to go stable, unless you were planning on breaking that too. [1] http://blog.flameeyes.eu/2010/08/15/broken-every-other-week [2] http://blog.flameeyes.eu/2010/08/18/compounded-issues-in-glibc-2-12 [3] http://blog.flameeyes.eu/2010/09/12/some-_fortify_source-far-fetched-warnings-are-funny
I'm quite sure I made my point clear before but maybe I really have to explicit it again: I'm fine with temporary _build-time_ breaking of ~arch. That's what we do with (most) glibc updates and other issues. What I'm _not_ happy about is having crashers or security issues open at runtime: once the package has built, it should work, not crash, and definitely not overflow. Are all of those issues security? Nope. I also say so on the template I'm using to report the "will always overflow" errors. Are they all worth considering? Yes: _we have a known flow path that causes an overflow._ Maybe we can have something like "die about this if ~arch is in keywords", I think it'd be a nice compromise (FEATURES=stricter is just unusable at make.conf level).
whether the crash in question is an actual security issue or just a crash would require someone to review the changes in question. as for the build failures, i dont see this being any different from any other transitory issue -- some packages in the tree today fail. then they'll get fixed. but devs wont really be able to introduce new versions into the tree with the failures because they'll hit them on their own machine while testing. which means end users wont see new problems.
okay i'm done. whatever you do just please keep the rest of the dev community in the loop.
yes, things like this should always be announced if not discussed first on g-d however, i dont think we should be aborting (just yet) on stable. that appears to be what is happening though. a possible false positive: Bug 337031
actually, thinking about it more, even if the fortify checks are "false positives", glibc will abort at runtime making them not-quite-so-false.
Do note: no stable Portage ever aborted, so people were experiencing aborts on stable only if they used ~arch (or in the case of the linked bug, masked) portage. Remains the problem that if there are false positives in the warnings, there are definitely more in the runtime code.. so we're still trading a run-time failure for a build-time failure and vice-versa.
i dont think using newer version of portage specifically in an otherwise stable system is a bad idea. if the check is reconstituted, it should probably contain a check so it only fails on unstable systems for the time being.
I'd welcome that; would that be enough for you Ryan?
Since someone asked for an example of a false positive. This is a minimalistic reproduction of the false positive found in app-emulation/wine. This is definitely not a buffer overflow, but it a slightly obfuscated way to do the "struct hack". I will be reporting this in gcc's bugtracker as well. ------------------------- #include <string.h> #include <stdlib.h> #include <stdint.h> struct T { union { struct { char str[1]; } x; } u; }; int main() { struct T *p = malloc(sizeof(char) * 100); strcpy(p->u.x.str, "ABCD"); return 0; }
Evan, the "struct hack", or rather the "variable-sized string at the end of a struct" usage, is done, at least in C99, with a very different syntax: struct foobar { int foo; void *ptr; char bar[]; }; access to bar is, by design, access to a variable-sized array that glibc/gcc won't try to sanitise. If you give it a 1-byte size, then you're playing with fire; afaik what wine is using is the kind of hack that relies on an "undefined behaviour", but I'll leave that to upstream GCC to decide.
@Diego, I agree, but this isn't an example of something I would write. If I were writing it, I would indeed use the C99 syntax since it is a cleaner approach... But, this is an example of something found in wine, which does cause a false positive. The struct hack itself is probably formally UB according to the standard, but at the same time, it is so common that its support is expected.
The problem is that just ignoring the warning in that case won't let you work it out nicely: flame@yamato mytmpfs % gcc -O2 -Wall test4.c -o test4 In file included from /usr/include/string.h:642:0, from test4.c:1: In function ‘strcpy’, inlined from ‘main’ at test4.c:14:9: /usr/include/bits/string3.h:107:3: warning: call to __builtin___strcpy_chk will always overflow destination buffer flame@yamato mytmpfs % ./test4 *** buffer overflow detected ***: ./test4 terminated ======= Backtrace: ========= /lib/libc.so.6(__fortify_fail+0x37)[0x7f4aa7dee7a7] /lib/libc.so.6(+0xe55c0)[0x7f4aa7dec5c0] ./test4[0x4005f0] /lib/libc.so.6(__libc_start_main+0xfd)[0x7f4aa7d25cdd] ./test4[0x4004e9] ======= Memory map: ======== 00400000-00401000 r-xp 00000000 00:14 226892125 /home/flame/mytmpfs/test4 00600000-00601000 r--p 00000000 00:14 226892125 /home/flame/mytmpfs/test4 00601000-00602000 rw-p 00001000 00:14 226892125 /home/flame/mytmpfs/test4 00a6a000-00a8b000 rw-p 00000000 00:00 0 [heap] 7f4aa7af0000-7f4aa7b05000 r-xp 00000000 09:00 135220 /lib64/libgcc_s.so.1 7f4aa7b05000-7f4aa7d05000 ---p 00015000 09:00 135220 /lib64/libgcc_s.so.1 7f4aa7d05000-7f4aa7d06000 r--p 00015000 09:00 135220 /lib64/libgcc_s.so.1 7f4aa7d06000-7f4aa7d07000 rw-p 00016000 09:00 135220 /lib64/libgcc_s.so.1 7f4aa7d07000-7f4aa7e69000 r-xp 00000000 09:00 175953 /lib64/libc-2.12.1.so 7f4aa7e69000-7f4aa8069000 ---p 00162000 09:00 175953 /lib64/libc-2.12.1.so 7f4aa8069000-7f4aa806d000 r--p 00162000 09:00 175953 /lib64/libc-2.12.1.so 7f4aa806d000-7f4aa806e000 rw-p 00166000 09:00 175953 /lib64/libc-2.12.1.so 7f4aa806e000-7f4aa8073000 rw-p 00000000 00:00 0 7f4aa8073000-7f4aa8091000 r-xp 00000000 09:00 175888 /lib64/ld-2.12.1.so 7f4aa8264000-7f4aa8267000 rw-p 00000000 00:00 0 7f4aa8290000-7f4aa8291000 rw-p 00000000 00:00 0 7f4aa8291000-7f4aa8292000 r--p 0001e000 09:00 175888 /lib64/ld-2.12.1.so 7f4aa8292000-7f4aa8293000 rw-p 0001f000 09:00 175888 /lib64/ld-2.12.1.so 7f4aa8293000-7f4aa8294000 rw-p 00000000 00:00 0 7fff8ade3000-7fff8ae05000 rw-p 00000000 00:00 0 [stack] 7fff8af44000-7fff8af45000 r-xp 00000000 00:00 0 [vdso] ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall] zsh: abort (core dumped) ./test4 You either have to fix the code (and by the way, it works if you don't have the union in the middle of it!) or you have to disable fortified sources altogether, as it'll be aborting when it hits the code.
i already sent a patch to the upstream wine devel list to change it to a flexible array member. based on previous e-mails from other people in the past, they're interested in having this fixed, but they arent interested in ugly hacks. guess we'll see how it goes.
Yes, I'm convinced. As long as it's announced beforehand I don't care what happens anymore. :d
Ping. FWICS the fatal part is still commented out. Do we want to enable this, or should we close the bug?
It's already in the list of warnings to flag but let's keep it non-fatal.
