From: Viktor Szakats Date: Sun, 22 Jun 2025 01:17:33 +0000 (+0200) Subject: cmake: replace the way clang-tidy verifies tests, fix issues found X-Git-Tag: curl-8_15_0~182 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e088e104549421914da9170eeead72a43d42c028;p=thirdparty%2Fcurl.git cmake: replace the way clang-tidy verifies tests, fix issues found Replace existing `mk-unity.pl` `--embed` workaround with running `clang-tidy` manually on individual test source instead. This aligns with how clang-tidy works and removes `mk-unity.pl` from the solution. Also: - mqttd: fix potentially uninitialized buffer by zero filling it. ``` tests/server/mqttd.c:484:41: error: The left operand of '<<' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors] 484 | payload_len = (size_t)(buffer[10] << 8) | buffer[11]; | ^ [...] tests/server/mqttd.c:606:45: error: The left operand of '<<' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors] 606 | topiclen = (size_t)(buffer[1 + bytes] << 8) | buffer[2 + bytes]; | ^ ``` - sockfilt: fix potential out-of-bound pointer: ``` tests/server/sockfilt.c:1128:33: error: The 2nd argument to 'send' is a buffer with size 17010 but should be a buffer with size equal to or greater than the value of the 3rd argument (which is 18446744073709551615) [clang-analyzer-unix.StdCLibraryFunctions,-warnings-as-errors] 1128 | ssize_t bytes_written = swrite(sockfd, buffer, buffer_len); | ^ ``` - clang-tidy: suppress bogus `bzero()` warnings that happens inside the notorious `FD_ZERO()` macros, on macOS. Ref: https://github.com/curl/curl/pull/17680#issuecomment-2991730158 Closes #17705 --- diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index 62354742f8..22622b3495 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -38,7 +38,7 @@ permissions: {} env: MAKEFLAGS: -j 5 CURL_CI: github - CURL_CLANG_TIDYFLAGS: '-checks=-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,-clang-analyzer-valist.Uninitialized' + CURL_CLANG_TIDYFLAGS: '-checks=-clang-analyzer-security.insecureAPI.bzero,-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,-clang-analyzer-valist.Uninitialized' # renovate: datasource=github-tags depName=libressl-portable/portable versioning=semver registryUrl=https://github.com LIBRESSL_VERSION: 4.1.0 # renovate: datasource=github-tags depName=wolfSSL/wolfssl versioning=semver extractVersion=^v?(?.+)-stable$ registryUrl=https://github.com diff --git a/CMake/Macros.cmake b/CMake/Macros.cmake index dfc0a95d93..362ccc3dc6 100644 --- a/CMake/Macros.cmake +++ b/CMake/Macros.cmake @@ -95,3 +95,71 @@ macro(curl_prefill_type_size _type _size) set(SIZEOF_${_type} ${_size}) set(SIZEOF_${_type}_CODE "#define SIZEOF_${_type} ${_size}") endmacro() + +# Create a clang-tidy target for test targets +macro(curl_clang_tidy_tests _target) + if(CURL_CLANG_TIDY) + + # Collect header directories and macro definitions from lib dependencies + set(_includes_l "") + set(_definitions_l "") + get_target_property(_libs ${_target} LINK_LIBRARIES) + foreach(_lib IN LISTS _libs) + if(TARGET "${_lib}") + get_target_property(_val ${_lib} INCLUDE_DIRECTORIES) + if(_val) + list(APPEND _includes_l ${_val}) + endif() + get_target_property(_val ${_lib} COMPILE_DEFINITIONS) + if(_val) + list(APPEND _definitions_l ${_val}) + endif() + endif() + endforeach() + + # Collect header directories applying to the target + get_directory_property(_includes_d INCLUDE_DIRECTORIES) + get_target_property(_includes_t ${_target} INCLUDE_DIRECTORIES) + + set(_includes "${_includes_l};${_includes_d};${_includes_t}") + list(REMOVE_ITEM _includes "") + string(REPLACE ";" ";-I" _includes ";${_includes}") + + # Collect macro definitions applying to the target + get_directory_property(_definitions_d COMPILE_DEFINITIONS) + get_target_property(_definitions_t ${_target} COMPILE_DEFINITIONS) + + set(_definitions "${_definitions_l};${_definitions_d};${_definitions_t}") + list(REMOVE_ITEM _definitions "") + string(REPLACE ";" ";-D" _definitions ";${_definitions}") + list(SORT _definitions) # Sort like CMake does + + # Assemble source list + set(_sources "") + foreach(_source IN ITEMS ${ARGN}) + if(NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${_source}") # if not in source tree + set(_source "${CMAKE_CURRENT_BINARY_DIR}/${_source}") # look in the build tree, for generated files, e.g. lib1521.c + endif() + list(APPEND _sources "${_source}") + endforeach() + + add_custom_target("${_target}-clang-tidy" USES_TERMINAL + WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" + COMMAND ${CMAKE_C_CLANG_TIDY} ${_sources} -- ${_includes} ${_definitions} + DEPENDS ${_sources}) + add_dependencies(tests-clang-tidy "${_target}-clang-tidy") + + unset(_includes_d) + unset(_includes_t) + unset(_includes) + unset(_definitions_l) + unset(_definitions_d) + unset(_definitions_t) + unset(_definitions) + unset(_sources) + unset(_source) + unset(_libs) + unset(_lib) + unset(_val) + endif() +endmacro() diff --git a/CMakeLists.txt b/CMakeLists.txt index a20d19a8d2..08c6632669 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -288,11 +288,9 @@ endif() option(CURL_CLANG_TIDY "Run the build through clang-tidy" OFF) if(CURL_CLANG_TIDY) - # clang-tidy is not looking into #included sources, thus not compatible with - # unity builds. Make test bundles compatible by embedding the source code. - set(CMAKE_UNITY_BUILD OFF) - set(CURL_MK_UNITY_OPTION "--embed") + set(CMAKE_UNITY_BUILD OFF) # clang-tidy is not looking into #included sources, thus not compatible with unity builds. set(_tidy_checks "") + list(APPEND _tidy_checks "-clang-analyzer-security.insecureAPI.bzero") # for FD_ZERO() (seen on macOS) list(APPEND _tidy_checks "-clang-analyzer-security.insecureAPI.strcpy") list(APPEND _tidy_checks "-clang-analyzer-optin.performance.Padding") list(APPEND _tidy_checks "-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling") diff --git a/lib/Makefile.am b/lib/Makefile.am index 2f1908046f..41889c955b 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -156,7 +156,7 @@ endif endif # disable the tests that are mostly causing false positives -TIDYFLAGS := -checks=-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling -quiet +TIDYFLAGS := -checks=-clang-analyzer-security.insecureAPI.bzero,-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling -quiet if CURL_WERROR TIDYFLAGS += --warnings-as-errors=* endif diff --git a/scripts/mk-unity.pl b/scripts/mk-unity.pl index a7d23f394f..e1bd39de6d 100755 --- a/scripts/mk-unity.pl +++ b/scripts/mk-unity.pl @@ -31,16 +31,13 @@ use strict; use warnings; if(!@ARGV) { - die "Usage: $0 [--test ] [--include ] [--srcdir ] [--embed]\n"; + die "Usage: $0 [--test ] [--include ]\n"; } my @src; my %include; my $in_include = 0; -my $srcdir = ""; -my $in_srcdir = 0; my $any_test = 0; -my $embed = 0; foreach my $src (@ARGV) { if($src eq "--test") { $in_include = 0; @@ -48,16 +45,6 @@ foreach my $src (@ARGV) { elsif($src eq "--include") { $in_include = 1; } - elsif($src eq "--embed") { - $embed = 1; - } - elsif($src eq "--srcdir") { - $in_srcdir = 1; - } - elsif($in_srcdir) { - $srcdir = $src; - $in_srcdir = 0; - } elsif($in_include) { $include{$src} = 1; push @src, $src; @@ -78,18 +65,7 @@ my $tlist = ""; foreach my $src (@src) { if($src =~ /([a-z0-9_]+)\.c$/) { my $name = $1; - if($embed) { - my $fn = $src; - if($srcdir ne "" && -e "$srcdir/$fn") { - $fn = $srcdir . "/" . $fn; - } - print "/* Embedding: \"$src\" */\n"; - my $content = do { local $/; open my $fh, '<', $fn or die $!; <$fh> }; - print $content; - } - else { - print "#include \"$src\"\n"; - } + print "#include \"$src\"\n"; if(not exists $include{$src}) { # register test entry function $tlist .= " {\"$name\", test_$name},\n"; } diff --git a/src/Makefile.am b/src/Makefile.am index f258a6b28c..bf7a44bed7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -80,7 +80,7 @@ else curlx_src = $(CURLX_CFILES) endif curltool_unity.c: $(top_srcdir)/scripts/mk-unity.pl $(CURL_CFILES) $(curl_cfiles_gen) $(curlx_src) - @PERL@ $(top_srcdir)/scripts/mk-unity.pl --srcdir $(srcdir) --include $(CURL_CFILES) $(curl_cfiles_gen) $(curlx_src) > curltool_unity.c + @PERL@ $(top_srcdir)/scripts/mk-unity.pl --include $(CURL_CFILES) $(curl_cfiles_gen) $(curlx_src) > curltool_unity.c nodist_curl_SOURCES = curltool_unity.c curl_SOURCES = @@ -205,7 +205,7 @@ endif endif # disable the tests that are mostly causing false positives -TIDYFLAGS := -checks=-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling -quiet +TIDYFLAGS := -checks=-clang-analyzer-security.insecureAPI.bzero,-clang-analyzer-security.insecureAPI.strcpy,-clang-analyzer-optin.performance.Padding,-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling -quiet if CURL_WERROR TIDYFLAGS += --warnings-as-errors=* endif diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0cf3e37529..e428076cc5 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -34,6 +34,11 @@ if(BUILD_CURL_EXE) add_dependencies(testdeps curlinfo) endif() +if(CURL_CLANG_TIDY) + add_custom_target(tests-clang-tidy) + add_dependencies(testdeps tests-clang-tidy) +endif() + add_subdirectory(http) add_subdirectory(client) add_subdirectory(server) diff --git a/tests/client/CMakeLists.txt b/tests/client/CMakeLists.txt index 7d980010c0..a2a67eea67 100644 --- a/tests/client/CMakeLists.txt +++ b/tests/client/CMakeLists.txt @@ -31,8 +31,8 @@ if(LIB_SELECTED STREQUAL LIB_STATIC) endif() add_custom_command(OUTPUT "${BUNDLE}.c" - COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" --include ${CURLX_CFILES} --test ${TESTFILES} - ${CURL_MK_UNITY_OPTION} --srcdir "${CMAKE_CURRENT_SOURCE_DIR}" > "${BUNDLE}.c" + COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" + --include ${CURLX_CFILES} --test ${TESTFILES} > "${BUNDLE}.c" DEPENDS "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" "${CMAKE_CURRENT_SOURCE_DIR}/Makefile.inc" ${FIRSTFILES} ${CURLX_CFILES} ${TESTFILES} @@ -48,4 +48,6 @@ target_include_directories(${BUNDLE} PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}" # for the generated bundle source to find included test sources ) set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_NO_OLDIES") -set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF) +set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF C_CLANG_TIDY "") + +curl_clang_tidy_tests(${BUNDLE} ${FIRSTFILES} ${TESTFILES}) diff --git a/tests/libtest/CMakeLists.txt b/tests/libtest/CMakeLists.txt index 9ef237ee0f..7a7e919e33 100644 --- a/tests/libtest/CMakeLists.txt +++ b/tests/libtest/CMakeLists.txt @@ -40,8 +40,8 @@ add_custom_command(OUTPUT "lib1521.c" list(APPEND TESTFILES "lib1521.c") add_custom_command(OUTPUT "${BUNDLE}.c" - COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" --include ${UTILS} ${CURLX_CFILES} --test ${TESTFILES} - ${CURL_MK_UNITY_OPTION} --srcdir "${CMAKE_CURRENT_SOURCE_DIR}" > "${BUNDLE}.c" + COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" + --include ${UTILS} ${CURLX_CFILES} --test ${TESTFILES} > "${BUNDLE}.c" DEPENDS "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" "${CMAKE_CURRENT_SOURCE_DIR}/Makefile.inc" ${FIRSTFILES} ${UTILS} ${CURLX_CFILES} ${TESTFILES} @@ -59,7 +59,9 @@ target_include_directories(${BUNDLE} PRIVATE ) set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "${CURL_DEBUG_MACROS}") set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_NO_OLDIES" "CURL_DISABLE_DEPRECATION") -set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF) +set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF C_CLANG_TIDY "") + +curl_clang_tidy_tests(${BUNDLE} ${FIRSTFILES} ${UTILS} ${TESTFILES} ${STUB_GSS}) if(HAVE_GSSAPI AND UNIX) add_library(stubgss SHARED EXCLUDE_FROM_ALL ${STUB_GSS}) diff --git a/tests/server/CMakeLists.txt b/tests/server/CMakeLists.txt index 1f5136fb44..9467a89c33 100644 --- a/tests/server/CMakeLists.txt +++ b/tests/server/CMakeLists.txt @@ -27,8 +27,8 @@ curl_transform_makefile_inc("Makefile.inc" "${CMAKE_CURRENT_BINARY_DIR}/Makefile include("${CMAKE_CURRENT_BINARY_DIR}/Makefile.inc.cmake") add_custom_command(OUTPUT "${BUNDLE}.c" - COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" --include ${UTILS} ${CURLX_CFILES} --test ${TESTFILES} - ${CURL_MK_UNITY_OPTION} --srcdir "${CMAKE_CURRENT_SOURCE_DIR}" > "${BUNDLE}.c" + COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" + --include ${UTILS} ${CURLX_CFILES} --test ${TESTFILES} > "${BUNDLE}.c" DEPENDS "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" "${CMAKE_CURRENT_SOURCE_DIR}/Makefile.inc" ${FIRSTFILES} ${UTILS} ${CURLX_CFILES} ${TESTFILES} @@ -54,4 +54,6 @@ set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_NO_OLDIE if(WIN32) set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_STATICLIB") endif() -set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF) +set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF C_CLANG_TIDY "") + +curl_clang_tidy_tests(${BUNDLE} ${FIRSTFILES} ${UTILS} ${TESTFILES}) diff --git a/tests/server/mqttd.c b/tests/server/mqttd.c index 2c453f1c6d..fb89ccb77e 100644 --- a/tests/server/mqttd.c +++ b/tests/server/mqttd.c @@ -438,6 +438,7 @@ static curl_socket_t mqttit(curl_socket_t fd) logmsg("Out of memory, unable to allocate buffer"); goto end; } + memset(buffer, 0, buff_size); do { unsigned char usr_flag = 0x80; diff --git a/tests/server/sockfilt.c b/tests/server/sockfilt.c index 93aff96104..c611b9d3bc 100644 --- a/tests/server/sockfilt.c +++ b/tests/server/sockfilt.c @@ -370,7 +370,7 @@ static void lograw(unsigned char *buffer, ssize_t len) * *buffer_len is the amount of data in the buffer (output) */ static bool read_data_block(unsigned char *buffer, ssize_t maxlen, - ssize_t *buffer_len) + ssize_t *buffer_len) { if(!read_stdin(buffer, 5)) return FALSE; @@ -1118,6 +1118,9 @@ static bool juggle(curl_socket_t *sockfdp, if(!read_data_block(buffer, sizeof(buffer), &buffer_len)) return FALSE; + if(buffer_len < 0) + return FALSE; + if(*mode == PASSIVE_LISTEN) { logmsg("*** We are disconnected!"); if(!disc_handshake()) diff --git a/tests/tunit/CMakeLists.txt b/tests/tunit/CMakeLists.txt index 136a1226b4..07e2164c39 100644 --- a/tests/tunit/CMakeLists.txt +++ b/tests/tunit/CMakeLists.txt @@ -27,8 +27,8 @@ curl_transform_makefile_inc("Makefile.inc" "${CMAKE_CURRENT_BINARY_DIR}/Makefile include("${CMAKE_CURRENT_BINARY_DIR}/Makefile.inc.cmake") add_custom_command(OUTPUT "${BUNDLE}.c" - COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" --test ${TESTFILES} - ${CURL_MK_UNITY_OPTION} --srcdir "${CMAKE_CURRENT_SOURCE_DIR}" > "${BUNDLE}.c" + COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" + --test ${TESTFILES} > "${BUNDLE}.c" DEPENDS "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" "${CMAKE_CURRENT_SOURCE_DIR}/Makefile.inc" ${FIRSTFILES} ${TESTFILES} VERBATIM) @@ -47,4 +47,6 @@ target_include_directories(${BUNDLE} PRIVATE ) set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "${CURL_DEBUG_MACROS}") set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_NO_OLDIES" "CURL_DISABLE_DEPRECATION") -set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF) +set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF C_CLANG_TIDY "") + +curl_clang_tidy_tests(${BUNDLE} ${FIRSTFILES} ${TESTFILES}) diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 8077c18ff6..85fe7dbb5a 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -27,8 +27,8 @@ curl_transform_makefile_inc("Makefile.inc" "${CMAKE_CURRENT_BINARY_DIR}/Makefile include("${CMAKE_CURRENT_BINARY_DIR}/Makefile.inc.cmake") add_custom_command(OUTPUT "${BUNDLE}.c" - COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" --test ${TESTFILES} - ${CURL_MK_UNITY_OPTION} --srcdir "${CMAKE_CURRENT_SOURCE_DIR}" > "${BUNDLE}.c" + COMMAND ${PERL_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" + --test ${TESTFILES} > "${BUNDLE}.c" DEPENDS "${PROJECT_SOURCE_DIR}/scripts/mk-unity.pl" "${CMAKE_CURRENT_SOURCE_DIR}/Makefile.inc" ${FIRSTFILES} ${TESTFILES} VERBATIM) @@ -47,4 +47,6 @@ set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "${CURL_DEBUG_ set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "CURL_NO_OLDIES" "CURL_DISABLE_DEPRECATION") # unit tests are small pretend-libcurl-programs set_property(TARGET ${BUNDLE} APPEND PROPERTY COMPILE_DEFINITIONS "BUILDING_LIBCURL") -set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF) +set_target_properties(${BUNDLE} PROPERTIES OUTPUT_NAME "${BUNDLE}" PROJECT_LABEL "Test ${BUNDLE}" UNITY_BUILD OFF C_CLANG_TIDY "") + +curl_clang_tidy_tests(${BUNDLE} ${FIRSTFILES} ${TESTFILES})