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
microsoft · Nov 10, 2022 · 5fdee72 · 5fdee72
1 parent 647c890
commit 5fdee72
Show file tree

Hide file tree

Showing 7 changed files with 124 additions and 72 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -4,6 +4,10 @@ if(NOT CMAKE_MSVC_RUNTIME_LIBRARY)
     set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>: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
@@ -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
@@ -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
@@ -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
@@ -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: (<non-zero constant> && <expression>) always evaluates to the result of <expression>:  Did you intend to use the bitwise-and (&) operator? If not, consider removing the redundant '<non-zero constant>' 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
@@ -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
@@ -1,8 +1,14 @@
+#pragma warning(push)
+// warning C28251: Inconsistent annotation for '_setjmp': this instance has no annotations. See <no file>(0).
+#pragma warning(disable : 28251)
+// warning C28301: No annotations for first declaration of '__fastfail'. See <no file>(0).
+#pragma warning(disable : 28301)
 #include <Windows.h>
 #include <process.h>
 #include <winhttp.h>
 
 #include <Softpub.h>
+#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;