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

1. 
   fanquake commented at 7:31 am on May 5, 2020: member

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

   0checking whether C++ compiler accepts -Wformat-security... no
   1...
   2configure:15907: checking whether C++ compiler accepts -Wformat-security
   3configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security  conftest.cpp >&5
   4cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]
   5cc1plus: 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.

2. fanquake added the label Build system on May 5, 2020
3. fanquake added the label Needs gitian build on May 5, 2020
4. fanquake added the label Needs Guix build on May 5, 2020
5. 
   MarcoFalke commented at 10:29 am on May 5, 2020: member

   I don’t think we use printf, do we?
6. in configure.ac:356 in 3c04a7a11f outdated

   352@@ -353,7 +353,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   353   AX_CHECK_COMPILE_FLAG([-Wformat],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
   354   AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
   355   AX_CHECK_COMPILE_FLAG([-Wswitch],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wswitch"],,[[$CXXFLAG_WERROR]])
   356-  AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
   357+  AX_CHECK_COMPILE_FLAG([-Wformat -Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
   

   ---

   ---

   

   vasild commented at 10:30 am on May 5, 2020:

   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?

7. 
   DrahtBot commented at 6:13 pm on May 5, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
   • #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.

8. DrahtBot removed the label Needs gitian build on May 6, 2020
9. 
   fanquake commented at 6:10 am on May 6, 2020: member

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

    0# cat /etc/debian_version 
    1bullseye/sid
    2
    3# g++ --version
    4g++ (Ubuntu 9.3.0-10ubuntu2) 9.3.0
    5Copyright (C) 2019 Free Software Foundation, Inc.
    6This is free software; see the source for copying conditions.  There is NO
    7warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
    8
    9# g++ test.cpp -Wformat-security
   10#
   

    0# cat /etc/debian_version 
    1bullseye/sid
    2
    3# g++ --version
    4g++ (Debian 9.3.0-11) 9.3.0
    5Copyright (C) 2019 Free Software Foundation, Inc.
    6This is free software; see the source for copying conditions.  There is NO
    7warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
    8
    9# g++ test.cpp -Wformat-security
   10cc1plus: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
   11# 
   

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

10. 
    laanwj commented at 11:07 am on May 6, 2020: member

    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.

11. 
    MarcoFalke commented at 11:08 am on May 6, 2020: member

    Is the reason so that we check during compilation of subtrees or header files of libs?
12. DrahtBot removed the label Needs Guix build on May 7, 2020
13. MarcoFalke deleted a comment on May 28, 2020
14. MarcoFalke deleted a comment on May 28, 2020
15. DrahtBot added the label Needs rebase on May 28, 2020
16. 
    DrahtBot commented at 2:57 pm on May 28, 2020: member

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

17. fanquake force-pushed on Jul 15, 2020
18. fanquake removed the label Needs rebase on Jul 15, 2020
19. build: fix -Wformat-security check when compiling with GCC

    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.

    6cef3652d1
20. fanquake force-pushed on Jul 15, 2020
21. 
    fanquake commented at 9:52 am on July 15, 2020: member

    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?

22. 
    MarcoFalke commented at 10:10 am on July 15, 2020: member

    Any reason to not use wformat=2 as pointed out in OP?
23. 
    fanquake commented at 10:15 am on July 15, 2020: member

    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.

24. 
    practicalswift commented at 10:55 am on July 15, 2020: contributor

    ACK 6cef3652d143a1dddad1254cab0953561d24c1fa

    No harm in fixing this.

25. 
    hebasto commented at 11:40 am on July 15, 2020: member

    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.

26. 
    laanwj commented at 11:51 am on July 15, 2020: member

    ACK 6cef3652d143a1dddad1254cab0953561d24c1fa
27. laanwj merged this on Jul 15, 2020
28. laanwj closed this on Jul 15, 2020

    ---

29. fanquake deleted the branch on Jul 15, 2020
30. 
    hebasto commented at 2:44 pm on July 15, 2020: member

    post-merge ACK 6cef3652d143a1dddad1254cab0953561d24c1fa, tested on Debian 10.4 (GCC 8.3.0).
31. 
    vasild commented at 8:29 am on July 16, 2020: member

    ACK 6cef365

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

32. DrahtBot locked this on Feb 15, 2022

Contributors
fanquake MarcoFalke vasild DrahtBot laanwj practicalswift hebasto

Labels
Build system