Skip to content

Enable FORTIFY_SOURCE=3 when supported by the compiler #12381

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 17, 2023
Merged

Enable FORTIFY_SOURCE=3 when supported by the compiler #12381

merged 3 commits into from
Jan 17, 2023

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Jan 5, 2023

Short description

Barely tested, we should at least measure the performance impact and ponder making that optional.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@rgacogne
Enable FORTIFY_SOURCE=3 when supported by the compiler
1b69ef9 
Barely tested, we should at least measure the performance impact and
ponder making that optional.
@rgacogne rgacogne added this to the common-soon milestone Jan 5, 2023
@rgacogne rgacogne mentioned this pull request Jan 5, 2023
Closed
@appliedprivacy
Copy link
Contributor

appliedprivacy commented Jan 5, 2023 •
edited
Loading

thanks!

When testing it I noticed that gcc (unlike clang) does not result in -D_FORTIFY_SOURCE=3 on Ubuntu 22.10:

gcc (Ubuntu 12.2.0-3ubuntu1) 12.2.0 (fails)

config.log:

[...]
configure:27520: checking whether C++ compiler handles -D_FORTIFY_SOURCE=3
configure:27540: g++ -std=c++17 -o conftest -Wall -W -Werror --param ssp-buffer-size=4 -fstack-protector -Wall -Wextra -Wshadow -Wno-unused-parameter -Wmissing-declarations -Wredundant-decls -fvisibility=hidden -g -O2   -D_FORTIFY_SOURCE=3   -rdynamic conftest.cpp  >&5
<command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror]
<built-in>: note: this is the location of the previous definition
cc1plus: all warnings being treated as errors
configure:27540: $? = 1
[...]

configure: Configuration summary
configure: =====================
configure: 
configure: PowerDNS Recursor 0.0.0.0.master.g44d305b302.dirty configured with: (no options)
configure: 
configure: CC: gcc
configure: CXX: g++ -std=c++17
configure: LD: /usr/bin/ld -m elf_x86_64
configure: CFLAGS:  -fPIE -DPIE -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 --param ssp-buffer-size=4 -fstack-protector -Wall -Wextra -Wshadow -Wno-unused-parameter -Wmissing-declarations -Wredundant-decls -fvisibility=hidden -g -O2 -g -O2
configure: CPPFLAGS: 
configure: CXXFLAGS:  -fPIE -DPIE -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 --param ssp-buffer-size=4 -fstack-protector -Wall -Wextra -Wshadow -Wno-unused-parameter -Wmissing-declarations -Wredundant-decls -fvisibility=hidden -g -O2 
configure: LDFLAGS: -Wl,-z -Wl,relro -Wl,-z -Wl,now  -rdynamic
configure: LIBS: 
configure: BOOST_CPPFLAGS:  -pthread

Ubuntu clang version 15.0.2-1

config.log:

[...]
configure:27520: checking whether C++ compiler handles -D_FORTIFY_SOURCE=3
configure:27540: /usr/bin/clang++ -std=c++17 -o conftest -Wall -W -Werror --param ssp-buffer-size=4 -fstack-protector -Wall -Wextra -Wshadow -Wno-unused-parameter -Wmissing-declarations -Wredundant-decls -fvisibility=hidden -g -O2  -Wunknown-warning-option -Werror -D_FORTIFY_SOURCE=3   -rdynamic conftest.cpp  >&5
configure:27540: $? = 0
[...]

configure: Configuration summary
configure: =====================
configure: 
configure: PowerDNS Recursor 0.0.0.0.master.g44d305b302.dirty configured with:  'CC=/usr/bin/clang' 'CXX=/usr/bin/clang++'
configure: 
configure: CC: /usr/bin/clang
configure: CXX: /usr/bin/clang++ -std=c++17
configure: LD: /usr/bin/ld -m elf_x86_64
configure: CFLAGS:  -fPIE -DPIE -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 --param ssp-buffer-size=4 -fstack-protector -Wall -Wextra -Wshadow -Wno-unused-parameter -Wmissing-declarations -Wredundant-decls -fvisibility=hidden -g -O2 -g -O2
configure: CPPFLAGS: 
configure: CXXFLAGS:  -fPIE -DPIE -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 --param ssp-buffer-size=4 -fstack-protector -Wall -Wextra -Wshadow -Wno-unused-parameter -Wmissing-declarations -Wredundant-decls -fvisibility=hidden -g -O2 
configure: LDFLAGS: -Wl,-z -Wl,relro -Wl,-z -Wl,now  -rdynamic
configure: LIBS: 
configure: BOOST_CPPFLAGS:

@rgacogne
Copy link
Member Author

rgacogne commented Jan 6, 2023

It works for me with GCC 12.2.0 on Arch. It requires glibc >= 2.34, but Ubuntu 22.10 is supposed to have 2.36, so I don't know.

@rgacogne
Copy link
Member Author

rgacogne commented Jan 6, 2023

Oh, you have -Werror..

@rgacogne
Add an --enable-fortify-source configure option
cd68e31 
The option defaults to 2 to keep the existing behaviour, but 3 and
auto are supported, with auto trying to select the highest version
supported by the compiler.
@rgacogne
Copy link
Member Author

rgacogne commented Jan 9, 2023

I just pushed a commit adding an --enable-fortify-source configure option. The option defaults to 2 to keep the existing behaviour, but 3 and auto are supported, with auto trying to select the highest version supported by the compiler.

@rgacogne rgacogne marked this pull request as ready for review January 10, 2023 16:20
fredmorcos
fredmorcos suggested changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@fredmorcos fredmorcos left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is behaving strangely for me. Using the first commit only, and the latest clang and gcc on Archlinux I get:

checking whether C++ compiler handles -D_FORTIFY_SOURCE=3... no
checking whether C++ compiler handles -D_FORTIFY_SOURCE=2... no

Additionally, with the second commit (which adds --enable-fortify-source) I get the following when I don't pass --enable-fortify-source:

checking whether FORTIFY_SOURCE is supported... checking whether C++ compiler handles -D_FORTIFY_SOURCE=2... no
checking whether C++ compiler handles -D_FORTIFY_SOURCE=1... no
no
  1. The output is garbled.
  2. It still says that none of the levels are supported.
  3. It sometimes can't find either libcrypto or libdecaf headers anymore, varies between runs:
checking whether compiling and linking against OpenSSL's libcrypto works... no
configure: error: OpenSSL/libcrypto not found

checking for decaf.hxx... no
configure: error: cannot find libdecaf headers

Although those work fine with the first commit. I can't tell what the second commit introduces that might break this. It happens whether I pass --enable-fortify-source or not.

@rgacogne
Copy link
Member Author

I pushed a commit fixing the interleaving during the detection of FORTIFY_SOURCE.

As for the rest, I don't get it :-/
Here are my results on Arch, depending on the value of --enable-fortify-source:

  • default (--enable-fortify-source not specified on the configure line)
checking whether C++ compiler handles -D_FORTIFY_SOURCE=2... yes
checking whether FORTIFY_SOURCE is supported... 2
  • auto
checking whether C++ compiler handles -D_FORTIFY_SOURCE=3... yes
checking whether FORTIFY_SOURCE is supported... 3
  • 3
checking whether C++ compiler handles -D_FORTIFY_SOURCE=3... yes
checking whether FORTIFY_SOURCE is supported... 3
  • 2
checking whether C++ compiler handles -D_FORTIFY_SOURCE=2... yes
checking whether FORTIFY_SOURCE is supported... 2
  • 1
checking whether C++ compiler handles -D_FORTIFY_SOURCE=1... yes
checking whether FORTIFY_SOURCE is supported... 1
  • no
checking whether FORTIFY_SOURCE is supported... no

And in all cases I get:

checking for decaf.hxx... yes

@fredmorcos
Copy link
Contributor

My issues were caused by the fact that FORTIFY_SOURCE requires at least -O and I was configuring with -O0.

This output is still interleaved though:

checking whether FORTIFY_SOURCE is supported... checking whether C++ compiler handles -D_FORTIFY_SOURCE=3... yes
3

@rgacogne
Copy link
Member Author

My issues were caused by the fact that FORTIFY_SOURCE requires at least -O and I was configuring with -O0.

Ah, right. We could change the optimization when we check if the compiler supports this feature, but that doesn't feel right since it will not work in the end anyway.

This output is still interleaved though:

checking whether FORTIFY_SOURCE is supported... checking whether C++ compiler handles -D_FORTIFY_SOURCE=3... yes
3

Weird, it really shouldn't be with the latest commit.

@fredmorcos
Copy link
Contributor

Weird, it really shouldn't be with the latest commit.

Argh. Sorry. I fetched and forgot to rebase. It works fine now. Thank you!

@rgacogne rgacogne merged commit ecc6996 into PowerDNS:master Jan 17, 2023
@rgacogne rgacogne deleted the fortify-3 branch January 17, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredmorcos fredmorcos fredmorcos approved these changes

@omoerbeek omoerbeek Awaiting requested review from omoerbeek

Assignees
No one assigned
Labels
auth dnsdist enhancement rec
Projects
None yet
Milestone
common-soon
Development

Successfully merging this pull request may close these issues.
3 participants
@rgacogne @appliedprivacy @fredmorcos