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

Remove _FORTIFY_SOURCES from build: #1812

Closed
wants to merge 1 commit into from
Closed

Conversation

seelabs
Copy link
Collaborator

@seelabs seelabs commented Aug 10, 2016

This option is silently ignorned at -O0 on ubuntu. On fedora, we get a warning that requires compiling with optimization (-O).

I see two options:

  1. Increase the optimization level to -O1 when debugging
  2. Remove _FORTIFY_SOURCES

I'm opting for (2). We have been building this way since at least Oct/2014 and the clang sanitizers serve a similar function.

This option is silently ignorned at -O0
@willwray
Copy link

👍

__FORTIFY_SOURCES should be removed from the debug build __FORTHWITH. This gcc/clang option is intended to fortify release builds against buffer overflow exploits. The utility of fortifying debug builds is unclear, and, as @seelabs points out, it was disabled anyway by the change to the -O flag.

We should consider adding such 'security'/'hardening'/'fortification' options on release builds instead.

Notes

gcc/clang have 'fortified' default configuration on some platforms and so rippled currently builds on those platforms with source code hardening anyways, intended or not.
(I have an Ubuntu distro with __FORTIFY_SOURCES and -fstack-protector-strong set by default; rippled release builds contain many chk calls and debug builds have __stack_chk_fail in almost all *.o's.)

clang sanitizers are a different thing again. They do not yet support source fortification as this issue shows:
google/sanitizers, Support source fortification

A suggestion from Jakub Jelinek:

Well, -D_FORTIFY_SOURCE=2 does things that asan doesn't and can't do, so disabling fortification if you build with -fsanitize=address sounds like a very bad idea to me.

Links

Enhance application security with FORTIFY_SOURCE

"Strong" stack protection for GCC; -fstack-protector options that complement __FORTIFY_SOURCE.

un-fortifying-rippled stackoverflow question (incidentally, perhaps he wanted to unfortify rippled so that he could test exploits for attacking unfortified instances).

@MarkusTeufelberger
Copy link
Collaborator

MarkusTeufelberger commented Aug 10, 2016

See #1432 (reported in December 2015)

Edit: -Og would be better for debugging than -O1, but apparently there were some issues.

@scottschurr
Copy link
Collaborator

As a side note, I noticed that a scons debug build has "+DEBUG" appended to the --version command result. A debug cmake build, built as "cmake -Dtarget=clang.debug.nounity" does not append "+DEBUG" to the version. Just an interesting observation...

@scottschurr
Copy link
Collaborator

I'm fairly confident this has nothing to do with your change, but the Travis GCC Coverage build has had an internal compiler error building server.o three times in a row. It's vaguely possible that we need to split the server.cpp unity file into two pieces...

@scottschurr
Copy link
Collaborator

👍 Other than the Travis build problem this change looks good to me. I built clang.debug.nounity and clang.release using both scons and cmake on OS X. No warnings on any of the builds. Unit tests ran on all builds.

@seelabs seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 11, 2016
@seelabs
Copy link
Collaborator Author

seelabs commented Aug 11, 2016

@scottschurr Thanks for the note about the cmake build. I'll open an issue.

@codecov-io
Copy link

Current coverage is 71.00% (diff: 100%)

Merging #1812 into develop will increase coverage by 0.01%

@@            develop      #1812   diff @@
==========================================
  Files           869        869          
  Lines         69535      69535          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          49363      49376    +13   
+ Misses        20172      20159    -13   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update d51a278...e60981f

@nbougalis nbougalis mentioned this pull request Aug 15, 2016
@nbougalis
Copy link
Contributor

Merged as b0704b4

@nbougalis nbougalis closed this Aug 15, 2016
@seelabs seelabs deleted the fortify branch August 15, 2016 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants