Make cppcheck github workflow (#3000)

Currently works with -DBUILD_TESTING=OFF -DBUILD_UTT=OFF -DUSE_LOG4CPP=OFF

Author: toly-kournik

.cppcheck/exitcode-suppressions.txt

@@ -1,8 +1,14 @@
-syntaxError
-preprocessorErrorDirective
-unknownMacro
-missingIncludeSystem
-unmatchedSuppression
+# Suppressions for concord-bft
+# Checked with the following options:
+# cmake -DBUILD_TESTING=OFF -DBUILD_UTT=OFF -DUSE_LOG4CPP=OFF -DCPPCHECK=ON ..
+# TODO [TK] - do we want enable cppckeck for tests?
 
-*:cryptlib.h:*
-*:gtest.h:*
+incorrectStringBooleanError
+uninitMemberVar
+noCopyConstructor
+noOperatorEq
+
+zerodiv:*ReplicasInfo.cpp
+preprocessorErrorDirective:*filesystem.hpp
+*:*/proto/*
+*:*/_deps/*

.cppcheck/suppressions.txt

@@ -57,4 +57,4 @@ jobs:
           with:
             name: artifacts-${{ matrix.ci_build_type }}-${{ matrix.comm_type }}-${{ github.sha }}
             path: artifact/
-            retention-days: 1
+            retention-days: 7

.github/workflows/build_and_test.yml

@@ -0,0 +1,26 @@
+name: cppcheck
+on: [ push, pull_request ]
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  build:
+    name: Build
+    runs-on: ubuntu-22.04
+    container:
+      image: concordbft/concord-bft:latest
+      options: --cap-add=NET_ADMIN
+    strategy:
+      fail-fast: false
+    steps:
+        - name: Checkout
+          uses: actions/checkout@v3
+        - name: cppcheck
+          run: |
+            mkdir build
+            cd build
+            echo $(date +%y-%m-%d_%H-%M-%S) > timestamp
+            cmake -DBUILD_TESTING=OFF -DBUILD_UTT=OFF -DUSE_LOG4CPP=OFF -DCPPCHECK=ON ..
+            make -j$(nproc)

.github/workflows/cppcheck.yml

@@ -38,7 +38,6 @@ option(USE_HTTPLIB "Enable use of httplib" ON)
 option(USE_GRPC "Enable GRPC and Protobuf" ON)
 option(USE_OPENSSL "Enable use of OpenSSL" ON)
 option(BUILD_THIRDPARTY "Whether to build third party libraries or use preinstalled ones" OFF)
-option(CODECOVERAGE "Enable Code Coverage Metrics in Clang" OFF)
 option(ENABLE_RESTART_RECOVERY_TESTS "Enable tests for restart recovery" OFF)
 option(BUILD_UTT "Build UTT library" ON)
 option(BUILD_SHARED_LIBS "whether to create shared libraires" OFF)
@@ -50,131 +49,21 @@ if(SLEEP_FOR_DBG)
     add_definitions(-DSLEEP_DBG)
 endif()
 
+# include compiler specific options
+include(cmake/${CMAKE_CXX_COMPILER_ID}.cmake)
+include(cmake/cppcheck.cmake)
 
-#
-# Compiler options
-#
-#
-
-string(APPEND CMAKE_CXX_FLAGS " -Wall")
-string(APPEND CMAKE_CXX_FLAGS " -Wbuiltin-macro-redefined")
-string(APPEND CMAKE_CXX_FLAGS " -pedantic")
-string(APPEND CMAKE_CXX_FLAGS " -Werror")
-string(APPEND CMAKE_CXX_FLAGS " -fno-omit-frame-pointer")
-
-# At most, only one of the next options should be defined from below ONLY_ONE_OPT_RAISED_CHECK
-set(COUNTER 0)
-set(ONLY_ONE_OPT_RAISED_CHECK LEAKCHECK;THREADCHECK;UNDEFINED_BEHAVIOR_CHECK;CODECOVERAGE;HEAPTRACK)
-foreach(option IN LISTS ONLY_ONE_OPT_RAISED_CHECK)
-  if(${option})
-    MATH(EXPR COUNTER "${COUNTER}+1")
-  endif()
-endforeach()
-if(${COUNTER} GREATER 1)
-  message(FATAL_ERROR "More than one of the following options were chosen: \
-LEAKCHECK=${LEAKCHECK} \
-THREADCHECK=${THREADCHECK} \
-UNDEFINED_BEHAVIOR_CHECK=${UNDEFINED_BEHAVIOR_CHECK} \
-CODECOVERAGE=${CODECOVERAGE} \
-HEAPTRACK=${HEAPTRACK} \
-")
-endif()
-
-if(LEAKCHECK)
-    string(APPEND CMAKE_CXX_FLAGS " -fsanitize=leak -fsanitize=address")
-    add_compile_definitions(RUN_WITH_LEAKCHECK=1)
-    message("-- Address and Leak Sanitizers Enabled")
-elseif(THREADCHECK)
-    string(APPEND CMAKE_CXX_FLAGS " -fsanitize=thread")
-    message("-- Thread Sanitizer Enabled")
-elseif(UNDEFINED_BEHAVIOR_CHECK)
-    string(APPEND CMAKE_CXX_FLAGS " -fsanitize=undefined")
-    message("-- Undefined Behavior Sanitizer Enabled")
-elseif(UNDEFINED_BEHAVIOR_CHECK)
-    string(APPEND CMAKE_CXX_FLAGS " -fsanitize=undefined")
-    message("-- Undefined Behavior Sanitizer Enabled")
-elseif(HEAPTRACK)
-    message("-- Heaptrack Enabled")
-endif()
-
-
-if(OMIT_TEST_OUTPUT)
-    message("-- OMIT_TEST_OUTPUT Enabled")
-endif()
-if(KEEP_APOLLO_LOGS)
-    message("-- KEEP_APOLLO_LOGS Enabled")
-endif()
-if(RUN_APOLLO_TESTS)
-    message("-- RUN_APOLLO_TESTS Enabled")
-endif()
+list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/cmake")
+include(cmake/grpc_utils.cmake)
 
-#
-# Code Quality (static, dynamic, coverage) Analysers
-#
-if(CODECOVERAGE)
-    string(APPEND CMAKE_CXX_FLAGS " -fprofile-instr-generate -fcoverage-mapping")
-    string(APPEND CMAKE_EXE_LINKER_FLAGS " -fprofile-instr-generate")
-    message( "-- Building with llvm Code Coverage Tools")
+if(BUILD_TESTING)
+	include(CTest)
 endif()
 
-if(CPPCHECK)
-    find_program(cppcheck cppcheck HINTS "/usr/local/bin/cppcheck" REQUIRED)
-    message(STATUS "cppcheck ${cppcheck}")
-    if(cppcheck MATCHES "NOTFOUND")
-      message(FATAL_ERROR "failed to find cppcheck executable for CPPCHECK option")
-    endif()
-    # Create <cppcheck> work folder for whole program analysis, for faster analysis and to store some useful debug information
-    # Add cppcheck work folder and reports folder for cppcheck output.
-    file(MAKE_DIRECTORY ${PROJECT_BINARY_DIR}/cppcheck/reports/)
-    # max number of threads = number of CPUs
-    include(ProcessorCount)
-    ProcessorCount(CPU_CORES)
-    set(CMAKE_CXX_CPPCHECK
-          "${cppcheck}"
-          "--enable=all"
-          "--inconclusive"
-          "--inline-suppr"
-          "--quiet"
-          "--std=c++17"
-          "--template=cppcheck1"
-          "--max-configs=1"
-          "--library=boost.cfg"
-          "--library=openssl.cfg"
-          "--library=googletest"
-          "--addon=threadsafety.py"
-          "--cppcheck-build-dir=${PROJECT_BINARY_DIR}/cppcheck/"
-          "--suppressions-list=${CMAKE_CURRENT_SOURCE_DIR}/.cppcheck/suppressions.txt"
-          "--exitcode-suppressions=${CMAKE_CURRENT_SOURCE_DIR}/.cppcheck/exitcode-suppressions.txt"
-        CACHE STRING "Default value for cppcheck CXX_CPPCHECK target property")
-endif(CPPCHECK)
-
 if(USE_S3_OBJECT_STORE)
     add_compile_definitions(USE_S3_OBJECT_STORE=1)
 endif()
-# TODO: Figure out right way to deal with -fstrict-overflow / -Wstrict-overflow related errors
-# string(APPEND CXX_FLAGS " -fno-strict-overflow")
-# Prevents some buffer overflows: https://access.redhat.com/blogs/766093/posts/1976213
-string(APPEND CMAKE_CXX_FLAGS_RELEASE " -D_FORTIFY_SOURCE=2")
-
-string(APPEND CMAKE_CXX_FLAGS_DEBUG " -fstack-protector-all")
-
-
-if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-
-    string(APPEND CMAKE_CXX_FLAGS " -ferror-limit=3")
-
-
-    # Export a compile database for use by semantic analysis tools
-    set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
-
-elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
-    string(APPEND CMAKE_CXX_FLAGS " -fmax-errors=3")
-endif()
-
-list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/cmake")
-include(cmake/grpc_utils.cmake)
 
-include(CTest)
 #
 # Subdirectories
 #
@@ -207,10 +96,18 @@ endif()
 #
 # Setup testing
 #
-
-option(OMIT_TEST_OUTPUT "Forwards output stdout and stdin to /dev/null" OFF)
-
 if(BUILD_TESTING)
+	option(OMIT_TEST_OUTPUT "Forwards output stdout and stdin to /dev/null" OFF)
+	if(OMIT_TEST_OUTPUT)
+	    message("-- OMIT_TEST_OUTPUT Enabled")
+	endif()
+	if(KEEP_APOLLO_LOGS)
+	    message("-- KEEP_APOLLO_LOGS Enabled")
+	endif()
+	if(RUN_APOLLO_TESTS)
+	    message("-- RUN_APOLLO_TESTS Enabled")
+	endif()
+	
     add_subdirectory(bftengine/tests)
     add_subdirectory(tests)
     add_subdirectory(messages)

CMakeLists.txt

@@ -98,10 +98,8 @@ target_include_directories(corebft PUBLIC include/)
 target_include_directories(corebft PUBLIC include/bftengine)
 target_include_directories(corebft PUBLIC include/bcstatetransfer)
 target_include_directories(corebft PUBLIC include/simplestatetransfer)
-target_include_directories(corebft PUBLIC include/metadatastorage)
 target_include_directories(corebft PRIVATE src/bftengine)
 target_include_directories(corebft PRIVATE src/preprocessor)
-target_include_directories(corebft PUBLIC ${perf_include}/performance/include)
 target_include_directories(corebft PUBLIC tests/mocks)
 target_include_directories(corebft PUBLIC ${Boost_INCLUDE_DIR})
 target_link_libraries(corebft PUBLIC

bftengine/CMakeLists.txt

@@ -143,6 +143,7 @@ class CryptoManager : public IKeyExchanger, public IMultiSigKeyGenerator {
       it++;
     }
     LOG_FATAL(logger(), "Cryptosystem not found for checkpoint: " << chckp << "seqnum: " << sn);
+    // cppcheck-suppress incorrectStringBooleanError
     ConcordAssert(false && "should never reach here");
   }
   // create CryptoSys for sn if still doesn't exist

bftengine/include/bftengine/CryptoManager.hpp

@@ -325,7 +325,7 @@ BCStateTran::BCStateTran(const Config &config, IAppState *const stateApi, DataSt
 
   // Make sure that the internal IReplicaForStateTransfer callback is always added, alongside any user-supplied
   // callbacks.
-  addOnTransferringCompleteCallback(
+  BCStateTran::addOnTransferringCompleteCallback(
       [this](uint64_t checkpoint_num) { replicaForStateTransfer_->onTransferringComplete(checkpoint_num); });
 }
 

bftengine/src/bcstatetransfer/BCStateTran.cpp

@@ -31,7 +31,7 @@ class InMemoryDataStore : public DataStore {
  public:
   explicit InMemoryDataStore(uint32_t sizeOfReservedPage);
   ~InMemoryDataStore() override {
-    deleteAllPendingPages();
+    InMemoryDataStore::deleteAllPendingPages();
     pages.clear();
   }
 

bftengine/src/bcstatetransfer/InMemoryDataStore.hpp

@@ -80,7 +80,6 @@ void DescriptorOfLastExitFromView::serializeSimpleParams(char *buf, size_t bufLe
   uint32_t complaintsNum = complaints.size();
   size_t complaintsNumSize = sizeof(complaintsNum);
   memcpy(buf, &complaintsNum, complaintsNumSize);
-  buf += complaintsNumSize;
 
   actualSize = isDefaultSize + viewSize + lastStableSize + lastExecutedSize + stableLowerBoundWhenEnteredToViewSize +
                myViewChangeMsgSize + elementsNumSize + complaintsNumSize;
@@ -150,7 +149,6 @@ void DescriptorOfLastExitFromView::deserializeSimpleParams(char *buf, size_t buf
   uint32_t complaintsNum;
   size_t complaintsNumSize = sizeof(complaintsNum);
   memcpy(&complaintsNum, buf, complaintsNumSize);
-  buf += complaintsNumSize;
 
   if (complaintsNum) complaints.resize(complaintsNum);
 

bftengine/src/bftengine/PersistentStorageDescriptors.cpp

@@ -169,17 +169,15 @@ bool ReplicasInfo::getCollectorsForPartialProofs(const ReplicaId refReplica,
     }
     return (collector == refReplica);
   } else {
-    const int16_t n = _numberOfReplicas;
-    const int16_t c = _cVal;
     const int16_t primary = primaryOfView(v);
 
-    if (c == 0) {
-      int16_t collector = (seq % n);
+    if (_cVal == 0) {
+      int16_t collector = (seq % _numberOfReplicas);
 
       // ignore primary
       if (collector >= primary) {
-        collector = ((collector + 1) % n);
-        if (collector == primary) collector = ((collector + 1) % n);
+        collector = ((collector + 1) % _numberOfReplicas);
+        if (collector == primary) collector = ((collector + 1) % _numberOfReplicas);
       }
 
       if (outNumOfCollectors) {
@@ -189,21 +187,21 @@ bool ReplicasInfo::getCollectorsForPartialProofs(const ReplicaId refReplica,
 
       return (collector == refReplica);
     } else {
-      const int16_t half = (n / 2);
+      const int16_t half = (_numberOfReplicas / 2);
       int16_t collector1 = (seq % half);
       int16_t collector2 = collector1 + half;
       //		int16_t collector1 = (seq % n);
       //		int16_t collector2 = collector1 + 1;
 
       // ignore primary
       if (collector1 >= primary) {
-        collector1 = ((collector1 + 1) % n);
-        if (collector1 == primary) collector1 = ((collector1 + 1) % n);
+        collector1 = ((collector1 + 1) % _numberOfReplicas);
+        if (collector1 == primary) collector1 = ((collector1 + 1) % _numberOfReplicas);
       }
 
       if (collector2 >= primary) {
-        collector2 = ((collector2 + 1) % n);
-        if (collector2 == primary) collector2 = ((collector2 + 1) % n);
+        collector2 = ((collector2 + 1) % _numberOfReplicas);
+        if (collector2 == primary) collector2 = ((collector2 + 1) % _numberOfReplicas);
       }
 
       if (outNumOfCollectors) {

bftengine/src/bftengine/ReplicasInfo.cpp

@@ -128,7 +128,6 @@ class SimpleClientImp : public SimpleClient, public IReceiver {
   uint16_t clientSendsRequestToAllReplicasFirstThresh_;
   uint16_t clientSendsRequestToAllReplicasPeriodThresh_;
   uint16_t clientPeriodicResetThresh_;
-  std::shared_ptr<concord::performance::PerformanceManager> pm_ = nullptr;
 
   logging::Logger logger_ = logging::getLogger("concord.bft.client");
 };
@@ -220,8 +219,7 @@ SimpleClientImp::SimpleClientImp(ICommunication* communication,
                                     p.clientSendsRequestToAllReplicasPeriodThresh),
       clientSendsRequestToAllReplicasFirstThresh_{p.clientSendsRequestToAllReplicasFirstThresh},
       clientSendsRequestToAllReplicasPeriodThresh_{p.clientSendsRequestToAllReplicasPeriodThresh},
-      clientPeriodicResetThresh_{p.clientPeriodicResetThresh},
-      pm_{pm} {
+      clientPeriodicResetThresh_{p.clientPeriodicResetThresh} {
   ConcordAssert(fVal_ >= 1);
 
   timeOfLastTransmission_ = MinTime;

bftengine/src/bftengine/SimpleClientImp.cpp

@@ -19,6 +19,7 @@ constexpr auto RESERVED_PAGE_ID = uint32_t{0};
 namespace bftEngine::impl {
 
 TimeServiceResPageClient::TimeServiceResPageClient() {
+  // cppcheck-suppress incorrectStringBooleanError
   ConcordAssert(res_pages_ != nullptr &&
                 "Reserved pages must be initialized before instantiating TimeServiceResPageClient");
   load();

bftengine/src/bftengine/TimeServiceResPageClient.cpp

@@ -18,9 +18,8 @@ target_include_directories(preprocessor PUBLIC ${bftengine_SOURCE_DIR}/include)
 target_include_directories(preprocessor PUBLIC ${bftengine_SOURCE_DIR}/include/bftengine)
 target_include_directories(preprocessor PUBLIC ${bftengine_SOURCE_DIR}/src/bftengine)
 get_property(perf_include GLOBAL PROPERTY PERF_MANAGER_INCLUDE_DIR)
-get_property(util_include GLOBAL PROPERTY UTIL_INCLUDE_DIR)
 get_property(kvbc_include GLOBAL PROPERTY KVBC_INCLUDE_DIR)
-target_include_directories(preprocessor PUBLIC ${perf_include} ${util_include} ${kvbc_include})
+target_include_directories(preprocessor PUBLIC ${perf_include} ${kvbc_include})
 
 if(BUILD_SLOWDOWN)
     target_compile_definitions(preprocessor PUBLIC USE_SLOWDOWN)

bftengine/src/preprocessor/CMakeLists.txt

@@ -1152,7 +1152,7 @@ TEST(requestPreprocessingState_test, rejectMsgWithInvalidView) {
 int main(int argc, char** argv) {
   ::testing::InitGoogleTest(&argc, argv);
   logging::initLogger("logging.properties");
-  logging::Logger::getInstance("preprocessor_test").setLogLevel(log4cplus::ERROR_LOG_LEVEL);
+  logging::getLogger("preprocessor_test").setLogLevel(logging::ERROR_LOG_LEVEL);
   if (replicaConfig.replicaMsgSigningAlgo == SignatureAlgorithm::EdDSA) {
     replicaPrivKeys = replicaEdDSAPrivKeys;
     replicaPubKeys = replicaEdDSAPubKeys;

bftengine/src/preprocessor/tests/preprocessor_test.cpp

@@ -11,7 +11,6 @@ target_include_directories(bftclient_new PUBLIC include)
 target_include_directories(bftclient_new PUBLIC src)
 target_include_directories(bftclient_new PUBLIC ${bftengine_SOURCE_DIR}/src)
 target_include_directories(bftclient_new PUBLIC ${bftengine_SOURCE_DIR}/include)
-target_include_directories(bftclient_new PUBLIC ${libdiagnostics_SOURCE_DIR}/include)
 target_include_directories(bftclient_new PUBLIC ${libbftcommunication_SOURCE_DIR}/include)
 
 target_link_libraries(bftclient_new PRIVATE