From 5fdee72bc1fceca198fb1ab7589837206a8b81ba Mon Sep 17 00:00:00 2001 From: Billy O'Neal Date: Thu, 10 Nov 2022 13:24:51 -0800 Subject: [PATCH] Turn on CodeQL and fix BinSkim regressions (#805) * Turn on CodeQL as requested by DevDiv. * Remove CMake output spew from CMP0135. * Fix BinSkim failures. * clang-format --- CMakeLists.txt | 81 +++++++++++++++++++++++++++-------- azure-pipelines/pipelines.yml | 9 ++++ azure-pipelines/signing.yml | 13 ++++++ cmake/FindCMakeRC.cmake | 4 ++ cmake/Findfmt.cmake | 21 +++++++++ cmake/utilities.cmake | 50 --------------------- src/tls12-download.c | 18 +++++--- 7 files changed, 124 insertions(+), 72 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6c94184c67..e1a1f7045d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,6 +4,10 @@ if(NOT CMAKE_MSVC_RUNTIME_LIBRARY) set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>") endif() +if(POLICY CMP0135) + cmake_policy(SET CMP0135 NEW) +endif() + # =============== # === Options === # =============== @@ -34,10 +38,6 @@ if(VCPKG_BUILD_TLS12_DOWNLOADER) list(APPEND LANGUAGES "C") endif() -if(VCPKG_DEVELOPMENT_WARNINGS) - set(FMT_PEDANTIC ON CACHE BOOL "") -endif() - if (VCPKG_ARTIFACTS_DEVELOPMENT) # https://gitlab.kitware.com/cmake/cmake/-/issues/20245 cmake_minimum_required(VERSION 3.17) @@ -116,10 +116,54 @@ set(CMAKE_CXX_EXTENSIONS OFF) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_STANDARD 17) if(MSVC) - string(APPEND CMAKE_CXX_FLAGS " /EHsc") - if(CMAKE_BUILD_TYPE STREQUAL "Release") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zi /guard:cf") - set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /DEBUG /debugtype:cv,fixup /guard:cf") + # either MSVC, or clang-cl + string(APPEND CMAKE_C_FLAGS " -FC -permissive- -utf-8 /guard:cf") + string(APPEND CMAKE_CXX_FLAGS " /EHsc -FC -permissive- -utf-8 /guard:cf") + string(APPEND CMAKE_C_FLAGS_RELEASE " /Zi") + string(APPEND CMAKE_CXX_FLAGS_RELEASE " /Zi") + + string(APPEND CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO " /DEBUG /INCREMENTAL:NO /debugtype:cv,fixup /guard:cf") + string(APPEND CMAKE_EXE_LINKER_FLAGS_RELEASE " /DEBUG /INCREMENTAL:NO /debugtype:cv,fixup /guard:cf") + if (MSVC_CXX_ARCHITECTURE_ID STREQUAL "x64") + string(APPEND CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO " /CETCOMPAT") + string(APPEND CMAKE_EXE_LINKER_FLAGS_RELEASE " /CETCOMPAT") + endif() + + if(VCPKG_DEVELOPMENT_WARNINGS) + string(APPEND CMAKE_C_FLAGS " /W4 /sdl") + string(APPEND CMAKE_CXX_FLAGS " /W4 /sdl") + if(VCPKG_COMPILER STREQUAL "clang") + string(APPEND CMAKE_C_FLAGS " -Wmissing-prototypes -Wno-missing-field-initializers") + string(APPEND CMAKE_CXX_FLAGS " -Wmissing-prototypes -Wno-missing-field-initializers") + else() + # -wd6553 is to workaround a violation in the Windows SDK + # c:\program files (x86)\windows kits\10\include\10.0.22000.0\um\winreg.h(780) : warning C6553: The annotation for function 'RegOpenKeyExW' on _Param_(3) does not apply to a value type. + string(APPEND CMAKE_C_FLAGS " -analyze -analyze:stacksize 39000 -wd6553") + string(APPEND CMAKE_CXX_FLAGS " -analyze -analyze:stacksize 39000 -wd6553") + endif() + endif() + + if(VCPKG_WARNINGS_AS_ERRORS) + string(APPEND CMAKE_C_FLAGS " /WX") + string(APPEND CMAKE_CXX_FLAGS " /WX") + endif() +else() + # Neither MSVC nor clang-cl + if(VCPKG_DEVELOPMENT_WARNINGS) + # GCC and clang have different names for the same warning + if(VCPKG_COMPILER STREQUAL "gcc") + set(DECL_WARNING "-Wmissing-declarations") + elseif(VCPKG_COMPILER STREQUAL "clang") + set(DECL_WARNING "-Wmissing-prototypes -Wno-range-loop-analysis") + endif() + + string(APPEND CMAKE_C_FLAGS " -Wall -Wextra -Wpedantic -Wno-unknown-pragmas -Wno-missing-field-initializers ${DECL_WARNING}") + string(APPEND CMAKE_CXX_FLAGS " -Wall -Wextra -Wpedantic -Wno-unknown-pragmas -Wno-missing-field-initializers -Wno-redundant-move ${DECL_WARNING}") + endif() + + if(VCPKG_WARNINGS_AS_ERRORS) + string(APPEND CMAKE_C_FLAGS " -Werror") + string(APPEND CMAKE_CXX_FLAGS " -Werror") endif() endif() @@ -143,12 +187,19 @@ include(GNUInstallDirs) find_package(fmt REQUIRED) find_package(CMakeRC REQUIRED) - - +# === Target: locale-resources === cmrc_add_resource_library(locale-resources ALIAS cmakerc::locales NAMESPACE cmakerc ${LOCALE_RESOURCES}) - - - +if(NOT MSVC) + if(VCPKG_COMPILER STREQUAL "gcc") + target_compile_options(locale-resources PRIVATE + -Wno-missing-declarations + ) + elseif(VCPKG_COMPILER STREQUAL "clang") + target_compile_options(locale-resources PRIVATE + -Wno-missing-prototypes + ) + endif() +endif() # === Target: vcpkglib === @@ -160,7 +211,6 @@ add_library(vcpkglib OBJECT ) target_include_directories(vcpkglib PUBLIC include) -vcpkg_target_add_warning_options(vcpkglib) target_compile_definitions(vcpkglib PUBLIC VCPKG_VERSION=${VCPKG_VERSION} VCPKG_BASE_VERSION=${VCPKG_BASE_VERSION} @@ -354,7 +404,6 @@ add_custom_target(vcpkg-ps1 ALL DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/vcpkg.ps1") add_executable(vcpkg ${VCPKG_SOURCES} "${CMAKE_CURRENT_SOURCE_DIR}/src/vcpkg.manifest") target_link_libraries(vcpkg PRIVATE vcpkglib) -vcpkg_target_add_warning_options(vcpkg) if(VCPKG_ADD_SOURCELINK) if(VCPKG_VERSION STREQUAL "unknownhash") message(FATAL_ERROR "Attempted to add source link information, but there was no git SHA defined. VCPKG_ADD_SOURCELINK only works if VCPKG_EMBED_GIT_SHA is set.") @@ -391,7 +440,6 @@ if (BUILD_TESTING) if(ANDROID) target_link_libraries(vcpkg-test PRIVATE log) endif() - vcpkg_target_add_warning_options(vcpkg-test) add_test(NAME default COMMAND vcpkg-test --order rand --rng-seed time) @@ -405,7 +453,6 @@ endif() if(VCPKG_BUILD_FUZZING) add_executable(vcpkg-fuzz ${VCPKG_FUZZ_SOURCES} "${CMAKE_CURRENT_SOURCE_DIR}/src/vcpkg.manifest") target_link_libraries(vcpkg-fuzz PRIVATE vcpkglib) - vcpkg_target_add_warning_options(vcpkg-fuzz) endif() diff --git a/azure-pipelines/pipelines.yml b/azure-pipelines/pipelines.yml index c968f0126c..3b4b1af48f 100644 --- a/azure-pipelines/pipelines.yml +++ b/azure-pipelines/pipelines.yml @@ -1,3 +1,6 @@ +variables: +- name: Codeql.Enabled + value: true jobs: - job: linux_gcc_9 displayName: 'Ubuntu 20.04 with GCC 9, plus vcpkg-artifacts' @@ -110,6 +113,8 @@ jobs: value: $(Build.ArtifactStagingDirectory)\format.diff - name: 'VCPKG_ROOT' value: $(Build.SourcesDirectory)\vcpkg-root + - name: Codeql.BuildIdentifier + value: vcpkg_cpp steps: - task: Powershell@2 displayName: "Clone vcpkg repo to serve as root" @@ -120,6 +125,8 @@ jobs: $sha = (Get-Content vcpkg-init/vcpkg-scripts-sha.txt -Raw).Trim() git clone https://github.com/microsoft/vcpkg $env:VCPKG_ROOT -n git -C "$env:VCPKG_ROOT" checkout $sha + - task: CodeQL3000Init@0 + displayName: 'CodeQL Initialize' - task: CmdLine@2 displayName: "Build vcpkg with CMake" inputs: @@ -163,3 +170,5 @@ jobs: inputs: PathtoPublish: '$(DiffFile)' ArtifactName: 'format.diff' + - task: CodeQL3000Finalize@0 + displayName: 'CodeQL Finalize' diff --git a/azure-pipelines/signing.yml b/azure-pipelines/signing.yml index 87fafcce5d..4cf1ae6838 100644 --- a/azure-pipelines/signing.yml +++ b/azure-pipelines/signing.yml @@ -27,6 +27,8 @@ parameters: variables: - name: TeamName value: vcpkg + - name: Codeql.Enabled + value: true - group: vcpkg-dependency-source-blobs - name: FMT_TARBALL_URL value: "$(fmt-tarball-url)" @@ -59,9 +61,13 @@ jobs: - ${{ if ne(parameters.VcpkgBaseVersionOverride, 'default') }}: - name: VCPKG_INITIAL_BASE_VERSION value: ${{parameters.VcpkgBaseVersionOverride}} + - name: Codeql.BuildIdentifier + value: vcpkg_ECMAScript pool: name: 'VSEngSS-MicroBuild2022-1ES' steps: + - task: CodeQL3000Init@0 + displayName: 'CodeQL Initialize' - task: Powershell@2 displayName: 'Lock VCPKG_BASE_VERSION' name: versions @@ -184,6 +190,8 @@ jobs: PathtoPublish: '$(Build.ArtifactStagingDirectory)\staging' ArtifactName: 'staging' publishLocation: 'Container' + - task: CodeQL3000Finalize@0 + displayName: 'CodeQL Finalize' - job: macos_build displayName: 'MacOS Build' dependsOn: @@ -272,7 +280,10 @@ jobs: VCPKG_STANDALONE_BUNDLE_SHA: $[ dependencies.arch_independent.outputs['shas.VCPKG_STANDALONE_BUNDLE_SHA'] ] VCPKG_CE_SHA: $[ dependencies.arch_independent.outputs['shas.VCPKG_CE_SHA'] ] VCPKG_BASE_VERSION: $[ dependencies.arch_independent.outputs['versions.VCPKG_BASE_VERSION'] ] + Codeql.BuildIdentifier: vcpkg_cpp steps: + - task: CodeQL3000Init@0 + displayName: 'CodeQL Initialize' - task: CmdLine@2 displayName: "Build vcpkg x86 with CMake" inputs: @@ -456,6 +467,8 @@ jobs: uploadRoslyn: false uploadTSLint: false condition: eq(variables['Build.SourceBranchName'], 'main') + - task: CodeQL3000Finalize@0 + displayName: 'CodeQL Finalize' # Publish everything to a Drop - task: PublishBuildArtifacts@1 displayName: 'Publish Drop' diff --git a/cmake/FindCMakeRC.cmake b/cmake/FindCMakeRC.cmake index 3c38d1845b..509092be53 100644 --- a/cmake/FindCMakeRC.cmake +++ b/cmake/FindCMakeRC.cmake @@ -6,6 +6,10 @@ option(VCPKG_DEPENDENCY_CMAKERC "CMake-based C++ resource compiler" OFF) # with different content. set(VCPKG_CMAKERC_URL "https://github.com/vector-of-bool/cmrc/archive/refs/tags/2.0.1.tar.gz" CACHE STRING "URL to the cmrc release tarball to use.") +if(POLICY CMP0135) + cmake_policy(SET CMP0135 NEW) +endif() + include(FetchContent) FetchContent_Declare( CMakeRC diff --git a/cmake/Findfmt.cmake b/cmake/Findfmt.cmake index e74c0b0352..e9bed00a78 100644 --- a/cmake/Findfmt.cmake +++ b/cmake/Findfmt.cmake @@ -12,6 +12,23 @@ if(NOT VCPKG_FMT_URL) set(VCPKG_FMT_URL "https://github.com/fmtlib/fmt/archive/refs/tags/9.1.0.tar.gz") endif() +if(POLICY CMP0135) + cmake_policy(SET CMP0135 NEW) +endif() + +set(OLD_CXX_FLAGS "${CMAKE_CXX_FLAGS}") +set(SKIP_WARNINGS OFF) +if(MSVC AND VCPKG_DEVELOPMENT_WARNINGS AND NOT (CMAKE_CXX_COMPILER_ID MATCHES "AppleClang") AND NOT (CMAKE_CXX_COMPILER_ID MATCHES "[Cc]lang")) + set(SKIP_WARNINGS ON) + # fmt\core.h(418): warning C6239: ( && ) always evaluates to the result of : Did you intend to use the bitwise-and (&) operator? If not, consider removing the redundant '' and the && operator. + string(APPEND CMAKE_CXX_FLAGS " /wd6239") + # This one is guarded by an assert + # fmt\format-inl.h(327): warning C6385: Reading invalid data from 'pow10_significands'.: Lines: 298, 300, 327 + string(APPEND CMAKE_CXX_FLAGS " /wd6385") + # fmt\os.h(377): warning C6326: Potential comparison of a constant with another constant. + string(APPEND CMAKE_CXX_FLAGS " /wd6326") +endif() + include(FetchContent) FetchContent_Declare( fmt @@ -28,3 +45,7 @@ if(VCPKG_DEPENDENCY_EXTERNAL_FMT) else() FetchContent_MakeAvailable(fmt) endif() + +if(SKIP_WARNINGS) + set(CMAKE_CXX_FLAGS "${OLD_CXX_FLAGS}") +endif() diff --git a/cmake/utilities.cmake b/cmake/utilities.cmake index 6d39848f8f..a975cc4e85 100644 --- a/cmake/utilities.cmake +++ b/cmake/utilities.cmake @@ -35,56 +35,6 @@ On CentOS try the following: endif() endfunction() -function(vcpkg_target_add_warning_options TARGET) - if(MSVC) - # either MSVC, or clang-cl - target_compile_options(${TARGET} PRIVATE -FC -permissive- -utf-8) - - if(VCPKG_DEVELOPMENT_WARNINGS) - target_compile_options(${TARGET} PRIVATE -W4) - if(VCPKG_COMPILER STREQUAL "clang") - target_compile_options(${TARGET} PRIVATE - -Wmissing-prototypes - -Wno-missing-field-initializers - ) - else() - # -wd6553 is to workaround a violation in the Windows SDK - # c:\program files (x86)\windows kits\10\include\10.0.22000.0\um\winreg.h(780) : warning C6553: The annotation for function 'RegOpenKeyExW' on _Param_(3) does not apply to a value type. - target_compile_options(${TARGET} PRIVATE -analyze -analyze:stacksize 39000 -wd6553) - endif() - endif() - - if(VCPKG_WARNINGS_AS_ERRORS) - target_compile_options(${TARGET} PRIVATE -WX) - endif() - else() - if(VCPKG_DEVELOPMENT_WARNINGS) - target_compile_options(${TARGET} PRIVATE - -Wall -Wextra -Wpedantic - -Wno-unknown-pragmas - -Wno-missing-field-initializers - -Wno-redundant-move - ) - - # GCC and clang have different names for the same warning - if(VCPKG_COMPILER STREQUAL "gcc") - target_compile_options(${TARGET} PRIVATE - -Wmissing-declarations - ) - elseif(VCPKG_COMPILER STREQUAL "clang") - target_compile_options(${TARGET} PRIVATE - -Wmissing-prototypes - -Wno-range-loop-analysis - ) - endif() - endif() - - if(VCPKG_WARNINGS_AS_ERRORS) - target_compile_options(${TARGET} PRIVATE -Werror) - endif() - endif() -endfunction() - function(vcpkg_target_add_sourcelink target) cmake_parse_arguments(PARSE_ARGV 1 "arg" "" "REPO;REF" "") if(DEFINED arg_UNPARSED_ARGUMENTS) diff --git a/src/tls12-download.c b/src/tls12-download.c index dd3216c1e9..e80cb5c3c5 100644 --- a/src/tls12-download.c +++ b/src/tls12-download.c @@ -1,8 +1,14 @@ +#pragma warning(push) +// warning C28251: Inconsistent annotation for '_setjmp': this instance has no annotations. See (0). +#pragma warning(disable : 28251) +// warning C28301: No annotations for first declaration of '__fastfail'. See (0). +#pragma warning(disable : 28301) #include #include #include #include +#pragma warning(pop) /* * This program must be as small as possible, because it is committed in binary form to the @@ -47,7 +53,7 @@ static void write_message(const HANDLE std_out, const wchar_t* msg) win32_abort(); } - if (WriteConsoleW(std_out, msg, wchars_to_write, 0, 0)) + if (WriteConsoleW(std_out, msg, (DWORD)wchars_to_write, 0, 0)) { return; } @@ -149,7 +155,7 @@ static void __declspec(noreturn) abort_api_failure(const HANDLE std_out, const w win32_abort(); } -static void set_delete_on_close_flag(const HANDLE std_out, const HANDLE target, BOOL setting) +static void set_delete_on_close_flag(const HANDLE std_out, const HANDLE target, BOOLEAN setting) { FILE_DISPOSITION_INFO fdi = {0}; fdi.DeleteFile = setting; @@ -159,6 +165,11 @@ static void set_delete_on_close_flag(const HANDLE std_out, const HANDLE target, } } +// these are sucked out to avoid +// warning C6262: Function uses '98624' bytes of stack. Consider moving some data to heap. +static wchar_t https_proxy_env[32767]; +static char buffer[32768]; + #ifndef NDEBUG int main() #else // ^^^ debug // !debug vvv @@ -197,7 +208,6 @@ int __stdcall entry() write_message(std_out, L" -> "); write_message(std_out, out_file_path); - wchar_t https_proxy_env[32767]; DWORD access_type; const wchar_t* proxy_setting; const wchar_t* proxy_bypass_setting; @@ -238,7 +248,6 @@ int __stdcall entry() // Setting delete on close before we do anything means the file will get deleted for us if we crash set_delete_on_close_flag(std_out, out_file, TRUE); - BOOL results = FALSE; const HINTERNET session = WinHttpOpen(L"tls12-download/1.0", access_type, proxy_setting, proxy_bypass_setting, 0); if (!session) { @@ -320,7 +329,6 @@ int __stdcall entry() TerminateProcess(GetCurrentProcess(), 2); } - char buffer[32768]; for (;;) { DWORD received_bytes;