Skip to content

Commit

Permalink
build: enable missing OpenSSF-recommended warnings, with fixes
Browse files Browse the repository at this point in the history 
https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C  .html
as of 2023-11-29 [1].

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 #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.

[1] https://github.com/ossf/wg-best-practices-os-developers/blob/56c0fde3895bfc55c8a973ef49a2572c507b2ae1/docs/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.md

Closes #12489
  • Loading branch information
@vszakats
vszakats committed Dec 16, 2023
1 parent ba8752e commit 3829759
Show file tree
Hide file tree
Showing 88 changed files with 531 additions and 318 deletions.
31 changes: 24 additions & 7 deletions CMake/PickyWarnings.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 23,12 @@
###########################################################################
include(CheckCCompilerFlag)

unset(WPICKY)

if(CURL_WERROR AND CMAKE_COMPILER_IS_GNUCC AND NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 5.0)
set(WPICKY "${WPICKY} -pedantic-errors")
endif()

if(PICKY_COMPILER)
if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID MATCHES "Clang")

Expand Down Expand Up @@ -86,8 92,10 @@ if(PICKY_COMPILER)
-Wno-sign-conversion # clang 2.9 gcc 4.3
-Wno-system-headers # clang 1.0 gcc 3.0
# -Wpadded # clang 2.9 gcc 4.1 # Not used because we cannot change public structs
-Wredundant-decls # clang 2.7 gcc 4.1
-Wold-style-definition # clang 2.7 gcc 3.4
-Wredundant-decls # clang 2.7 gcc 4.1
# -Wsign-conversion # clang 2.9 gcc 4.3 # FIXME
# -Wno-error=sign-conversion # FIXME
-Wstrict-prototypes # clang 1.0 gcc 3.3
# -Wswitch-enum # clang 2.7 gcc 4.1 # Not used because this basically disallows default case
-Wtype-limits # clang 2.7 gcc 4.3
Expand All @@ -110,6 118,7 @@ if(PICKY_COMPILER)
-Wshift-sign-overflow # clang 2.9
-Wshorten-64-to-32 # clang 1.0
-Wlanguage-extension-token # clang 3.0
-Wformat=2 # clang 3.0 gcc 4.8
)
# Enable based on compiler version
if((CMAKE_C_COMPILER_ID STREQUAL "Clang" AND NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 3.6) OR
Expand All @@ -135,6 144,12 @@ if(PICKY_COMPILER)
-Wextra-semi-stmt # clang 7.0 appleclang 10.3
)
endif()
if((CMAKE_C_COMPILER_ID STREQUAL "Clang" AND NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 10.0) OR
(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" AND NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 12.4))
list(APPEND WPICKY_ENABLE
-Wimplicit-fallthrough # clang 4.0 gcc 7.0 appleclang 12.4 # we have silencing markup for clang 10.0 and above only
)
endif()
else() # gcc
list(APPEND WPICKY_DETECT
${WPICKY_COMMON}
Expand All @@ -147,6 162,7 @@ if(PICKY_COMPILER)
-Wmissing-parameter-type # gcc 4.3
-Wold-style-declaration # gcc 4.3
-Wstrict-aliasing=3 # gcc 4.0
-Wtrampolines # gcc 4.3
)
endif()
if(NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 4.5 AND MINGW)
Expand All @@ -156,7 172,7 @@ if(PICKY_COMPILER)
endif()
if(NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 4.8)
list(APPEND WPICKY_ENABLE
-Wformat=2 # clang 3.0 gcc 4.8 (clang part-default, enabling it fully causes -Wformat-nonliteral warnings)
-Wformat=2 # clang 3.0 gcc 4.8
)
endif()
if(NOT CMAKE_C_COMPILER_VERSION VERSION_LESS 5.0)
Expand All @@ -179,6 195,7 @@ if(PICKY_COMPILER)
-Wduplicated-branches # gcc 7.0
-Wformat-overflow=2 # gcc 7.0
-Wformat-truncation=2 # gcc 7.0
-Wimplicit-fallthrough # clang 4.0 gcc 7.0
-Wrestrict # gcc 7.0
)
endif()
Expand All @@ -191,8 208,6 @@ if(PICKY_COMPILER)

#

unset(WPICKY)

foreach(_CCOPT IN LISTS WPICKY_ENABLE)
set(WPICKY "${WPICKY} ${_CCOPT}")
endforeach()
Expand All @@ -209,8 224,10 @@ if(PICKY_COMPILER)
set(WPICKY "${WPICKY} ${_CCOPT}")
endif()
endforeach()

message(STATUS "Picky compiler options:${WPICKY}")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${WPICKY}")
endif()
endif()

if(WPICKY)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${WPICKY}")
message(STATUS "Picky compiler options:${WPICKY}")
endif()
8 changes: 5 additions & 3 deletions docs/examples/chkspeed.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 126,17 @@ int main(int argc, char *argv[])
default:
fprintf(stderr, "\r%s: invalid parameter %s\n",
appname, *argv 3);
exit(1);
return 1;
}
break;
}
/* FALLTHROUGH */
fprintf(stderr, "\r%s: invalid or unknown option %s\n",
appname, *argv);
return 1;
default:
fprintf(stderr, "\r%s: invalid or unknown option %s\n",
appname, *argv);
exit(1);
return 1;
}
}
else {
Expand Down
5 changes: 2 additions & 3 deletions docs/examples/debug.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 95,7 @@ int my_trace(CURL *handle, curl_infotype type,
switch(type) {
case CURLINFO_TEXT:
fprintf(stderr, "== Info: %s", data);
/* FALLTHROUGH */
default: /* in case a new one is introduced to shock us */
return 0;

case CURLINFO_HEADER_OUT:
text = "=> Send header";
break;
Expand All @@ -117,6 114,8 @@ int my_trace(CURL *handle, curl_infotype type,
case CURLINFO_SSL_DATA_IN:
text = "<= Recv SSL data";
break;
default: /* in case a new one is introduced to shock us */
return 0;
}

dump(text, stderr, (unsigned char *)data, size, config->trace_ascii);
Expand Down
5 changes: 2 additions & 3 deletions docs/examples/http2-download.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 115,7 @@ int my_trace(CURL *handle, curl_infotype type,
switch(type) {
case CURLINFO_TEXT:
fprintf(stderr, "== %u Info: %s", num, data);
/* FALLTHROUGH */
default: /* in case a new one is introduced to shock us */
return 0;

case CURLINFO_HEADER_OUT:
text = "=> Send header";
break;
Expand All @@ -137,6 134,8 @@ int my_trace(CURL *handle, curl_infotype type,
case CURLINFO_SSL_DATA_IN:
text = "<= Recv SSL data";
break;
default: /* in case a new one is introduced to shock us */
return 0;
}

dump(text, num, (unsigned char *)data, size, 1);
Expand Down
5 changes: 2 additions & 3 deletions docs/examples/http2-serverpush.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 100,7 @@ int my_trace(CURL *handle, curl_infotype type,
switch(type) {
case CURLINFO_TEXT:
fprintf(stderr, "== Info: %s", data);
/* FALLTHROUGH */
default: /* in case a new one is introduced to shock us */
return 0;

case CURLINFO_HEADER_OUT:
text = "=> Send header";
break;
Expand All @@ -122,6 119,8 @@ int my_trace(CURL *handle, curl_infotype type,
case CURLINFO_SSL_DATA_IN:
text = "<= Recv SSL data";
break;
default: /* in case a new one is introduced to shock us */
return 0;
}

dump(text, (unsigned char *)data, size, 1);
Expand Down
5 changes: 2 additions & 3 deletions docs/examples/http2-upload.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 133,7 @@ int my_trace(CURL *handle, curl_infotype type,
switch(type) {
case CURLINFO_TEXT:
fprintf(stderr, "%s [%d] Info: %s", timebuf, num, data);
/* FALLTHROUGH */
default: /* in case a new one is introduced to shock us */
return 0;

case CURLINFO_HEADER_OUT:
text = "=> Send header";
break;
Expand All @@ -155,6 152,8 @@ int my_trace(CURL *handle, curl_infotype type,
case CURLINFO_SSL_DATA_IN:
text = "<= Recv SSL data";
break;
default: /* in case a new one is introduced to shock us */
return 0;
}

dump(text, num, (unsigned char *)data, size, 1);
Expand Down
5 changes: 2 additions & 3 deletions docs/examples/multi-debugcallback.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 100,7 @@ int my_trace(CURL *handle, curl_infotype type,
switch(type) {
case CURLINFO_TEXT:
fprintf(stderr, "== Info: %s", data);
/* FALLTHROUGH */
default: /* in case a new one is introduced to shock us */
return 0;

case CURLINFO_HEADER_OUT:
text = "=> Send header";
break;
Expand All @@ -116,6 113,8 @@ int my_trace(CURL *handle, curl_infotype type,
case CURLINFO_DATA_IN:
text = "<= Recv data";
break;
default: /* in case a new one is introduced to shock us */
return 0;
}

dump(text, stderr, data, size, TRUE);
Expand Down
12 changes: 9 additions & 3 deletions include/curl/mprintf.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 34,16 @@ extern "C" {

#if (defined(__GNUC__) || defined(__clang__)) && \
defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && \
!defined(__MINGW32__) && !defined(CURL_NO_FMT_CHECKS)
#define CURL_TEMP_PRINTF(a,b) __attribute__ ((format(printf, a, b)))
!defined(CURL_NO_FMT_CHECKS)
#if defined(__MINGW32__) && !defined(__clang__)
#define CURL_TEMP_PRINTF(fmt, arg) \
__attribute__((format(gnu_printf, fmt, arg)))
#else
#define CURL_TEMP_PRINTF(a,b)
#define CURL_TEMP_PRINTF(fmt, arg) \
__attribute__((format(printf, fmt, arg)))
#endif
#else
#define CURL_TEMP_PRINTF(fmt, arg)
#endif

CURL_EXTERN int curl_mprintf(const char *format, ...) CURL_TEMP_PRINTF(1, 2);
Expand Down
8 changes: 4 additions & 4 deletions lib/cf-h1-proxy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 183,7 @@ static void h1_tunnel_go_state(struct Curl_cfilter *cf,
infof(data, "CONNECT phase completed");
data->state.authproxy.done = TRUE;
data->state.authproxy.multipass = FALSE;
/* FALLTHROUGH */
FALLTHROUGH();
case H1_TUNNEL_FAILED:
if(new_state == H1_TUNNEL_FAILED)
CURL_TRC_CF(data, cf, "new tunnel state 'failed'");
Expand Down Expand Up @@ -912,7 912,7 @@ static CURLcode H1_CONNECT(struct Curl_cfilter *cf,
if(result)
goto out;
h1_tunnel_go_state(cf, ts, H1_TUNNEL_CONNECT, data);
/* FALLTHROUGH */
FALLTHROUGH();

case H1_TUNNEL_CONNECT:
/* see that the request is completely sent */
Expand All @@ -921,7 921,7 @@ static CURLcode H1_CONNECT(struct Curl_cfilter *cf,
if(result || !done)
goto out;
h1_tunnel_go_state(cf, ts, H1_TUNNEL_RECEIVE, data);
/* FALLTHROUGH */
FALLTHROUGH();

case H1_TUNNEL_RECEIVE:
/* read what is there */
Expand All @@ -936,7 936,7 @@ static CURLcode H1_CONNECT(struct Curl_cfilter *cf,
goto out;
/* got it */
h1_tunnel_go_state(cf, ts, H1_TUNNEL_RESPONSE, data);
/* FALLTHROUGH */
FALLTHROUGH();

case H1_TUNNEL_RESPONSE:
CURL_TRC_CF(data, cf, "CONNECT response");
Expand Down
6 changes: 3 additions & 3 deletions lib/cf-h2-proxy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 155,7 @@ static void h2_tunnel_go_state(struct Curl_cfilter *cf,
infof(data, "CONNECT phase completed");
data->state.authproxy.done = TRUE;
data->state.authproxy.multipass = FALSE;
/* FALLTHROUGH */
FALLTHROUGH();
case H2_TUNNEL_FAILED:
if(new_state == H2_TUNNEL_FAILED)
CURL_TRC_CF(data, cf, "[%d] new tunnel state 'failed'", ts->stream_id);
Expand Down Expand Up @@ -1033,7 1033,7 @@ static CURLcode H2_CONNECT(struct Curl_cfilter *cf,
if(result)
goto out;
h2_tunnel_go_state(cf, ts, H2_TUNNEL_CONNECT, data);
/* FALLTHROUGH */
FALLTHROUGH();

case H2_TUNNEL_CONNECT:
/* see that the request is completely sent */
Expand All @@ -1052,7 1052,7 @@ static CURLcode H2_CONNECT(struct Curl_cfilter *cf,
result = CURLE_OK;
goto out;
}
/* FALLTHROUGH */
FALLTHROUGH();

case H2_TUNNEL_RESPONSE:
DEBUGASSERT(ts->has_final_response);
Expand Down
4 changes: 2 additions & 2 deletions lib/cf-haproxy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 125,7 @@ static CURLcode cf_haproxy_connect(struct Curl_cfilter *cf,
if(result)
goto out;
ctx->state = HAPROXY_SEND;
/* FALLTHROUGH */
FALLTHROUGH();
case HAPROXY_SEND:
len = Curl_dyn_len(&ctx->data_out);
if(len > 0) {
Expand All @@ -141,7 141,7 @@ static CURLcode cf_haproxy_connect(struct Curl_cfilter *cf,
}
}
ctx->state = HAPROXY_DONE;
/* FALLTHROUGH */
FALLTHROUGH();
default:
Curl_dyn_free(&ctx->data_out);
break;
Expand Down
2 changes: 1 addition & 1 deletion lib/cf-https-connect.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 266,7 @@ static CURLcode cf_hc_connect(struct Curl_cfilter *cf,
cf_hc_baller_init(&ctx->h21_baller, cf, data, "h21",
cf->conn->transport);
ctx->state = CF_HC_CONNECT;
/* FALLTHROUGH */
FALLTHROUGH();

case CF_HC_CONNECT:
if(cf_hc_baller_is_active(&ctx->h3_baller)) {
Expand Down
Loading

7 comments on commit 3829759

@gvanem
Copy link
Contributor

@gvanem gvanem commented on 3829759 Dec 16, 2023

Choose a reason for hiding this comment

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

This commit makes the code look like shit IMHO. Why not hide these awful pragmas in a macro:

--- a/tool_easysrc.c 2023-12-16 14:22:16
    b/tool_easysrc.c 2023-12-16 14:54:23
@@ -37,6  37,12 @@

 #include "memdebug.h" /* keep this as LAST include */

 #if defined(__clang__)
   #define CLANG_PRAGMA(x)  _Pragma (#x)
 #else
   #define CLANG_PRAGMA(x)
 #endif
 
 /* global variable definitions, for easy-interface source code generation */

 struct slist_wc *easysrc_decl = NULL; /* Variable declarations */
@@ -113,14  119,14 @@
   char *bufp;
   va_list ap;
   va_start(ap, fmt);
-#ifdef __clang__
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wformat-nonliteral"
-#endif
 
   CLANG_PRAGMA (clang diagnostic push)
   CLANG_PRAGMA (clang diagnostic ignored "-Wformat-nonliteral")
 
   bufp = curlx_mvaprintf(fmt, ap);
-#ifdef __clang__
-#pragma clang diagnostic pop
-#endif
 
   CLANG_PRAGMA (clang diagnostic pop)
 
   va_end(ap);
   if(!bufp) {
     ret = CURLE_OUT_OF_MEMORY;

@vszakats
Copy link
Member Author

@vszakats vszakats commented on 3829759 Dec 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that helps with the "shit" look, but that's entirely subjective.

I prefer the committed version because it's clear at first sight what it
does without having to look up a custom macro, which IMO doesn't add
much. This pattern is used 20 times total, in low-level code that rarely
anybody needs to read.

@gvanem
Copy link
Contributor

@gvanem gvanem commented on 3829759 Dec 16, 2023

Choose a reason for hiding this comment

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

I don't think that helps with the "shit" look, but that's entirely subjective.

I meant no offence, but as you say it's subjective.
But why don't put these one place at the global-level? Or are there other places were these warnings are useful?

@vszakats
Copy link
Member Author

@vszakats vszakats commented on 3829759 Dec 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the premiss of this warning is useful and may catch real issues
where the mask is coming from an input and thus user controlled, which
is exploitable. Thus, disabling it globally wouldn't be the best solution.
Enabling it was in fact the main point of this commit, to follow the (IMO
sensible) recommendation from OpenSSF.

A better path would be to reduce the code which uses this pattern, or
find any upstream calls that may be passing down format strings to
these internal functions without a matching CURL_PRINTF() attribute.

I haven't traced back all of them, and may have missed valid issues, so
some of these warning suppressions may be premature or optimistic.
On the upside they are now trivial to spot or look up in the code.

...a quick review revealed a missing CURL_PRINTF() for imap_sendf(),
and probably all dbg functions could be fixed the same way as done in
tool_ipfs.c in this patch, to avoid passing a variable format string.
Though it will make the code more verbose.

edit: as for the dbg function, it rather looks like it's just a hack to fixup
the format string for various socket type sizes, but we have a macro
for that. We'll see in a PR.

@vszakats
Copy link
Member Author

Choose a reason for hiding this comment

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

PR #12540

@vszakats
Copy link
Member Author

Choose a reason for hiding this comment

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

Continuing on this, it turned out that most of the "shit" pragmas were
redundant after all. After #12812, only six of them remain.

@vszakats
Copy link
Member Author

@vszakats vszakats commented on 3829759 Jan 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Down to 3 with #12814.

Please sign in to comment.