| Summary: | net-libs/cyassl-2.9.4 injects CFLAGS: -g -O0 -funroll-loops -fomit-frame-pointer -fwrapv and maybe many more | ||
|---|---|---|---|
| Product: | Gentoo Linux | Reporter: | Jeroen Roovers (RETIRED) <jer> |
| Component: | [OLD] Library | Assignee: | Anthony Basile <blueness> |
| Status: | RESOLVED WONTFIX | ||
| Severity: | normal | CC: | qa, zerochaos |
| Priority: | Normal | Keywords: | PATCH |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| URL: | https://github.com/cyassl/cyassl/issues/85 | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | |||
| Bug Blocks: | 495848 | ||
| Attachments: |
build.log after ebuild cyassl-2.9.4.ebuild clean compile
net-libs:cyassl-2.9.4:20140608-142658.log net-libs:cyassl-2.9.4-r1:20140613-210221.log.xz |
||
|
Description
Jeroen Roovers (RETIRED)
2014-06-03 19:45:05 UTC
Well, that should be an easy fix. All that ax_enable_debug does is add those compiler flags:
Index: cyassl-2.9.4.ebuild
===================================================================
RCS file: /var/cvsroot/gentoo-x86/net-libs/cyassl/cyassl-2.9.4.ebuild,v
retrieving revision 1.2
diff -u -B -r1.2 cyassl-2.9.4.ebuild
--- cyassl-2.9.4.ebuild 31 May 2014 13:12:54 -0000 1.2
+++ cyassl-2.9.4.ebuild 3 Jun 2014 19:48:55 -0000
@@ -18,7 +18,7 @@
CRYPTO_OPTS="aes-gcm aes-ccm aes-ni blake2 camellia dsa ecc hc128 hkdf md2 md4 nullcipher psk leanpsk pkcs7 rabbit ripemd scep sha512 supportedcurves"
CERT_OPTS="ocsp crl crl-monitor savesession savecert sessioncerts testcert"
EXTRAS="atomicuser pkcallbacks sep maxfragment truncatedhmac tlsx"
-DEBUG="debug errorstrings memory test"
+DEBUG="errorstrings memory test"
#Note: sniffer is broken at the configure.ac level. Its not too important and we'll disable it for this release.
#IUSE="-dtls examples extra fortress ipv6 httpd mcapi pwdbased sni sniffer static-libs threads zlib ${CACHE_SIZE} ${CRYPTO_OPTS} ${CERT_OPTS} ${EXTRAS} ${DEBUG}"
@@ -116,7 +116,6 @@
$(use_enable truncatedhmac) \
$(use_enable tlsx) \
\
- $(use_enable debug) \
$(use_enable errorstrings) \
$(use_enable memory) \
\
I kept the USE=debug flag but fixed the behavior. Apparently giving either --enable-debug or --disable-debug will add the flags. If you simply don't sent it the flag, it doesn't add the debug flags. (In reply to Anthony Basile from comment #2) > I kept the USE=debug flag but fixed the behavior. Apparently giving either > --enable-debug or --disable-debug will add the flags. If you simply don't > sent it the flag, it doesn't add the debug flags. That sounds like a common PEBKAC interpreting what AC_ARG_ENABLE actually does (where action-if-true is assumed to be the --enable-foo case, whereas $enableval ought to be evaluated instead). Upstream might like to be told. (In reply to Jeroen Roovers from comment #3) > (In reply to Anthony Basile from comment #2) > > I kept the USE=debug flag but fixed the behavior. Apparently giving either > > --enable-debug or --disable-debug will add the flags. If you simply don't > > sent it the flag, it doesn't add the debug flags. > > That sounds like a common PEBKAC interpreting what AC_ARG_ENABLE actually > does (where action-if-true is assumed to be the --enable-foo case, whereas > $enableval ought to be evaluated instead). Upstream might like to be told. Yes, that's precisely what's up. And I'm working on it. (In reply to Anthony Basile from comment #4) > Yes, that's precisely what's up. And I'm working on it. https://github.com/cyassl/cyassl/issues/85 (In reply to Anthony Basile from comment #5) > (In reply to Anthony Basile from comment #4) > > Yes, that's precisely what's up. And I'm working on it. > > > https://github.com/cyassl/cyassl/issues/85 Fixed upstream. jer, how are you getting this? CFLAGS+="-g -ggdb -O0 -funroll-loops -fomit-frame-pointer" 1) When I do ebuild cyassl-2.9.4.ebuild clean configure, I get the following... * Installation prefix: /usr * System type: pc-linux-gnu * Host CPU: x86_64 * C Compiler: x86_64-pc-linux-gnu-gcc * C Flags: -Wno-pragmas -Wall -Wno-strict-aliasing -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-arith -Wpointer-sign -Wredundant-decls -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -fPIE -O3 -pipe -fstack-check * C++ Compiler: x86_64-pc-linux-gnu-g++ * C++ Flags: -O3 -pipe -fstack-check * CPP Flags: -fvisibility=hidden -DHAVE_LIBZ * LIB Flags: -z relro -z now * Debug enabled: no * Warnings as failure: no * make -j: 7 * VCS checkout: no 2) When I grep the build.log I don't see any -ggdb or -g. Created attachment 378522 [details]
build.log after ebuild cyassl-2.9.4.ebuild clean compile
Created attachment 378542 [details]
net-libs:cyassl-2.9.4:20140608-142658.log
cyassl-2.9.4 # grep -E '(fomit|funroll)' configure.ac OPTIMIZE_CFLAGS="-Os -fomit-frame-pointer" OPTIMIZE_FAST_CFLAGS="-O2 -fomit-frame-pointer" OPTIMIZE_HUGE_CFLAGS="-funroll-loops -DTFM_SMALL_SET -DTFM_HUGE_SET" (In reply to Jeroen Roovers from comment #9) > Created attachment 378542 [details] > net-libs:cyassl-2.9.4:20140608-142658.log In both logs I do see -funroll-loops -fomit-frame-pointer. I don't see a major problem with either for these being the default since the user can override them with their own CFLAGS. Only in your log do I see -ggdb. How are you getting that. Can I have the exact steps to reproduce because I just can't seem to hit it here. Niether log has -O0 or -g. Can other arche teams take a look at this. My actual cflags: CFLAGS="-Os -pipe -march=native -frecord-gcc-switches" Without USE=debug: * C Flags: -Wno-pragmas -Wall -Wno-strict-aliasing -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-ari th -Wpointer-sign -Wredundant-decls -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -fPIE -Os -pipe -march=native -frecord-gcc-switches With USE=debug: * C Flags: -g -ggdb -O0 -Wno-pragmas -Wall -Wno-strict-aliasing -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-arith -Wpointer-sign -Wredundant-decls -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -fPIE -Os -pipe -march=native -frecord-gcc-switches A lot of cflags are being injected in here for sure, QA's official policy is that this doens't block stabilization of security bugs (but we would be happy if this is fixed) (In reply to Rick Farina (Zero_Chaos) from comment #13) > A lot of cflags are being injected in here for sure, QA's official policy is > that this doens't block stabilization of security bugs (but we would be > happy if this is fixed) Is the fix to remove just -funroll-loops -fomit-frame-pointer ? (In reply to Anthony Basile from comment #14) > (In reply to Rick Farina (Zero_Chaos) from comment #13) > > A lot of cflags are being injected in here for sure, QA's official policy is > > that this doens't block stabilization of security bugs (but we would be > > happy if this is fixed) > > Is the fix to remove just -funroll-loops -fomit-frame-pointer ? Looks like it's injecting : -Wno-pragmas -Wall -Wno-strict-aliasing -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-ari th -Wpointer-sign -Wredundant-decls -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -fPIE so I'd personally say no, the fix is not just to remove -funrool-loops and -fomit-frame-pointer. I see 34 extra cflagss, not 2. Maybe some of them are desired or even required for cyassl, I wouldn't know, but there are definitely 34 extra cflags added that the user didn't request, and it would be cool to figure out why, and prevent that if possible. (In reply to Rick Farina (Zero_Chaos) from comment #15) > (In reply to Anthony Basile from comment #14) > > (In reply to Rick Farina (Zero_Chaos) from comment #13) > > > A lot of cflags are being injected in here for sure, QA's official policy is > > > that this doens't block stabilization of security bugs (but we would be > > > happy if this is fixed) > > > > Is the fix to remove just -funroll-loops -fomit-frame-pointer ? > > Looks like it's injecting : > > -Wno-pragmas -Wall -Wno-strict-aliasing -Wextra -Wunknown-pragmas > --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast > -Wchar-subscripts -Wcomment > -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized > -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes > -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-ari > th -Wpointer-sign -Wredundant-decls -Wshadow -Wsign-compare > -Wstrict-overflow=1 -Wswitch-enum -Wundef -Wunused -Wunused-result > -Wunused-variable -Wwrite-strings -fwrapv -fPIE > > so I'd personally say no, the fix is not just to remove -funrool-loops and > -fomit-frame-pointer. > > I see 34 extra cflagss, not 2. Maybe some of them are desired or even > required for cyassl, I wouldn't know, but there are definitely 34 extra > cflags added that the user didn't request, and it would be cool to figure > out why, and prevent that if possible. Oh you mean the -W flags too? These are compile time warnings, and shouldn't affect the final elfs. For example -Wswitch-enum just warns if in there is no case for an enum, etc. I know there's the issue of future proofing your code, ie, today's gcc warnings are tomorrows errors, so on upgrading gcc your code no longer compiles. In practice, this doesn't happen that often, and may even be something you want. Yes, adding all those warnings isn't needed, and honestly it seems best to find where all that is added and remove it. Also, as stated by multiple parties on irc, it is undesirable for a use flag to modify cflags. if all USE=debug does is change the cflags then it's not needed at all. We don't need things like -ggdb being injected, we all know if we want that or not. (In reply to Rick Farina (Zero_Chaos) from comment #17) > if all USE=debug does is change the cflags then it's not > needed at all that is not all it does Can the teams please check cyassl-2.9.4-r1. I've removed all the -W flags and all bug -fwrapv. The latter can change the behavior of arithmetic and therefore the way crypto is done. I also have some reservations about removing -fomit-frame-pointer on x86 since it does free up %ebp which, when hardening is used and -fstack-check, could lead to asm problems --- I tested and it isn't a problem when fastmath is off. Also I will reintroduce the -W flags in the future under USE=debug. There are numerous examples throughout the tree of this and it is a correct use of the flag. (In reply to Anthony Basile from comment #19) > I've removed all the -W flags and all bug -fwrapv. That should read: I've removed all the -W flags and the -f flags, except -fwrapv. (In reply to Anthony Basile from comment #19) > Can the teams please check cyassl-2.9.4-r1. I've removed all the -W flags > and all bug -fwrapv. The latter can change the behavior of arithmetic and > therefore the way crypto is done. I also have some reservations about > removing -fomit-frame-pointer on x86 since it does free up %ebp which, when > hardening is used and -fstack-check, could lead to asm problems --- I tested > and it isn't a problem when fastmath is off. You should have kept the -W flags as they don't affect compiling. > Also I will reintroduce the -W flags in the future under USE=debug. Please don't. That's not what USE=debug should mean. > There are numerous examples throughout the tree of this and it is a correct use of the flag. No, it isn't. Those "numerous examples" are bugs that should be fixed. (In reply to Jeroen Roovers from comment #21) > (In reply to Anthony Basile from comment #19) > > Can the teams please check cyassl-2.9.4-r1. I've removed all the -W flags > > and all bug -fwrapv. The latter can change the behavior of arithmetic and > > therefore the way crypto is done. I also have some reservations about > > removing -fomit-frame-pointer on x86 since it does free up %ebp which, when > > hardening is used and -fstack-check, could lead to asm problems --- I tested > > and it isn't a problem when fastmath is off. > > You should have kept the -W flags as they don't affect compiling. > > > Also I will reintroduce the -W flags in the future under USE=debug. > > Please don't. That's not what USE=debug should mean. > > > There are numerous examples throughout the tree of this and it is a correct use of the flag. > > No, it isn't. Those "numerous examples" are bugs that should be fixed. Is it a question of rutime debug vs build-time debug? I'm trying to figure out why this is a bad move. Anyhow, we can't have a blanket rule like USE flags can't modify CFLAGS because sometimes we must, eg USE=hardened might have to add -fPIC or the like. Please reopen if you find more flag issues. -funroll-loops -fomit-frame-pointer are still there. diff -u configure.ac{.orig,}
--- configure.ac.orig 2014-06-13 23:19:14.491698529 +0200
+++ configure.ac 2014-06-13 23:22:26.868203760 +0200
@@ -96,10 +96,8 @@
AM_PROG_CC_C_O
LT_LIB_M
-OPTIMIZE_CFLAGS="-Os -fomit-frame-pointer"
-OPTIMIZE_FAST_CFLAGS="-O2 -fomit-frame-pointer"
-OPTIMIZE_HUGE_CFLAGS="-funroll-loops -DTFM_SMALL_SET -DTFM_HUGE_SET"
-DEBUG_CFLAGS="-g -DDEBUG -DDEBUG_CYASSL"
+OPTIMIZE_HUGE_CFLAGS="-DTFM_SMALL_SET -DTFM_HUGE_SET"
+DEBUG_CFLAGS="-DDEBUG -DDEBUG_CYASSL"
thread_ls_on=no
# Thread local storage
@@ -1611,7 +1609,6 @@
esac
# add user C_EXTRA_FLAGS back
-CFLAGS="-fwrapv $CFLAGS"
OPTION_FLAGS="$AM_CFLAGS"
CREATE_HEX_VERSION
(In reply to Jeroen Roovers from comment #25) > diff -u configure.ac{.orig,} > --- configure.ac.orig 2014-06-13 23:19:14.491698529 +0200 > +++ configure.ac 2014-06-13 23:22:26.868203760 +0200 > @@ -96,10 +96,8 @@ > AM_PROG_CC_C_O > LT_LIB_M > > -OPTIMIZE_CFLAGS="-Os -fomit-frame-pointer" > -OPTIMIZE_FAST_CFLAGS="-O2 -fomit-frame-pointer" > -OPTIMIZE_HUGE_CFLAGS="-funroll-loops -DTFM_SMALL_SET -DTFM_HUGE_SET" > -DEBUG_CFLAGS="-g -DDEBUG -DDEBUG_CYASSL" > +OPTIMIZE_HUGE_CFLAGS="-DTFM_SMALL_SET -DTFM_HUGE_SET" > +DEBUG_CFLAGS="-DDEBUG -DDEBUG_CYASSL" > > thread_ls_on=no > # Thread local storage Okay with these. I will add this change in a bit. > @@ -1611,7 +1609,6 @@ > esac > > # add user C_EXTRA_FLAGS back > -CFLAGS="-fwrapv $CFLAGS" > OPTION_FLAGS="$AM_CFLAGS" > > CREATE_HEX_VERSION I added back -fwrapv. I don't think we should remove it because it might change some crypt calculation in a corner case. tree cleaning bug #495848 The problem is still there in -r1. Created attachment 378908 [details] net-libs:cyassl-2.9.4-r1:20140613-210221.log.xz (In reply to Anthony Basile from comment #27) > tree cleaning bug #495848 What does that even mean? (In reply to Jeroen Roovers from comment #29) > Created attachment 378908 [details] > net-libs:cyassl-2.9.4-r1:20140613-210221.log.xz > > (In reply to Anthony Basile from comment #27) > > tree cleaning bug #495848 > > What does that even mean? # cat gentoo-x86/profiles/package.mask | grep -C 2 cyassl # Anthony G. Basile <blueness@gentoo.org> (14 Jun 2014) # Masked for removal in 30 days, bug #495848 net-libs/cyassl It means that after other issues mgorny found, I decided this is not a reliable package to maintain. Its going to be removed from the tree. So this bug is either WONTFIX or maybe better OBSOLETE. |