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 curl#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%2B%2B.md

Closes curl#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
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
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
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
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
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
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
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
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
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
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
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
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

0 comments on commit 3829759

Please sign in to comment.