build: fix -Wformat-security check when compiling with GCC #18882

fanquake · 2020-05-05T07:31:13Z

GCC expects -Wformat to be passed with -Wformat-security, which means
when we test for it in configure it currently fails:

checking whether C++ compiler accepts -Wformat-security... no
...
configure:15907: checking whether C++ compiler accepts -Wformat-security
configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security  conftest.cpp >&5
cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]
cc1plus: all warnings being treated as errors

and never gets added to our CXX flags. Note that Clang does not have this requirement and the check is working correctly there.

The change in this PR is the simple fix, however we might want to consider using something like -Wformat=2 in future, which in GCC is equivalent to -Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k. and similar in Clang.

maflcko · 2020-05-05T10:29:45Z

I don't think we use printf, do we?

vasild · 2020-05-05T10:30:32Z

configure.ac

-  AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wformat -Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])


I think it is counter intuitive to test whether flag X is supported and if yes, then compile with flag Y. IMO it is better to always have X equal to Y. In this particular case the code would depend on -Wformat being already set from somewhere else and would break if that is removed.

Having that in mind, would it be better to test whether -Wformat -Wformat-security is supported and if yes, then enable -Wformat -Wformat-security? We would end up with -Wformat being mentioned twice in the flags, but there is no downside to that?

DrahtBot · 2020-05-05T18:13:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: avoid repetitions when enabling warnings in configure.ac #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
Replace -Wthread-safety-analysis with broader -Wthread-safety #18635 (Replace -Wthread-safety-analysis with broader -Wthread-safety by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake · 2020-05-06T06:10:39Z

Looks like the issue here is more nuanced. -Wformat-security will be accepted alone by Ubuntu GCC, but not Debian GCC:

# cat /etc/debian_version 
bullseye/sid

# g++ --version
g++ (Ubuntu 9.3.0-10ubuntu2) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

# g++ test.cpp -Wformat-security
#

# cat /etc/debian_version 
bullseye/sid

# g++ --version
g++ (Debian 9.3.0-11) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

# g++ test.cpp -Wformat-security
cc1plus: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
#

I think I'll just look at switching to -Wformat=2. That should achieve the same result without the need for any additional flags.

laanwj · 2020-05-06T11:07:29Z

I don't think we use printf, do we?

I also wonder. Not that it hurts to enable the warning, but it seems pointless for our project.

maflcko · 2020-05-06T11:08:45Z

Is the reason so that we check during compilation of subtrees or header files of libs?

DrahtBot · 2020-05-28T14:57:03Z

🐙 This pull request conflicts with the target branch and needs rebase.

Debian GCC ignores -Wformat-security, without -Wformat, which means when we test for it, it currently fails: ```bash checking whether C++ compiler accepts -Wformat-security... no ... configure:15907: checking whether C++ compiler accepts -Wformat-security configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security conftest.cpp >&5 cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security] cc1plus: all warnings being treated as errors ``` Fix this by just combining the -Wformat and -Wformat-security checks together.

fanquake · 2020-07-15T09:52:08Z

I've modified the change to just combine the checking for -Wformat and -Wformat-security. This will fix the issue with Debian GCC, and worked fine with all of the compilers I tested which included:

Apple Clang: 11.0.3 (clang-1103.0.32.62)
Alpine GCC: gcc (Alpine 9.3.0) 9.3.0
Debian GCC: gcc (Debian 8.3.0-6) 8.3.0
Fedora GCC: gcc (GCC) 10.1.1 20200507 (Red Hat 10.1.1-1)
LLVM Clang: 10.0.0
Ubuntu GCC: gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0

@vasild and maybe @hebasto would you like to take another look?

maflcko · 2020-07-15T10:10:07Z

Any reason to not use wformat=2 as pointed out in OP?

fanquake · 2020-07-15T10:15:39Z

Any reason to not use wformat=2 as pointed out in OP?

I don't have the details on hand, but I remember it was generating additional warnings in in some dependencies.

