Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

explicit_bzero optimized away when using link-time optimization #5

Closed
jiixyj opened this issue Jul 18, 2014 · 9 comments

Comments

@jiixyj
Copy link

commented Jul 18, 2014

Hi,
I've adapted "libressl-2.0.2/tests/explicit_bzero.c" to test some implementations of explicit_bzero.
Compiling this test file with "gcc49 -O1 -flto -DELF_HOOK_IMPL gistfile1.c" on FreeBSD optimizes away the explicit_bzero calls and the __explicit_bzero_hook symbol, resulting in a test failure. This is the current implementation used by OpenBSD and LibreSSL. I don't know if this is caused by toolchain advances or differences in weak symbol semantics. On OpenBSD 5.5 the test works fine with the system compiler and GCC 4.8 from ports.

I've fixed the elf hook implementation by removing the default implementation of __explicit_bzero_hook and putting the call to __explicit_bzero_hook behind an if check.

Another fix could be to use one of the VOLATILE_{1,2,3}_IMPL implementations as they rely on portable C semantics.

@busterb

This comment has been minimized.

Copy link
Member

commented Jul 31, 2014

Thanks for the report and suggested fix, we'll take a look.

@busterb

This comment has been minimized.

Copy link
Member

commented Jul 31, 2014

Confirmed, I can reproduce on Ubuntu with gcc 4.9.1 using your test file as well.

@busterb

This comment has been minimized.

Copy link
Member

commented Jul 31, 2014

I tried to recreate this same test within libressl but could not. I think the test might have a slight flaw.

Note that libressl builds explicit_bzero with independent compiler flags (as part of the 'libcompatnoopt' library, an automake hack to allow using different flags). This prevents the compiler from considering this object file for LTO even if it was enabled globally for the project. I tried compiling the explicit_bzero portion of your test file as a separate object as well. Linking that with the rest of the test, I was unable to reproduce, though I may have done something wrong.

Are you able to reproduce any issues when explicit_bzero is built as an independent object, or able to produce any CFLAGS/LDFLAGS, etc. settings that can cause the same when libressl is building libcompatnoopt?

@jiixyj

This comment has been minimized.

Copy link
Author

commented Jul 31, 2014

Yes, putting explicit_zero into its own source file and compiling with:

gcc49 -O1 -flto -DELF_HOOK_IMPL -c explicit_bzero.c
gcc49 -O1 -flto -DEXTERN_IMPL test.c explicit_bzero.o

also makes the test fail. There has to be a function prototype for explicit_bzero in the test case, though. If there isn't, GCC won't optimize the symbol away and warns with "warning: implicit declaration of function 'explicit_bzero'" when using -Wall. So currently, the unmodified test case won't ever fail on systems that don't have the explicit_zero declaration in string.h (at least when using GCC).

I'll check again if I can reproduce the issue within libcompatnoopt.

@jiixyj

This comment has been minimized.

Copy link
Author

commented Jul 31, 2014

I can confirm that when '-O0 -flto' is used the symbol is not optimized away. So libressl is safe for now. I guess this still could catch people out who drop explicit_bzero.c from OpenBSD into their own projects.

@bob-beck

This comment has been minimized.

Copy link
Member

commented Jul 31, 2014

People who drop code from OpenBSD into their own projects without later
following updates to it, or paying attention to how it is compiled, or how
things are different in openbsd as compared to their own projects, will get
caught. We have seen this with other things before.
(Such as arc4random in freebsd)

On Thu, Jul 31, 2014 at 7:35 AM, jiixyj notifications@github.com wrote:

I can confirm that when '-O0 -flto' is used the symbol is not optimized
away. So libressl is safe for now. I guess this still could catch people
out who drop explicit_bzero.c from OpenBSD into their own projects.


Reply to this email directly or view it on GitHub
#5 (comment)
.

@jiixyj

This comment has been minimized.

Copy link
Author

commented Jul 31, 2014

I agree completely. I have tested this now on OpenBSD 5.5 with GCC 4.8.2 from ports:

egcc -O1 -flto -c explicit_bzero.c
egcc -O1 -flto -fwhole-program -DEXTERN_IMPL test.c explicit_bzero.o

…and the test fails. So "-fwhole-program" is needed to uncover this problem on OpenBSD. With this patch here the test passes again. GCC doesn't seem to remove weak symbols if they are not defined even when doing whole program optimization. I didn't rebuild the whole system with this patch, though.

@ghost

This comment has been minimized.

Copy link

commented Aug 5, 2014

We might want to add a similar workaround as done by BoringSSL in

https://boringssl.googlesource.com/boringssl/+/ad1907fe73334d6c696c8539646c21b11178f20f%5E!/#F0

and adding this to explicit_bzero():

/* As best as we can tell, this is sufficient to break any optimisations that
might try to eliminate "superfluous" memsets. If there's an easy way to
detect memset_s, it would be better to use that. */
#if defined(OPENSSL_WINDOWS)
asm;
#else
__asm
volatile("" : : "r"(ptr) : "memory");
#endif

@busterb

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

I will run it by matthew as a general improvement in the openbsd libc. This should be useful at some point, though this isn't an issue how we build it in libressl today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.