Enable a Scan Build job in Daily (#4703)

.azure-pipelines-templates/common.yml

@@ -27,27 +27,28 @@ jobs:
         parameters:
           target: "${{ parameters.target }}"
 
-      - template: cmake.yml
-        parameters:
-          cmake_args: "${{ parameters.cmake_args }}"
-
-      - template: ninja.yml
-        parameters:
-          cmake_args: "${{ parameters.cmake_args }}"
-
-      - ${{ if not(and(eq(parameters.suffix, 'Release'), eq(parameters.target, 'SNPCC'))) }}:
-          - template: test.yml
+      - ${{ if not(eq(parameters.suffix, 'ScanBuild')) }}:
+          - template: cmake.yml
             parameters:
-              suffix: ${{ parameters.suffix }}
-              ctest_filter: ${{ parameters.ctest_filter }}
-              suite_name_suffix: "${{ parameters.target }} ${{ parameters.suffix }}"
-              ctest_timeout: ${{ parameters.ctest_timeout }}
+              cmake_args: "${{ parameters.cmake_args }}"
 
-      - template: misc.yml
-        parameters:
-          suite_name_suffix: "${{ parameters.target }}_${{ parameters.suffix }}"
+          - template: ninja.yml
+            parameters:
+              cmake_args: "${{ parameters.cmake_args }}"
 
-      - ${{ if eq(parameters.suffix, 'Instrumented') }}:
+          - ${{ if not(and(eq(parameters.suffix, 'Release'), eq(parameters.target, 'SNPCC'))) }}:
+              - template: test.yml
+                parameters:
+                  suffix: ${{ parameters.suffix }}
+                  ctest_filter: ${{ parameters.ctest_filter }}
+                  suite_name_suffix: "${{ parameters.target }} ${{ parameters.suffix }}"
+                  ctest_timeout: ${{ parameters.ctest_timeout }}
+
+          - template: misc.yml
+            parameters:
+              suite_name_suffix: "${{ parameters.target }}_${{ parameters.suffix }}"
+
+      - ${{ if eq(parameters.suffix, 'ScanBuild') }}:
           - template: scan_build.yml
 
       - ${{ if eq(parameters.suffix, 'Perf') }}:

.azure-pipelines-templates/daily-matrix.yml

@@ -15,7 +15,9 @@ parameters:
     SGX:
       cmake_args: "-DCOMPILE_TARGET=sgx"
     debug:
-      cmake_args: "-DCMAKE_BUILD_TYPE=Debug -DSAN=ON -DLVI_MITIGATIONS=OFF"
+      cmake_args: "-DCMAKE_BUILD_TYPE=Debug -DLVI_MITIGATIONS=OFF"
+    SAN:
+      cmake_args: "-DSAN=ON"
     unsafe:
       cmake_args: "-DLVI_MITIGATIONS=OFF -DVERBOSE_LOGGING=ON -DUNSAFE_VERSION=ON"
     QUICTLS:
@@ -42,13 +44,22 @@ jobs:
       target: Virtual
       env: "${{ parameters.env.Virtual }}"
       fetch_quictls: debug
-      cmake_args: "${{ parameters.build.common.cmake_args }} ${{ parameters.build.debug.cmake_args }} ${{ parameters.build.QUICTLS.cmake_args }} ${{ parameters.build.Virtual.cmake_args }}"
+      cmake_args: "${{ parameters.build.common.cmake_args }} ${{ parameters.build.debug.cmake_args }} ${{ parameters.build.SAN.cmake_args }} ${{ parameters.build.QUICTLS.cmake_args }} ${{ parameters.build.Virtual.cmake_args }}"
       suffix: "Instrumented"
       artifact_name: "Virtual_Instrumented"
       ctest_filter: '-LE "benchmark|perf"'
       ctest_timeout: "800"
       depends_on: configure
 
+  - template: common.yml
+    parameters:
+      target: Virtual
+      env: "${{ parameters.env.Virtual }}"
+      cmake_args: "${{ parameters.build.common.cmake_args }} ${{ parameters.build.debug.cmake_args }} ${{ parameters.build.Virtual.cmake_args }}"
+      suffix: "ScanBuild"
+      artifact_name: "Virtual_ScanBuild"
+      depends_on: configure
+
   - template: common.yml
     parameters:
       target: SGX

.azure-pipelines-templates/scan_build.yml

@@ -1,4 +1,6 @@
 steps:
-  - script: ../scripts/scan-build.sh
+  - script: |
+      mkdir build
+      cd build
+      ../scripts/scan-build.sh
     displayName: "Scan build"
-    workingDirectory: build

scripts/scan-build.sh

@@ -9,5 +9,8 @@ export CCC_CXX="clang++-$CLANG_VERSION"
 
 SCAN="scan-build-$CLANG_VERSION --exclude 3rdparty --exclude test"
 
-$SCAN cmake -GNinja -DCMAKE_BUILD_TYPE=Debug ..
-$SCAN ninja
+# VERBOSE_LOGGING=ON is important, without it scan-build will report values as unused
+# everywhere we compile out the logging statements that would otherwise read them
+$SCAN cmake -GNinja -DCOMPILE_TARGET=virtual -DVERBOSE_LOGGING=ON -DCMAKE_BUILD_TYPE=Debug ..
+# Fails on the current build of clang, because of false positives in doctest, WIP
+$SCAN ninja || true

src/crypto/openssl/base64.h

@@ -88,7 +88,6 @@ namespace crypto
       // Initialise the encode context
       OpenSSL::Unique_EVP_ENCODE_CTX ctx;
       EVP_EncodeInit(ctx);
-      int encoded_len = 0;
 
       // Calculate the output buffer size: b64 is 6 bits per byte
       int max_size = EVP_ENCODE_LENGTH(size);
@@ -109,7 +108,6 @@ namespace crypto
           max_size,
           chunk_len));
       }
