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

build: enable missing OpenSSF-recommended warnings, with fixes #12489

Closed
wants to merge 34 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Dec 8, 2023

https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html
[as of 2023-11-29]

Enable new recommended warnings (except -Wsign-conversion):

  • enable -Wformat=2 for clang (in both cmake and autotools).
  • add CURL_PRINTF() internal attribute and mark functions accepting
    printf arguments with it. This is a copy of existing
    CURL_TEMP_PRINTF() but using __printf__ to make it compatible
    with redefinting the printf symbol:
    https://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_5.html#SEC94
  • fix CURL_PRINTF() and existing CURL_TEMP_PRINTF() for
    mingw-w64 and enable it on this platform.
  • enable -Wimplicit-fallthrough.
  • enable -Wtrampolines.
  • add -Wsign-conversion commented with a FIXME.
  • cmake: enable -pedantic-errors the way we do it with autotools.
    Follow-up to d5c0351 Enable and fix more GCC warnings #2747
  • lib/curl_trc.h: use CURL_FORMAT(), this also fixes it to enable format
    checks. Previously it was always disabled due to the internal printf
    macro.

Fix them:

  • fix bug where an set_ipv6_v6only() call was missed in builds with
    --disable-verbose / CURL_DISABLE_VERBOSE_STRINGS=ON.
  • add internal FALLTHROUGH() macro.
  • replace obsolete fall-through comments with FALLTHROUGH().
  • fix fallthrough markups: Delete redundant ones (showing up as
    warnings in most cases). Add missing ones. Fix indentation.
  • silence -Wformat-nonliteral warnings with llvm/clang.
  • fix one -Wformat-nonliteral warning.
  • fix new -Wformat and -Wformat-security warnings.
  • fix CURL_FORMAT_SOCKET_T value for mingw-w64. Also move its
    definition to lib/curl_setup.h allowing use in tests/server.
  • lib: fix two wrongly passed string arguments in log outputs.
    Co-authored-by: Jay Satiro
  • fix new -Wformat warnings on mingw-w64.

Closes #12489


TODO:

  • fix to enable format checks for mingw.
  • fix -Warith-conversion warnings. [other PR]
  • fix -Wsign-conversion warnings. [other PR]

@vszakats vszakats added the build label Dec 8, 2023
@vszakats vszakats changed the title enable recommended OpenSSF warnings build: enable recommended OpenSSF warnings Dec 8, 2023
include/curl/curl.h Outdated Show resolved Hide resolved
@vszakats vszakats changed the title build: enable recommended OpenSSF warnings build: enable missing OpenSSF-recommended warnings, with fixes Dec 8, 2023
@vszakats vszakats force-pushed the openssf branch 6 times, most recently from 8467031 to 722d4c8 Compare December 9, 2023 00:16
@github-actions github-actions bot added the CI Continuous Integration label Dec 9, 2023
@vszakats
Copy link
Member Author

vszakats commented Dec 9, 2023

This is ready now.

lib/cf-h1-proxy.c Outdated Show resolved Hide resolved
vszakats added a commit to vszakats/curl that referenced this pull request Dec 11, 2023
lib/curl_setup.h Outdated Show resolved Hide resolved
@vszakats
Copy link
Member Author

Renamed to use FALLTHROUGH();.

Is there something else to address?

Even though the logic of activating it differs from the rest of picky
warnings. Following how we implemented this for autotools.
@vszakats
Copy link
Member Author

vszakats commented Dec 15, 2023

The printf format checking "journey":

My goal was to enable it for internal functions and with the mingw-w64 compiler. This presented some issues, here's a summary:

  1. The existing CURL_TEMP_PRINTF() macro (in curl/mprintf.h) gets undefined after use, so had to add a new, persistent one to the internal headers.
  2. A one to one copy did not work because curl's headers redefine the symbol printf which happens to be the keyword we need to pass to __attribute__(format()). The solution is to pass the alternate keyword: __printf__. This has been around for ages, though probably a little younger than printf.
  3. I kept printf in the public (temporary) macro, though IMO it'd be cleaner to use the underscored flavour there too. The compatibility cost seems negligible, but would need double checking in the GNU C source code to confirm.
  4. This revealed the first round of legitimate warnings.
  5. The format attribute has been historically disabled for __MINGW32__ in curl, thus the next task was to enable it.
  6. In the first test, as expected, mingw started spitting warnings all over the place with __printf__.
  7. I tried __MINGW_PRINTF_FORMAT instead.
  8. This needs a fallback because it was introduced in mingw-w64 3.0.
  9. CI results were already useful to fix Windows-specific format issues.
  10. At this point mingw (tested with gcc) did not like the %qd format specifier in CURL_FORMAT_SOCKET_T. Fixed that by replacing it with %zd.
  11. Then it turned out while testing locally, that it doesn't work with __clang__ because for this compiler, the mingw-w64 header maps the __MINGW_PRINTF_FORMAT macro to printf which conflicts with the local macro, as seen earlier. The solution was to map it manually to __printf__ for __clang__. [CI]
  12. The next problem was that depending on GNU C version and/or mingw-w64 version, __MINGW_PRINTF_FORMAT gets mapped to gnu_printf in newer releases (gcc v13 job in AppVeyor), which works without warnings, but for gcc v6, v7, v9 it maps to ms_printf, which triggers warnings for %z. I fixed this by abandoning __MINGW_PRINTF_FORMAT and using gnu_printf straight, for mingw + gcc.
  13. This triggered another problem where curl sources use MS-specific formats for size_t types, raising warnings with older mingw-w64 + gcc (v6, v7, v9 again) releases. [CI] I fixed this by redefining CURL_FORMAT_CURL_OFF_T[U] for mingw to the GNU format. (Another option would be to disable format checks for older gcc versions.)

48 format strings had issues after extending verification to new functions.
37 additional format strings had issues when enabled for mingw (19 of them in libcurl, the rest in tests).

In summary: The idea is that libcurl, the curl tool and tests are all using the local printf() implementation (from lib/mprintf.c), which speaks the GNU format (and understands some MS extensions). The formats used in code must comply with that (and not whatever the CRT's printf() speaks). Then setup the format checker compiler attribute for the GNU format, where this isn't the default or needs a special keyword (gnu_printf).

I hope this makes sense or maybe even useful. Any remarks or comments are welcome, especially if it points to any flaw in the above, before merging this.

@bagder
Copy link
Member

bagder commented Dec 15, 2023

🎈 very nice work on this @vszakats!

mingw sets `__MINGW_PRINTF_FORMAT` to `printf` for clang. It means
it hits curl's `printf` redefinition macro and breaks. Make sure to
catch this combination and manually set `__printf__` instead.

This isn't stricly needed for `curl/mprintf.h` but let's do it anyway
for symmetry with `lib/curl_setup.h`.
Notes:

Also apply some formatting for existing `CURL_TEMP_PRINTF()` macro.

1. Initially used the `__MINGW_PRINTF_FORMAT` macro.
2. This needed fixup for mingw-w64 releases 1.x and 2.x that didn't
   define this macro.
3. Another fixup needed for __clang__ because it mapped it to `printf`
   which conflicts in `lib/curl_setup.h` with curl macro `printf`. Fix
   was to use `__printf__` explicitly.
4. Another fallout was for gcc 9, 7, 6, because `__MINGW_PRINTF_FORMAT`
   mapped to `ms_printf` for them, which doesn't recognize `%z`. Fixed
   by using `gnu_printf`.
5. Next fallout is that SOME formats actually use the MS format.
when compiling curl itself (including tests.)
@vszakats vszakats closed this in 3829759 Dec 16, 2023
@vszakats vszakats deleted the openssf branch December 16, 2023 13:18
vszakats added a commit to vszakats/curl that referenced this pull request Dec 16, 2023
- add `CURL_PRINTF()` to `imap_sendf()`.

- update memdebug functions to no longer trigger `-Wformat-nonliteral`
  warnings.

Follow-up to 3829759 curl#12489

Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request Dec 16, 2023
- add `CURL_PRINTF()` to `imap_sendf()`.

- update memdebug functions to no longer trigger `-Wformat-nonliteral`
  warnings.

Follow-up to 3829759 curl#12489

Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request Dec 16, 2023
- add `CURL_PRINTF()` to `imap_sendf()`.

- update memdebug functions to no longer trigger `-Wformat-nonliteral`
  warnings.

Follow-up to 3829759 curl#12489

Closes #xxxxx
vszakats added a commit that referenced this pull request Dec 18, 2023
- memdebug: update to not trigger `-Wformat-nonliteral` warnings.
- imap: mark `imap_sendf()` with  `CURL_PRINTF()`.
- tool_msgs: mark static function with `CURL_PRINTF()`.

Follow-up to 3829759 #12489

Closes #12540
vszakats added a commit that referenced this pull request Jan 27, 2024
- delete redundant warning suppressions for `-Wformat-nonliteral`.
  This now relies on `CURL_PRINTF()` and it's theoratically possible
  that this macro isn't active but the warning is. We're ignoring this
  as a corner-case here.

- replace two pragmas with code changes to avoid the warnings.

Follow-up to aee4ebe #12803
Follow-up to 0923012 #12540
Follow-up to 3829759 #12489

Reviewed-by: Daniel Stenberg
Closes #12812
vszakats added a commit that referenced this pull request Jan 28, 2024
- tool_msgs: delete redundant `-Wformat-nonliteral` suppression pragma.

- whitespace formatting in `mprintf.h`, lib518, lib537.

- lib518: fix wrong variable in `sizeof()`.

- lib518: bump variables to `rlim_t`.
  Follow-up to e2b3941 #1469

- lib518: sync error message with lib537
  Follow-up to 365322b

- lib518, lib537: replace `-Wformat-nonliteral` suppression pragmas
  by reworking test code.

Follow-up to 5b286c2 #12812
Follow-up to aee4ebe #12803
Follow-up to 0923012 #12540
Follow-up to 3829759 #12489

Reviewed-by: Daniel Stenberg
Closes #12814
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants