Merge pull request owasp-modsecurity#3283 from eduar-hte/cppcheck2142

Use latest version of cppcheck (2.15.0) to analyze codebase
eduar-hte · Oct 22, 2024 · 29a86b1 · 29a86b1
2 parents ec506da + aca93f5
commit 29a86b1
Show file tree

Hide file tree

Showing 77 changed files with 210 additions and 297 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -178,20 +178,21 @@ jobs:
           ctest -C ${{ matrix.configuration }} --output-on-failure
 
   cppcheck:
-    runs-on: [ubuntu-22.04]
+    runs-on: [macos-14]
     steps:
       - name: Setup Dependencies
         run: |
-          sudo apt-get update -y -qq
-          sudo apt-get install -y cppcheck
-      - name: Checkout source
-        uses: actions/checkout@v4
+          brew install autoconf \
+                       automake \
+                       libtool \
+                       cppcheck
+      - uses: actions/checkout@v4
         with:
           submodules: true
           fetch-depth: 0
-      - name: Configure libModSecurity
+      - name: configure
         run: |
           ./build.sh
           ./configure
-      - name: Run cppcheck on libModSecurity
+      - name: cppcheck
         run: make check-static
diff --git a/Makefile.am b/Makefile.am
@@ -63,9 +63,11 @@ cppcheck:
 		--enable=warning,style,performance,portability,unusedFunction,missingInclude \
 		--inconclusive \
 		--template="warning: {file},{line},{severity},{id},{message}" \
-		-I headers -I . -I others -I src -I others/mbedtls/include -I src/parser \
+		-I headers -I . -I others -I src -I others/mbedtls/include \
 		--error-exitcode=1 \
 		-i "src/parser/seclang-parser.cc" -i "src/parser/seclang-scanner.cc" \
+		-i others \
+		--std=c++17 \
 		--force --verbose .
 
 

diff --git a/examples/multithread/multithread.cc b/examples/multithread/multithread.cc
@@ -40,7 +40,7 @@ int main (int argc, char *argv[]) {
     modsec->setConnectorInformation("ModSecurity-test v0.0.1-alpha (Simple " \
         "example on how to use ModSecurity API");
 
-    char main_rule_uri[] = "basic_rules.conf";
+    const char main_rule_uri[] = "basic_rules.conf";
     auto rules = std::make_unique<modsecurity::RulesSet>();
     if (rules->loadFromUri(main_rule_uri) < 0) {
         std::cerr << "Problems loading the rules..." << std::endl;

diff --git a/headers/modsecurity/anchored_set_variable_translation_proxy.h b/headers/modsecurity/anchored_set_variable_translation_proxy.h
@@ -42,7 +42,7 @@ class AnchoredSetVariableTranslationProxy {
         : m_name(name),
         m_fount(fount)
     {
-        m_translate = [](std::string *name, std::vector<const VariableValue *> *l) {
+        m_translate = [](const std::string *name, std::vector<const VariableValue *> *l) {
             for (int i = 0; i < l->size(); ++i) {
                 VariableValue *newVariableValue = new VariableValue(name, &l->at(i)->getKey(), &l->at(i)->getKey());
                 const VariableValue *oldVariableValue = l->at(i);

diff --git a/headers/modsecurity/rule_with_actions.h b/headers/modsecurity/rule_with_actions.h
@@ -79,7 +79,7 @@ class RuleWithActions : public Rule {
         bool chainedParentNull = false) const;
 
     std::vector<actions::Action *> getActionsByName(const std::string& name,
-        Transaction *t);
+        const Transaction *t);
     bool containsTag(const std::string& name, Transaction *t);
     bool containsMsg(const std::string& name, Transaction *t);
 

diff --git a/headers/modsecurity/rule_with_operator.h b/headers/modsecurity/rule_with_operator.h
@@ -62,7 +62,7 @@ class RuleWithOperator : public RuleWithActions {
     static void cleanMatchedVars(Transaction *trasn);
 
 
-    std::string getOperatorName() const;
+    const std::string& getOperatorName() const;
 
     virtual std::string getReference() override {
         return std::to_string(m_ruleId);

diff --git a/headers/modsecurity/rules.h b/headers/modsecurity/rules.h
@@ -50,7 +50,7 @@ class Rules {
     int append(Rules *from, const std::vector<int64_t> &ids, std::ostringstream *err) {
          size_t j = 0;
          for (; j < from->size(); j++) {
-            RuleWithOperator *rule = dynamic_cast<RuleWithOperator *>(from->at(j).get());
+            const RuleWithOperator *rule = dynamic_cast<RuleWithOperator *>(from->at(j).get());
             if (rule && std::binary_search(ids.begin(), ids.end(), rule->m_ruleId)) {
                  if (err != NULL) {
                      *err << "Rule id: " << std::to_string(rule->m_ruleId) \
@@ -68,7 +68,7 @@ class Rules {
     }
 
     bool insert(std::shared_ptr<Rule> rule, const std::vector<int64_t> *ids, std::ostringstream *err) {
-        RuleWithOperator *r = dynamic_cast<RuleWithOperator *>(rule.get());
+        const RuleWithOperator *r = dynamic_cast<RuleWithOperator *>(rule.get());
         if (r && ids != nullptr && std::binary_search(ids->begin(), ids->end(), r->m_ruleId)) {
             if (err != nullptr) {
                 *err << "Rule id: " << std::to_string(r->m_ruleId) \

diff --git a/headers/modsecurity/rules_set.h b/headers/modsecurity/rules_set.h
@@ -93,7 +93,7 @@ extern "C" {
 #endif
 
 RulesSet *msc_create_rules_set(void);
-void msc_rules_dump(RulesSet *rules);
+void msc_rules_dump(const RulesSet *rules);
 int msc_rules_merge(RulesSet *rules_dst, RulesSet *rules_from, const char **error);
 int msc_rules_add_remote(RulesSet *rules, const char *key, const char *uri,
     const char **error);

diff --git a/headers/modsecurity/rules_set_properties.h b/headers/modsecurity/rules_set_properties.h
@@ -70,7 +70,7 @@ class ConfigInt {
     bool m_set;
     int m_value;
 
-    void merge(ConfigInt *from) {
+    void merge(const ConfigInt *from) {
         if (m_set == true || from->m_set == false) {
             return;
         }
@@ -87,7 +87,7 @@ class ConfigDouble {
     bool m_set;
     double m_value;
 
-    void merge(ConfigDouble *from) {
+    void merge(const ConfigDouble *from) {
         if (m_set == true || from->m_set == false) {
             return;
         }
@@ -104,7 +104,7 @@ class ConfigString {
     bool m_set;
     std::string m_value;
 
-    void merge(ConfigString *from) {
+    void merge(const ConfigString *from) {
         if (m_set == true || from->m_set == false) {
             return;
         }
@@ -150,7 +150,7 @@ class ConfigUnicodeMap {
     static void loadConfig(std::string f, double codePage,
         RulesSetProperties *driver, std::string *errg);
 
-    void merge(ConfigUnicodeMap *from) {
+    void merge(const ConfigUnicodeMap *from) {
         if (from->m_set == false) {
             return;
         }

diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h
@@ -405,7 +405,7 @@ class Transaction : public TransactionAnchoredVariables, public TransactionSecMa
     size_t getRequestBodyLength();
 
 #ifndef NO_LOGS
-    void debug(int, const std::string &) const; // cppcheck-suppress functionStatic
+    void debug(int, const std::string &) const;
 #endif
     void serverLog(const RuleMessage &rm);
 
@@ -713,7 +713,7 @@ int msc_process_uri(Transaction *transaction, const char *uri,
     const char *protocol, const char *http_version);
 
 /** @ingroup ModSecurity_C_API */
-const char *msc_get_response_body(Transaction *transaction);
+const char *msc_get_response_body(const Transaction *transaction);
 
 /** @ingroup ModSecurity_C_API */
 size_t msc_get_response_body_length(Transaction *transaction);

diff --git a/src/actions/ctl/rule_remove_by_id.cc b/src/actions/ctl/rule_remove_by_id.cc
@@ -84,10 +84,10 @@ bool RuleRemoveById::init(std::string *error) {
 }
 
 bool RuleRemoveById::evaluate(RuleWithActions *rule, Transaction *transaction) {
-    for (auto &i : m_ids) {
+    for (const auto &i : m_ids) {
         transaction->m_ruleRemoveById.push_back(i);
     }
-    for (auto &i : m_ranges) {
+    for (const auto &i : m_ranges) {
         transaction->m_ruleRemoveByIdRange.push_back(i);
     }
 

diff --git a/src/actions/exec.h b/src/actions/exec.h
@@ -31,10 +31,7 @@ namespace actions {
 class Exec : public Action {
  public:
     explicit Exec(const std::string &action) 
-        : Action(action),
-        m_script("") { }
-
-    ~Exec() { }
+        : Action(action) { }
 
     bool evaluate(RuleWithActions *rule, Transaction *transaction) override;
     bool init(std::string *error) override;

diff --git a/src/actions/expire_var.cc b/src/actions/expire_var.cc
@@ -32,11 +32,6 @@ namespace modsecurity {
 namespace actions {
 
 
-bool ExpireVar::init(std::string *error) {
-    return true;
-}
-
-
 bool ExpireVar::evaluate(RuleWithActions *rule, Transaction *t) {
 
     std::string expireExpressionExpanded(m_string->evaluate(t));

diff --git a/src/actions/expire_var.h b/src/actions/expire_var.h
@@ -40,7 +40,6 @@ class ExpireVar : public Action {
             m_string(std::move(z)) { }
 
     bool evaluate(RuleWithActions *rule, Transaction *transaction) override;
-    bool init(std::string *error) override;
 
  private:
 

diff --git a/src/actions/set_env.cc b/src/actions/set_env.cc
@@ -26,11 +26,6 @@ namespace modsecurity {
 namespace actions {
 
 
-bool SetENV::init(std::string *error) {
-    return true;
-}
-
-
 bool SetENV::evaluate(RuleWithActions *rule, Transaction *t) {
     std::string colNameExpanded(m_string->evaluate(t));
 

diff --git a/src/actions/set_env.h b/src/actions/set_env.h
@@ -40,7 +40,6 @@ class SetENV : public Action {
             m_string(std::move(z)) { }
 
     bool evaluate(RuleWithActions *rule, Transaction *transaction) override;
-    bool init(std::string *error) override;
 
  private:
     std::unique_ptr<RunTimeString> m_string;

diff --git a/src/actions/set_rsc.cc b/src/actions/set_rsc.cc
@@ -26,11 +26,6 @@ namespace modsecurity {
 namespace actions {
 
 
-bool SetRSC::init(std::string *error) {
-    return true;
-}
-
-
 bool SetRSC::evaluate(RuleWithActions *rule, Transaction *t) {
     std::string colNameExpanded(m_string->evaluate(t));
     ms_dbg_a(t, 8, "RESOURCE initiated with value: \'"

diff --git a/src/actions/set_rsc.h b/src/actions/set_rsc.h
@@ -40,7 +40,6 @@ class SetRSC : public Action {
             m_string(std::move(z)) { }
 
     bool evaluate(RuleWithActions *rule, Transaction *transaction) override;
-    bool init(std::string *error) override;
 
  private:
     std::unique_ptr<RunTimeString> m_string;

diff --git a/src/actions/set_sid.cc b/src/actions/set_sid.cc
@@ -26,11 +26,6 @@ namespace modsecurity {
 namespace actions {
 
 
-bool SetSID::init(std::string *error) {
-    return true;
-}
-
-
 bool SetSID::evaluate(RuleWithActions *rule, Transaction *t) {
     std::string colNameExpanded(m_string->evaluate(t));
     ms_dbg_a(t, 8, "Session ID initiated with value: \'"

diff --git a/src/actions/set_sid.h b/src/actions/set_sid.h
@@ -40,7 +40,6 @@ class SetSID : public Action {
             m_string(std::move(z)) { }
 
     bool evaluate(RuleWithActions *rule, Transaction *transaction) override;
-    bool init(std::string *error) override;
 
  private:
     std::unique_ptr<RunTimeString> m_string;

diff --git a/src/actions/set_uid.cc b/src/actions/set_uid.cc
@@ -26,11 +26,6 @@ namespace modsecurity {
 namespace actions {
 
 
-bool SetUID::init(std::string *error) {
-    return true;
-}
-
-
 bool SetUID::evaluate(RuleWithActions *rule, Transaction *t) {
     std::string colNameExpanded(m_string->evaluate(t));
     ms_dbg_a(t, 8, "User collection initiated with value: \'"

diff --git a/src/actions/set_uid.h b/src/actions/set_uid.h
@@ -40,7 +40,6 @@ class SetUID : public Action {
             m_string(std::move(z)) { }
 
     bool evaluate(RuleWithActions *rule, Transaction *transaction) override;
-    bool init(std::string *error) override;
 
  private:
     std::unique_ptr<RunTimeString> m_string;

diff --git a/src/actions/set_var.cc b/src/actions/set_var.cc
@@ -35,11 +35,6 @@ namespace modsecurity {
 namespace actions {
 
 
-bool SetVar::init(std::string *error) {
-    return true;
-}
-
-
 bool SetVar::evaluate(RuleWithActions *rule, Transaction *t) {
     std::string targetValue;
     std::string resolvedPre;

diff --git a/src/actions/set_var.h b/src/actions/set_var.h
@@ -59,7 +59,6 @@ class SetVar : public Action {
         m_variable(std::move(variable)) { }
 
     bool evaluate(RuleWithActions *rule, Transaction *transaction) override;
-    bool init(std::string *error) override;
 
  private:
     SetVarOperation m_operation;

diff --git a/src/actions/transformations/hex_decode.cc b/src/actions/transformations/hex_decode.cc
@@ -26,7 +26,7 @@ static inline int inplace(std::string &value) {
 
     const auto len = value.length();
     auto d = reinterpret_cast<unsigned char *>(value.data());
-    const auto data = d;
+    const auto *data = d;
 
     for (int i = 0; i <= len - 2; i += 2) {
         *d++ = utils::string::x2c(&data[i]);

diff --git a/src/actions/transformations/html_entity_decode.cc b/src/actions/transformations/html_entity_decode.cc
@@ -118,7 +118,7 @@ static inline bool inplace(std::string &value) {
                     j++;
                 }
                 if (j > k) { /* Do we have at least one digit? */
-                    const auto x = reinterpret_cast<const char*>(&input[k]);
+                    const auto *x = reinterpret_cast<const char*>(&input[k]);
 
                     /* Decode the entity. */
                     /* ENH What about others? */

diff --git a/src/actions/transformations/normalise_path.cc b/src/actions/transformations/normalise_path.cc
@@ -29,9 +29,6 @@ bool NormalisePath::transform(std::string &value, const Transaction *trans) cons
  * IMP1 Assumes NUL-terminated
  */
 bool NormalisePath::normalize_path_inplace(std::string &val, const bool win) {
-    unsigned char *src;
-    unsigned char *dst;
-    unsigned char *end;
     int hitroot = 0;
     int done = 0;
     int relative;
@@ -49,13 +46,13 @@ bool NormalisePath::normalize_path_inplace(std::string &val, const bool win) {
      * ENH: Deal with UNC and drive letters?
      */
 
-    src = dst = input;
-    end = input + (input_len - 1);
+    auto src = input;
+    auto dst = input;
+    const auto *end = input + (input_len - 1);
 
     relative = ((*input == '/') || (win && (*input == '\\'))) ? 0 : 1;
     trailing = ((*end == '/') || (win && (*end == '\\'))) ? 1 : 0;
 
-
     while (!done && (src <= end) && (dst <= end)) {
         /* Convert backslash to forward slash on Windows only. */
         if (win) {
@@ -152,7 +149,7 @@ bool NormalisePath::normalize_path_inplace(std::string &val, const bool win) {
 
         /* Skip to the last forward slash when multiple are used. */
         if (*src == '/') {
-            unsigned char *oldsrc = src;
+            const unsigned char *oldsrc = src;
 
             while ((src < end)
                 && ((*(src + 1) == '/') || (win && (*(src + 1) == '\\'))) ) {

diff --git a/src/actions/transformations/parity_zero_7bit.cc b/src/actions/transformations/parity_zero_7bit.cc
@@ -22,7 +22,7 @@ namespace modsecurity::actions::transformations {
 static inline bool inplace(std::string &value) {
     if (value.empty()) return false;
 
-    for(auto &c : value) {
+    for(auto &c : value) { // cppcheck-suppress constVariableReference ; false positive
         ((unsigned char&)c) &= 0x7f;
     }
 

diff --git a/src/actions/transformations/sql_hex_decode.cc b/src/actions/transformations/sql_hex_decode.cc
@@ -38,7 +38,7 @@ static inline bool inplace(std::string &value) {
 
     auto d = reinterpret_cast<unsigned char*>(value.data());
     const unsigned char *data = d;
-    const auto end = data + value.size();
+    const auto *end = data + value.size();
 
     bool changed = false;
 

diff --git a/src/collection/backend/in_memory-per_process.h b/src/collection/backend/in_memory-per_process.h
@@ -74,7 +74,7 @@ class InMemoryPerProcess :
     public Collection {
  public:
     explicit InMemoryPerProcess(const std::string &name);
-    ~InMemoryPerProcess();
+    ~InMemoryPerProcess() override;
     void store(const std::string &key, const std::string &value);
 
     bool storeOrUpdateFirst(const std::string &key,

diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc
@@ -559,7 +559,7 @@ void LMDB::resolveMultiMatches(const std::string& var,
                 continue;
             }
 
-            char *a = reinterpret_cast<char *>(key.mv_data);
+            const char *a = reinterpret_cast<char *>(key.mv_data);
             if (strncmp(var.c_str(), a, keySize) == 0) {
                 std::string key_to_insert(reinterpret_cast<char *>(key.mv_data), key.mv_size);
                 l->insert(l->begin(), new VariableValue(&m_name, &key_to_insert, &collectionData.getValue()));