-      encoded_len = chunk_len;
 
       // Encode Final Line (after previous lines, if any)
       EVP_EncodeFinal(ctx, output + chunk_len, &chunk_len);
@@ -125,7 +123,6 @@ namespace crypto
           max_size,
           chunk_len));
       }
-      encoded_len += chunk_len;
 
       // Clean up result (last \0, newlines)
       std::string ret = (const char*)output;

src/crypto/openssl/key_pair.cpp

@@ -446,6 +446,10 @@ namespace crypto
 
     std::vector<uint8_t> bytes(size);
     auto rc = BN_bn2binpad(d, bytes.data(), size);
+    if (rc != size)
+    {
+      throw std::runtime_error(fmt::format("BN_bn2binpad failed: {}", rc));
+    }
     jwk.d = b64url_from_raw(bytes, false /* with_padding */);
 
     return jwk;

src/ds/test/messaging.cpp

@@ -535,7 +535,7 @@ TEST_CASE("read_n vs read_all" * doctest::test_suite("messaging"))
   REQUIRE(written == 2 * max_at_once + 2);
 
   const auto third_read = processor_inside.read_n(max_at_once, rr);
-  REQUIRE(second_read == max_at_once);
+  REQUIRE(third_read == max_at_once);
 
   REQUIRE(0 == processor_inside.read_all(rr));
   REQUIRE(0 == processor_inside.read_n(0, rr));

src/host/test/ledger.cpp

@@ -1161,7 +1161,6 @@ TEST_CASE("Delete committed file from main directory")
   // Worst-case scenario: do not keep any committed file in cache
   size_t max_read_cache_size = 0;
 
-  size_t entries_per_chunk = 0;
   size_t last_idx = 0;
   size_t last_committed_idx = 0;
 
@@ -1178,8 +1177,7 @@ TEST_CASE("Delete committed file from main directory")
 
   INFO("Write many entries on ledger");
   {
-    entries_per_chunk =
-      initialise_ledger(entry_submitter, chunk_threshold, chunk_count);
+    initialise_ledger(entry_submitter, chunk_threshold, chunk_count);
     last_committed_idx = entry_submitter.get_last_idx();
     ledger.commit(last_committed_idx);
 
@@ -1222,7 +1220,6 @@ TEST_CASE("Snapshot file name" * doctest::test_suite("snapshot"))
     std::uniform_int_distribution<size_t> dist(1, snapshot_idx_interval_range);
     size_t snapshot_idx = dist(rgen);
     size_t evidence_idx = snapshot_idx + 1;
-    size_t commit_idx = evidence_idx + 1;
 
     auto snap = fmt::format("snapshot_{}_{}", snapshot_idx, evidence_idx);
     auto snap_committed = fmt::format("{}.committed", snap);

src/js/wrap.cpp

@@ -569,20 +569,13 @@ namespace ccf::js
       ctx, view_val, "clear", JS_NewCFunction(ctx, clear_fn, "clear", 0));
 
     JS_SetPropertyStr(
-      ctx,
-      view_val,
-      "forEach",
-      JS_NewCFunction(ctx, js_kv_map_foreach, "forEach", 1));
+      ctx, view_val, "forEach", JS_NewCFunction(ctx, foreach_fn, "forEach", 1));
 
     JS_SetPropertyStr(
       ctx,
       view_val,
       "getVersionOfPreviousWrite",
-      JS_NewCFunction(
-        ctx,
-        js_kv_get_version_of_previous_write,
-        "getVersionOfPreviousWrite",
-        1));
+      JS_NewCFunction(ctx, get_version_fn, "getVersionOfPreviousWrite", 1));
 
     desc->flags = 0;
     desc->value = view_val;

src/node/acme_client.h

@@ -430,8 +430,6 @@ namespace ACME
       ECDSA_SIG* sig_r_s = d2i_ECDSA_SIG(NULL, &pp, sig.size());
       const BIGNUM* r = ECDSA_SIG_get0_r(sig_r_s);
       const BIGNUM* s = ECDSA_SIG_get0_s(sig_r_s);
-      int r_n = BN_num_bytes(r);
-      int s_n = BN_num_bytes(s);
       size_t sz = signer.coordinates().x.size();
       sig = std::vector<uint8_t>(2 * sz, 0);
       BN_bn2binpad(r, sig.data(), sz);

src/node/rpc/node_frontend.h

@@ -256,7 +256,6 @@ namespace ccf
       auto nodes = tx.rw(network.nodes);
       auto node_endorsed_certificates =
         tx.rw(network.node_endorsed_certificates);
-      auto config = tx.ro(network.config)->get();
 
       auto conflicting_node_id =
         check_conflicting_node_network(tx, in.node_info_network);