practicalswift · 2020-07-15T10:55:30Z

ACK 6cef365

No harm in fixing this.

hebasto · 2020-07-15T11:40:15Z

Concept ACK.

FWIW, the -Wformat diagnostic is enabled by default in Clang, and it is enabled by the -Wall (which is already in use) in GCC.

laanwj · 2020-07-15T11:51:16Z

ACK 6cef365

hebasto

post-merge ACK 6cef365, tested on Debian 10.4 (GCC 8.3.0).

vasild · 2020-07-16T08:29:25Z

ACK 6cef365

Excellent: test if -Wformat -Wformat-security is supported and if yes, then enable -Wformat -Wformat-security. KISS.

fanquake added Build system Needs gitian build DrahtBot Guix build requested labels May 5, 2020

fanquake mentioned this pull request May 5, 2020

build: avoid repetitions when enabling warnings in configure.ac #18857

Closed

vasild reviewed May 5, 2020

View reviewed changes

DrahtBot removed the Needs gitian build label May 6, 2020

DrahtBot removed the DrahtBot Guix build requested label May 7, 2020

DrahtBot mentioned this pull request May 9, 2020

Replace -Wthread-safety-analysis with broader -Wthread-safety #18635

Merged

bitcoin deleted a comment from DrahtBot May 28, 2020

DrahtBot added the Needs rebase label May 28, 2020

fanquake force-pushed the fix_gcc_wformat_security branch from 3c04a7a to 987c7df Compare July 15, 2020 09:48

fanquake removed the Needs rebase label Jul 15, 2020

fanquake force-pushed the fix_gcc_wformat_security branch from 987c7df to 6cef365 Compare July 15, 2020 09:50

laanwj merged commit 2c4093e into bitcoin:master Jul 15, 2020

fanquake deleted the fix_gcc_wformat_security branch July 15, 2020 12:22

hebasto reviewed Jul 15, 2020

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: fix -Wformat-security check when compiling with GCC #18882

build: fix -Wformat-security check when compiling with GCC #18882

Uh oh!

fanquake commented May 5, 2020

Uh oh!

maflcko commented May 5, 2020

Uh oh!

vasild May 5, 2020

Uh oh!

DrahtBot commented May 5, 2020 •

edited

Loading

Uh oh!

fanquake commented May 6, 2020

Uh oh!

laanwj commented May 6, 2020 •

edited

Loading

Uh oh!

maflcko commented May 6, 2020

Uh oh!

DrahtBot commented May 28, 2020

Uh oh!

fanquake commented Jul 15, 2020

Uh oh!

maflcko commented Jul 15, 2020

Uh oh!

fanquake commented Jul 15, 2020

Uh oh!

practicalswift commented Jul 15, 2020

Uh oh!

hebasto commented Jul 15, 2020

Uh oh!

laanwj commented Jul 15, 2020

Uh oh!

hebasto left a comment

Uh oh!

vasild commented Jul 16, 2020

Uh oh!

Uh oh!

		AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
		AX_CHECK_COMPILE_FLAG([-Wformat -Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])

build: fix -Wformat-security check when compiling with GCC #18882

build: fix -Wformat-security check when compiling with GCC #18882

Uh oh!

Conversation

fanquake commented May 5, 2020

Uh oh!

maflcko commented May 5, 2020

Uh oh!

vasild May 5, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake commented May 6, 2020

Uh oh!

laanwj commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 6, 2020

Uh oh!

DrahtBot commented May 28, 2020

Uh oh!

fanquake commented Jul 15, 2020

Uh oh!

maflcko commented Jul 15, 2020

Uh oh!

fanquake commented Jul 15, 2020

Uh oh!

practicalswift commented Jul 15, 2020

Uh oh!

hebasto commented Jul 15, 2020

Uh oh!

laanwj commented Jul 15, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Jul 16, 2020

Uh oh!

Uh oh!

DrahtBot commented May 5, 2020 •

edited

Loading

laanwj commented May 6, 2020 •

edited

Loading