Fix static analysis errors (microsoft#325)

poppastring · Nov 20, 2020 · d976f47 · d976f47
1 parent 2660ce7
commit d976f47
Show file tree

Hide file tree

Showing 115 changed files with 783 additions and 468 deletions.
diff --git a/build/ClrInstrumentationEngine.cpp.props b/build/ClrInstrumentationEngine.cpp.props
@@ -17,4 +17,28 @@
       <AdditionalOptions Condition="'$(Platform)'=='Win32'">/SafeSEH %(AdditionalOptions)</AdditionalOptions>
     </Link>
   </ItemDefinitionGroup>
+
+  <!--
+    Disabled preFAST warnings:
+
+    C25007: member function ... can be static.
+    C25021: size of type can be decreased by rearranging its data members
+    C25031: Unchecked HRESULT returned by ...
+    C25046: global variable ... should be declared with '__declspec(selectany)'.
+    C25090: Use 'false' instead of FALSE
+    C25129: Possible truncation on 64bit OS
+    C25164: C++ Reserved global name.
+    C25165: This project forbids the use of a throwing operator new.
+    C25166: This project forbids the use of a non-throwing operator new.
+    
+    -->
+  <PropertyGroup>
+    <CodeAnalysisWarningsToDisable>/wd25007 /wd25021 /wd25031 /wd25046 /wd25090 /wd25129 /wd25164 /wd25165 /wd25166</CodeAnalysisWarningsToDisable>
+  </PropertyGroup>
+
+  <ItemDefinitionGroup Condition="'$(Microbuild_Prefast)'=='true'">
+    <ClCompile>
+      <AdditionalOptions>%(AdditionalOptions) $(CodeAnalysisWarningsToDisable)</AdditionalOptions>
+    </ClCompile>
+  </ItemDefinitionGroup>
 </Project>
diff --git a/build/yaml/jobs/codeanalysis/PolicheckExclusion.xml b/build/yaml/jobs/codeanalysis/PolicheckExclusion.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<!--    When adding new values, please use UPPER CASE - all values will be compared only to the UPPER CASE strings -->
+<PoliCheckExclusions>
+
+  <!-- Each of these exclusions is a folder name - if \[name]\ exists in the file path, it will be skipped -->
+  <!-- <Exclusion Type="FolderPathFull">ABC|XYZ\QWERTY</Exclusion> -->
+
+  <!-- Each of these exclusions is a folder name - if any folder or file starts with "\[name]", it will be skipped -->
+  <!--<Exclusion Type="FolderPathStart">ABC|XYZ</Exclusion>-->
+
+  <!-- Each of these file types will be completely skipped for the entire scan -->
+  <!--<Exclusion Type="FileType">.ABC|.XYZ</Exclusion>-->
+
+  <!-- The specified file names will be skipped during the scan regardless which folder they are in -->
+  <Exclusion Type="FileName">CODESIGNVERIFY.YAML</Exclusion>
+
+</PoliCheckExclusions>
diff --git a/build/yaml/jobs/codeanalysis/apiscan.yaml b/build/yaml/jobs/codeanalysis/apiscan.yaml
@@ -17,9 +17,9 @@ jobs:
 
   workspace:
     clean: all
-
   steps:
-  - checkout: none
+  - checkout: self
+    clean: true
 
   - task: DownloadBuildArtifacts@0
     displayName: Download Windows Files (Release)
@@ -45,7 +45,6 @@ jobs:
       TargetFolder: '$(Build.ArtifactStagingDirectory)\ApiScan'
 
   # Run API Scan
-
   - task: securedevelopmentteam.vss-secure-development-tools.build-task-apiscan.APIScan@2
     displayName: 'Run APIScan'
     inputs:

diff --git a/build/yaml/jobs/codeanalysis/binskim.yaml b/build/yaml/jobs/codeanalysis/binskim.yaml
@@ -17,7 +17,8 @@ jobs:
     clean: all
 
   steps:
-  - checkout: none
+  - checkout: self
+    clean: true
 
   - task: DownloadBuildArtifacts@0
     displayName: Download Windows Files (Release)

diff --git a/build/yaml/jobs/codeanalysis/policheck.yaml b/build/yaml/jobs/codeanalysis/policheck.yaml
@@ -24,6 +24,7 @@ jobs:
     inputs:
       targetType: F
       optionsFC: 1
+      optionsUEPATH: $(Build.SourcesDirectory)\build\yaml\jobs\codeanalysis\PolicheckExclusion.xml
     continueOnError: true
 
   # Publish SecurityAnalysis Logs

diff --git a/build/yaml/steps/codeanalysis/baseline.gdnbaselines b/build/yaml/steps/codeanalysis/baseline.gdnbaselines
@@ -0,0 +1,168 @@
+{
+  "version": "1.0.0",
+  "baselines": {
+    "ApiScanBaseline": {
+      "name": "ApiScanBaseline",
+      "createdDate": "2020-11-06 18:31:35Z",
+      "lastUpdatedDate": "2020-11-06 18:31:35Z"
+    }
+  },
+  "results": {
+    "f9161db938b600dbc540052744fb9f56b4ddb189fa8af2dd3d1440ed877aba27": {
+      "signature": "f9161db938b600dbc540052744fb9f56b4ddb189fa8af2dd3d1440ed877aba27",
+      "target": "x64/instrumentationengine.profilerproxy_x64.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "unpublisheddependency",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "a4f38aba45c6d1cb052b3325324f13734f9556a7ea3a7291b5a56659dedc6ac7": {
+      "signature": "a4f38aba45c6d1cb052b3325324f13734f9556a7ea3a7291b5a56659dedc6ac7",
+      "target": "x64/instrumentationengine.profilerproxy_x64.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "duplicatefunctionnamesdefined",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "5a286c1bd1664a64a5695b91413562ecaaa8b4966f25a63d8928f8612164dc76": {
+      "signature": "5a286c1bd1664a64a5695b91413562ecaaa8b4966f25a63d8928f8612164dc76",
+      "target": "x64/microsoft.instrumentationengine.extensions.base_x64.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "unpublisheddependency",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "0590d0a061affc2c5c0dee04003a41c9fe4cd2eff1ee6263d5b3790c41318193": {
+      "signature": "0590d0a061affc2c5c0dee04003a41c9fe4cd2eff1ee6263d5b3790c41318193",
+      "target": "x64/microsoft.instrumentationengine.extensions.base_x64.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "duplicatefunctionnamesdefined",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "d1704e2466dde92d518f6d55319d49b0b9815838cffec0dc2eff69ae455fa5de": {
+      "signature": "d1704e2466dde92d518f6d55319d49b0b9815838cffec0dc2eff69ae455fa5de",
+      "target": "x64/microsoftinstrumentationengine_x64.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "unpublisheddependency",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "1a3e28074d0e8c9fed99fb1736aad132b85bc23e35f2a4a7a774e4643a7d9e52": {
+      "signature": "1a3e28074d0e8c9fed99fb1736aad132b85bc23e35f2a4a7a774e4643a7d9e52",
+      "target": "x64/microsoftinstrumentationengine_x64.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "duplicatefunctionnamesdefined",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "68fd14ab9bf2f5cf6da3f9b87af45de16cf735a85681bc5b646e5e7947e1393c": {
+      "signature": "68fd14ab9bf2f5cf6da3f9b87af45de16cf735a85681bc5b646e5e7947e1393c",
+      "target": "x86/instrumentationengine.profilerproxy_x86.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "unpublisheddependency",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "606ff1a32562ced566594456285a2c00e47a3483f056da990406e87003e01c4f": {
+      "signature": "606ff1a32562ced566594456285a2c00e47a3483f056da990406e87003e01c4f",
+      "target": "x86/instrumentationengine.profilerproxy_x86.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "duplicatefunctionnamesdefined",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "1e30042cdad939a4605939692e76e2ea51c567ad7749a3078465fcb5c0378918": {
+      "signature": "1e30042cdad939a4605939692e76e2ea51c567ad7749a3078465fcb5c0378918",
+      "target": "x86/microsoft.instrumentationengine.extensions.base_x86.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "unpublisheddependency",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "4b287efe3b35e04f74c818284f5954e06c863a1f67c6ebf1b285cabd3b5d64af": {
+      "signature": "4b287efe3b35e04f74c818284f5954e06c863a1f67c6ebf1b285cabd3b5d64af",
+      "target": "x86/microsoft.instrumentationengine.extensions.base_x86.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "duplicatefunctionnamesdefined",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "d0859c9d9ecc46ee33a83348587ccceddd040a1746c160b872d9a03f229dfafa": {
+      "signature": "d0859c9d9ecc46ee33a83348587ccceddd040a1746c160b872d9a03f229dfafa",
+      "target": "x86/microsoftinstrumentationengine_x86.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "unpublisheddependency",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    },
+    "7810672618fab2fe1b77804887f6938d909346bc0e2f49738a8036f1404831b4": {
+      "signature": "7810672618fab2fe1b77804887f6938d909346bc0e2f49738a8036f1404831b4",
+      "target": "x86/microsoftinstrumentationengine_x86.dll",
+      "memberOf": [
+        "ApiScanBaseline"
+      ],
+      "tool": "APIScanCmd",
+      "ruleId": "duplicatefunctionnamesdefined",
+      "justification": null,
+      "createdDate": "2020-11-06 18:31:35Z",
+      "expirationDate": null,
+      "type": null
+    }
+  }
+}
diff --git a/build/yaml/steps/codeanalysis/csharpsuppressions.txt b/build/yaml/steps/codeanalysis/csharpsuppressions.txt
@@ -2,3 +2,5 @@ CA1707
 CA1303
 CA1031
 CA1822
+CA1016
+CS2008
diff --git a/build/yaml/steps/codeanalysis/postanalysis.yaml b/build/yaml/steps/codeanalysis/postanalysis.yaml
@@ -29,4 +29,6 @@ steps:
     GdnBreakOutputBaseline: MyBL1
     GdnBreakOutputSuppressionFile: MySupp1
     GdnBreakOutputSuppressionSet: MySupp1
+    GdnBreakBaselineFiles: $(Build.SourcesDirectory)\build\yaml\steps\codeanalysis\baseline
+    GdnBreakBaselines: ApiScanBaseline
   condition: succeededOrFailed()
diff --git a/build/yaml/steps/codeanalysis/prefast.yaml b/build/yaml/steps/codeanalysis/prefast.yaml
@@ -8,6 +8,14 @@ steps:
 - task: ms-vseng.MicroBuildShipTasks.9ef02907-718e-41b7-a57e-b10f86130bb8.MicroBuildPrefastPlugin@2
   displayName: 'Install Prefast Plugin'
 
+# Increase the maximum cyclomatic complexity threshold.
+# The cyclomatic complexity is the number of closed loops in a function + 1.
+# A quick way to count this metric is to sum 1 plus the count of {IF FOR WHILE SWITCH BREAK RETURN OPERATOR.QUESTION OPERATOR.AND OPERATOR.OR CATCH} in the function body.
+# Ideally we'd like to simplify the faulting methods and use the default (25), but for now we'll ignore them.
+- powershell: |
+    Write-Host "##vso[task.setvariable variable=_PREFAST_CYCLOMATIC;] 83"
+  displayName: Set _PREFAST_CYCLOMATIC=83
+
 - template: ../windows/binaries.yaml
   parameters:
     NuGetRestore: false
diff --git a/inc/clr/extra/LegacyActivationShim.h b/inc/clr/extra/LegacyActivationShim.h
@@ -1348,7 +1348,10 @@ WRAP_HRESULT_IMPL_BOOLEAN(StrongNameGetPublicKeyEx_HRESULT,
         }
         else
         {
+#pragma warning( push )
+#pragma warning( disable: 25130 ) // 'CoCreateInstance' needs to be reviewed if a non-global or non-const CLSID is passed.
             IfHrFailRet(::CoCreateInstance(rclsid, pUnkOuter, dwClsContext, riid, ppv));
+#pragma warning( pop )
         }
 
         return hr;

diff --git a/inc/clr/extra/LegacyActivationShimUtil.h b/inc/clr/extra/LegacyActivationShimUtil.h
@@ -534,14 +534,14 @@ namespace LegacyActivationShim
         inline
         bool HasNewActivationAPIs()
         {
-            if (ReadAcquire((LONG volatile *)&g_fHasNewActivationAPIs) == LONG(-1))
+            if (ReadAcquire(&g_fHasNewActivationAPIs) == -1l)
             {
                 ICLRMetaHost *pMetaHost = NULL;
                 HRESULT hr = GetCLRMetaHost(&pMetaHost);
                 InterlockedCompareExchange((LONG volatile *)&g_fHasNewActivationAPIs, (LONG)(SUCCEEDED(hr)), ULONG(-1));
             }
 
-            return ReadAcquire((LONG volatile *)&g_fHasNewActivationAPIs) != 0;
+            return ReadAcquire(&g_fHasNewActivationAPIs) != 0l;
         }
 
         // ---CLRMETAHOSTPOLICY INTERFACE DATA-----------------------------------------------------
@@ -1020,7 +1020,7 @@ namespace LegacyActivationShim
         inline
         HRESULT AddStartupFlags(
             ICLRRuntimeInfo *pInfo,
-            LPCWSTR wszBuildFlavor,
+            _In_opt_z_ LPCWSTR wszBuildFlavor,
             DWORD dwStartupFlags,
             LPCWSTR wszHostConfigFile)
         {

diff --git a/inc/clr/prof/clrprofiler.h b/inc/clr/prof/clrprofiler.h
@@ -0,0 +1,5 @@
+#pragma once
+#pragma warning(push) // corprof.h from the coreclr does not include SAL anotations
+#pragma warning(disable : 6313 25057 25155 28718 28204 28722 28723 28729 28740)
+#include "corprof.h"
+#pragma warning(pop)
diff --git a/src/Common.Lib/CriticalSectionHolder.h b/src/Common.Lib/CriticalSectionHolder.h
@@ -50,18 +50,15 @@ namespace CommonLib
      This function checks if a critical section is owned or not.
 
     --*/
-    inline BOOL IsCriticalSectionOwned(
-        _In_ CRITICAL_SECTION* lpCriticalSection
+    inline bool IsCriticalSectionOwned(
+        _In_ const CRITICAL_SECTION* lpCriticalSection
     )
     {
-        BOOL fOwned = FALSE;
         DWORD threadId = GetCurrentThreadId();
 
         //
         // this is actually thread safe
         //
-        fOwned = (DWORD_PTR)lpCriticalSection->OwningThread == threadId;
-
-        return fOwned;
+        return (DWORD_PTR)lpCriticalSection->OwningThread == threadId;
     }
 }
diff --git a/src/Common.Lib/EventLoggingBase.h b/src/Common.Lib/EventLoggingBase.h
@@ -18,7 +18,7 @@ namespace CommonLib
 {
     struct EventLogItem
     {
-        EventLogItem(WORD wType, tstring tsLog)
+        EventLogItem(WORD wType, const tstring& tsLog)
             : wEventType(wType), tsEventLog(tsLog)
         {
         }

diff --git a/src/Common.Lib/InitOnce.h b/src/Common.Lib/InitOnce.h
@@ -25,7 +25,7 @@ namespace CommonLib
         HRESULT m_result;
 
     public:
-        CInitOnce(std::function<HRESULT()> func) : m_isCreated(false), m_func(func), m_result(0) { }
+        CInitOnce(const std::function<HRESULT()>& func) : m_isCreated(false), m_func(func), m_result(0) { }
 
     public:
         HRESULT Get()
@@ -42,7 +42,7 @@ namespace CommonLib
             return m_result;
         }
 
-        bool IsSuccessful()
+        bool IsSuccessful() const
         {
             return m_isCreated.load(std::memory_order_relaxed) && SUCCEEDED(m_result);
         }

diff --git a/src/Extensions.Base.Api.Tests/Extensions.Base.Api.Tests.csproj b/src/Extensions.Base.Api.Tests/Extensions.Base.Api.Tests.csproj
@@ -13,6 +13,7 @@
     <RootNamespace>Extensions.Base.Api.Tests</RootNamespace>
     <AssemblyName>Extensions.Base.Api.Tests</AssemblyName>
     <TargetFrameworkVersion>v4.0</TargetFrameworkVersion>
+    <GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>
     <FileAlignment>512</FileAlignment>
     <ProjectTypeGuids>{3AC096D0-A1C2-E12C-1390-A8335801FDAB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
     <VisualStudioVersion Condition="'$(VisualStudioVersion)' == ''">10.0</VisualStudioVersion>
-Original file line number
+Diff line change
@@ Expand Up / @@ -2,3 +2,5 @@ CA1707 @@
     CA1303
     CA1031
     CA1822
+    CA1016
+    CS2008